-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[No QA] test: add a test for API.write() persistence ordering #82434
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
13f3abc
3b8870f
a6c264c
27b1e34
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -855,3 +855,59 @@ describe('APITests', () => { | |
| }); | ||
| }); | ||
| }); | ||
|
|
||
| // Issue: https://github.com/Expensify/App/issues/80759 | ||
| describe('API.write() persistence guarantees', () => { | ||
| // BUG: Currently, prepareRequest() applies optimistic data via Onyx.update() BEFORE | ||
| // processRequest() persists the request via SequentialQueue.push(). This test documents | ||
| // the buggy ordering. When the bug is fixed, flip the final assertion to toBe(true). | ||
| test('Issue 1: should persist the request before applying optimistic data', () => | ||
| Onyx.multiSet({ | ||
| [ONYXKEYS.CREDENTIALS]: {autoGeneratedLogin: 'test', autoGeneratedPassword: 'passwd'}, | ||
| [ONYXKEYS.SESSION]: {authToken: 'testToken'}, | ||
| [ONYXKEYS.NETWORK]: {isOffline: true}, | ||
| }).then(() => { | ||
| let optimisticDataApplied = false; | ||
| let requestPersistedBeforeOptimistic = false; | ||
|
|
||
| // Mock Onyx.update so that when it receives our marker key we snapshot the | ||
| // persisted-requests queue. This avoids spy-ordering issues that caused | ||
| // false passes on CI. | ||
| const updateMock = jest.spyOn(Onyx, 'update').mockImplementation((data) => { | ||
| // We use ONYXKEYS.IS_CHECKING_PUBLIC_ROOM as a sample key to identify the marker | ||
| const hasMarker = data.some((entry) => entry.key === ONYXKEYS.IS_CHECKING_PUBLIC_ROOM); | ||
| if (hasMarker) { | ||
| optimisticDataApplied = true; | ||
| // Note: getAll() checks the in-memory queue, not durable (disk) state. | ||
| // This is intentionally a weaker assertion – if even the in-memory | ||
| // ordering is wrong (request not queued before optimistic data), the | ||
| // stronger disk-persistence guarantee is certainly broken too. | ||
| requestPersistedBeforeOptimistic = PersistedRequests.getAll().some((r) => r.command === 'MockCommand'); | ||
| } | ||
| return Promise.resolve(); | ||
| }); | ||
|
|
||
| try { | ||
| API.write('MockCommand' as WriteCommand, {param1: 'value1'} as ApiRequestCommandParameters[WriteCommand], { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The test calls Useful? React with 👍 / 👎. |
||
| optimisticData: [ | ||
| { | ||
| onyxMethod: Onyx.METHOD.SET, | ||
| key: ONYXKEYS.IS_CHECKING_PUBLIC_ROOM, | ||
| value: true, | ||
| }, | ||
| ], | ||
| }); | ||
|
|
||
| // Guard: ensure our mock actually intercepted the optimistic data. | ||
| // Without this, the test could pass for the wrong reason (e.g. mock | ||
| // never fires, flag stays false, and we'd incorrectly confirm the bug). | ||
| expect(optimisticDataApplied).toBe(true); | ||
|
|
||
| // BUG: The request is NOT in the persisted queue when optimistic data is | ||
| // applied. When fixed, this assertion should be changed to toBe(true). | ||
| expect(requestPersistedBeforeOptimistic).toBe(false); | ||
| } finally { | ||
| updateMock.mockRestore(); | ||
| } | ||
| })); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check records success from
PersistedRequests.getAll()during optimisticOnyx.update, butgetAll()only reflects in-memory state, not whether the request has been written to storage. Insrc/libs/actions/PersistedRequests.ts,save()updates the in-memory array before callingOnyx.set(...), so this test can pass even when the request is not yet durable and a crash would still drop it. As written, the test does not actually guard the persistence guarantee described in the PR.Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test is marked
it.failingto document a known bug where the request isn't even in the in-memory queue before optimistic data fires. If the weaker (in-memory) ordering invariant doesn't hold, the stronger (disk durability) guarantee is certainly broken too.Added a clarifying code comment to make this explicit. Once the fix lands and the ordering is corrected,
save()will completeOnyx.set()before optimistic data is applied, satisfying both properties.