I created it in order to provide unit tests that can serve as a test plan for previous differentials in this stack since most of them break the app.
Details
Details
Execute implemented tests.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- marcin/eng-3477
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| native/cpp/CommonCpp/DatabaseManagers/entities/Message.h | ||
|---|---|---|
| 82 ↗ | (On Diff #35895) | I had issues with passing int and std::unique_ptr<int> between JS and C++ so I decided to give up since Messages queries don't use integer fields in Message structure so mocking them with some defaults doesn't affect credibility of tests. If we ever consider to land this differential we can workaround by converting int <-> string |
Comment Actions
Initially I didn't plan to do so but I don't see a reason we shouldn't. One thing to note is that if we want to land unit tests we need a closer review from @kamil since this diff introduces some work he would need to do in future when migrating the rest of redux to SQLite on web so we need to ensure this work is done in a way he would expect to.
Comment Actions
Overall this diff is super helpful, adds tests, and introduces code that will be needed in the future anyway.
Three things:
- Please use camelCase on the JS side.
- remove those default values on WebMessage
- split storeOlmPersistDataWeb into two functions - it will be easier to adapt ops on the web later with this code
| native/cpp/CommonCpp/DatabaseManagers/DatabaseQueryExecutor.h | ||
|---|---|---|
| 19–22 | Maybe it's better to define this in Message.h? | |
| 98 | I would prefer to fo split this into two separate functions (on the web), one for persisting account and one for persisting session. I think when only the session is updated we can avoid persisting the same account again. | |
| native/cpp/CommonCpp/DatabaseManagers/entities/Message.h | ||
| 99 | why this is 0? | |
| web/cpp/SQLiteQueryExecutorBindings.cpp | ||
| 66 | ||
| web/database/types/sqlite-query-executor.js | ||
| 11 | ||
| 27 | ||
| 49–51 | we should user camel case in JS | |
Comment Actions
- Update to camel case in JS
- Move MessageWithMedias type to Message.h
- Split olm account and session persistence.
- Make WebMessage type complete.
Comment Actions
A couple of things but it's very close
| native/cpp/CommonCpp/DatabaseManagers/DatabaseQueryExecutor.h | ||
|---|---|---|
| 80–82 ↗ | (On Diff #36357) | to make it useful on the web I think we should also add the id of both account and session (cc. @michal who is working on moving crypto things to worker on web) |
| native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp | ||
| 1892–1893 ↗ | (On Diff #36357) | Can't find where it was introduced but I think it can be deleted now? (because of D10703) |
| web/cpp/SQLiteQueryExecutorBindings.cpp | ||
| 31 ↗ | (On Diff #36357) | can you in one of the stores add a test that checks this value to make sure everything works? |
| 163–174 ↗ | (On Diff #36357) | I think you can put it above to group it logically |
| web/database/_generated/comm-query-executor.js | ||
| 119 ↗ | (On Diff #36357) | same comment as here: https://phab.comm.dev/D10704#inline-66310 |
| web/database/queries/messages-and-media-queries.test.js | ||
| 156–158 ↗ | (On Diff #36357) | you can use find |
| web/database/queries/olm-persist-data-queries.test.js | ||
| 40 ↗ | (On Diff #36357) | I think it's safer to check value |
| web/database/queries/threads-queries.test.js | ||
| 78 ↗ | (On Diff #36357) | message store threads is something different |
| native/cpp/CommonCpp/DatabaseManagers/DatabaseQueryExecutor.h | ||
|---|---|---|
| 80–82 ↗ | (On Diff #36357) | We can't do it now unfortunately since the implementation of getOlmPersistAccountData will throw an error if if fetches more than two rows it will throw an error. We will be able to do it after https://linear.app/comm/issue/ENG-6510/persist-notifications-crypto-account-in-sqlite-database is done. |
| native/cpp/CommonCpp/DatabaseManagers/DatabaseQueryExecutor.h | ||
|---|---|---|
| 80–82 ↗ | (On Diff #36357) | We should make a note of this on Linear somewhere so it's not missed later |
| native/cpp/CommonCpp/DatabaseManagers/DatabaseQueryExecutor.h | ||
|---|---|---|
| 80–82 ↗ | (On Diff #36357) | I think the task I linked is enough. As a part of this task we will introduce id argument to the function and compilation will force us to update web code as well. |
Comment Actions
- Use find in tests
- Test for isNull correctness for NullableInt
- Fix naming in ThreadStore tests.
- Check for value of NullableString in OlmStore tests
- Reorder olm-related method definitions.
Comment Actions
Good job, this diff is super helpful
| web/database/queries/olm-persist-data-queries.test.js | ||
|---|---|---|
| 8 ↗ | (On Diff #36605) | |