-
Notifications
You must be signed in to change notification settings - Fork 557
test(ordered-collection): Add Fuzz testing for ConsensusOrderedCollection #25829
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
base: main
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This pull request adds fuzz testing infrastructure for the ConsensusOrderedCollection DDS. The implementation follows the established pattern used in other DDS packages within the repository.
Key changes:
- Adds fuzz testing utilities and test suite for ConsensusOrderedCollection
- Implements test infrastructure with operation generators and reducers for fuzz testing
- Updates
applyStashedOpmethod to support stashing operations in fuzz tests
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Adds dependency on @fluid-private/stochastic-test-utils for fuzz testing |
| package.json | Adds @fluid-private/stochastic-test-utils as a dev dependency |
| fuzzUtils.ts | New file implementing fuzz test utilities including operation generators, reducers, and validation logic |
| dirname.cts | New file exporting __dirname for ESM compatibility |
| consensusOrderedCollection.fuzz.spec.ts | New file defining fuzz test suites with and without rebasing |
| consensusOrderedCollection.ts | Changes applyStashedOp from throwing error to empty implementation for stashing support |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Co-authored-by: Copilot <[email protected]>
|
|
||
| protected applyStashedOp(): void { | ||
| throw new Error("not implemented"); | ||
| // empty implementation |
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.
Is this just for testing purposes? Or is nothing actually the right thing to do? I vaguely recall something about this, but I could be misremembering.
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 helpful for local server stress tests (not in this PR yet) since we can't easily disable rehydration scenarios, so throwing an error would prevent us from integrating this DDS into those tests.
I believe throwing/empty implementation should be correct. acquire/release/complete ops depend on the distributed state (i.e. jobTracking, acquireId) that are lost/changed during rehydration. In theory, we may be able to apply stashed add ops since they do not rely on the distributed state, but dropping them should also be okay since they were not ack'd, so the item was never added and we will stay consistent with the distributed state.
The other consensus-based DDSes also don't apply stashed ops (either throw or have an empty implementation), so this would be consistent with those DDSes.
Description
This PR enables fuzz testing for the
ConsensusOrderedCollectionDDS.applyStashedOp()This PR also changes the
applyStashedOpimplementation from throwing "not implemented" to an empty implementation. This allows fuzz testing in rehydration scenarios as well as local server stress tests scenarios. This is also in-line withConsensusRegisterCollection.Alternatively, we can leave the original
applyStashedOpimplementation and disable rehydration scenarios in fuzz testing. However, this will prevent us from integratingConsensusOrderedCollectioninto local server stress tests.Misc
AB#52361