Implement unit tests for IDB provider#705
Conversation
…ad of using the transaction" and "IDBKeyValProvider: Index misalignment between pairs and values in multiMerge"
…ectly stored when using multiMerge"
|
cc @VickyStash |
| const provider: StorageProvider = { | ||
| const provider: StorageProvider<UseStore | undefined> = { | ||
| // We don't want to initialize the store while the JS bundle loads as idb-keyval will try to use global.indexedDB | ||
| // which might not be available in certain environments that load the bundle (e.g. electron main process). |
There was a problem hiding this comment.
@VickyStash We have deprecated the App for Electron. So, do we still need this logic?
There was a problem hiding this comment.
Considering starting this PR, all providers are initialized lazily (all providers got a new init method), so I think we would want to keep it this way even with deprecated electron.
@shubham1206agra What do you think?
There was a problem hiding this comment.
Make sense to me, though should we remove (e.g. electron main process)? Maybe a different example would be useful to mention?
|
|
||
| return getValues.then((values) => { | ||
| const pairsWithoutNull = pairs.filter(([key, value]) => { | ||
| pairs.forEach(([key, value], index) => { |
There was a problem hiding this comment.
@VickyStash Please use for ... of here instead of forEach.
|
|
||
| expect(result).toEqual(testObjectWithNullValuesRemoved); | ||
| }); | ||
| describe('objects', () => { |
There was a problem hiding this comment.
@VickyStash Add tests for Date and RegExp here as they are technically objects that should be replaced, not merged.
|
@shubham1206agra Thank you for the feedback, I'll get back to it |
| return true; | ||
| }) as Array<[IDBValidKey, unknown]>; | ||
| return provider.store('readwrite', (store) => { | ||
| pairs.forEach(([key, value]) => { |
There was a problem hiding this comment.
@VickyStash Can you do for...of instead of forEach in every place you made the change?
|
I'll be OOO Dec 20 - 28, and then Dec 31 - Jan 6 🎄 |
|
@Beamanator Bump here |
|
Oops sorry team! Reviewing now! Mostly back from OOO 🙏 |
Beamanator
left a comment
There was a problem hiding this comment.
Overall looking really great here, I think we should move forward!
Details
Proposal: https://expensify.slack.com/archives/C05LX9D6E07/p1765532047195689
This PR:
Related Issues
Expensify/App#71203
Automated Tests
Unit tests were created/updated to cover the fixes above.
Manual Tests
Basic test in E/App:
Author Checklist
### Related Issuessection aboveTestssectiontoggleReportand notonIconClick)myBool && <MyComponent />.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)Avataris modified, I verified thatAvataris working as expected in all cases)mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Screen.Recording.2025-12-11.at.23.01.31-compressed.mov
Android: mWeb Chrome
Screen.Recording.2025-12-11.at.23.06.45-compressed.mov
iOS: Native
Screen.Recording.2025-12-11.at.23.13.24-compressed.mov
iOS: mWeb Safari
Screen.Recording.2025-12-11.at.23.16.46-compressed.mov
MacOS: Chrome / Safari
Screen.Recording.2025-12-11.at.23.19.44-compressed.mov
MacOS: Desktop
Screen.Recording.2025-12-11.at.23.21.32-compressed.mov