chore(firestore): spec test that reproduces the race condition leading to ca9 pendingResponses < 0#9841
chore(firestore): spec test that reproduces the race condition leading to ca9 pendingResponses < 0#9841MarkDuckworth wants to merge 1 commit intomainfrom
Conversation
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the allowUnlistedTargetRemoval configuration to the Firestore spec test framework, enabling tests to handle server-side removals of targets not currently tracked by the client. It includes a new test case for target removal after a re-listen. Feedback focuses on a logic error in spec_builder.ts where the flag incorrectly prevents the removal of active targets from the expected state, and suggests removing unnecessary any type casts now that the property is part of the SpecConfig interface.
| if (!(this.config as any).allowUnlistedTargetRemoval) { | ||
| delete this.activeTargets[this.getTargetId(query)]; | ||
| } |
There was a problem hiding this comment.
This conditional logic appears to be incorrect. If the server sends a watchRemove with a cause (error), the target should be removed from the expected activeTargets regardless of the allowUnlistedTargetRemoval setting. The flag is intended to allow the server to remove targets that the client doesn't currently track as active (suppressing the assertion in the test runner), but if the target is active, it should still be removed from the expectations when a remove event is received. Additionally, the any cast is unnecessary.
delete this.activeTargets[this.getTargetId(query)];| !this.currentStep, | ||
| 'allowUnlistedTargetRemoval() must be called before all spec steps.' | ||
| ); | ||
| (this.config as any).allowUnlistedTargetRemoval = true; |
There was a problem hiding this comment.
The cast to any is unnecessary here because allowUnlistedTargetRemoval has been added to the SpecConfig interface in spec_test_runner.ts.
| (this.config as any).allowUnlistedTargetRemoval = true; | |
| this.config.allowUnlistedTargetRemoval = true; |
1 participant
To run this test:
npx ts-node ./scripts/run-tests.ts --main=test/register.ts --emulator 'test/unit/specs/remote_store_spec.test.ts'