Enable toggleMove in base model; fix fuzz utils #25428
Conversation
github-actions
bot
added
area: dds
There was a problem hiding this comment.
Pull Request Overview
This PR fixes several bugs in the SharedArray fuzz testing infrastructure and enables toggleMove operations in the base model. The changes primarily focus on improving tracking of available entry IDs for operations and fixing bugs in operation handling.
- Fix TrackableSharedArray to properly track available entry IDs across all operation types (insert, delete, move, toggle, toggleMove)
- Add value fields to fuzz operations for better debugging capabilities
- Enable toggle and toggleMove operations in the base model and consolidate array fuzz tests into a single comprehensive suite
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| fuzzUtils.ts | Fixes entry ID tracking bugs, adds value fields to operations, and enables toggle operations |
| arrayFuzzTests.spec.ts | Consolidates multiple fuzz test suites into a single comprehensive test |
| array.rollback.spec.ts | Adds test coverage for toggle and toggleMove rollback operations |
| sharedArray.ts | Implements missing toggle and toggleMove rollback functionality and fixes move operation stashing bug |
Comments suppressed due to low confidence (1)
packages/dds/legacy-dds/src/array/sharedArray.ts:1
- The moveIds mapping is inconsistent. Line 151 sets
moveIds.set(op.changedToEntryId, op.entryId)but this should bemoveIds.set(op.entryId, op.changedToEntryId)to maintain the correct old->new mapping convention used elsewhere in the code.
/*!
| index: random.integer(0, Math.max(0, client.channel.get().length - 1)), | ||
| }); | ||
| }: DDSFuzzTestState<SharedArrayFactory<string>>): SharedArrayDelete => { | ||
| const index = random.integer(0, Math.max(0, client.channel.get().length - 1)); |
There was a problem hiding this comment.
Any reason behind this? other fuzz utils use the exact same pattern. Also pick does not seem to be a particularly good option here cause I would need to create an array with all valid indexes which is kinda unnecessary.
| channel.insertIds = new Map<string, unknown>(); | ||
| channel.moveIds = new Map<string, string>(); | ||
| channel.toggleIds = new Map<string, unknown>(); |
There was a problem hiding this comment.
i wonder if we can simplify this, potentially in a follow up, and just keep a single set of ids, and randomly try operations, or only keep track of ids not in sharedarray.get, so basically removed ids. i suggest this, as i'm a little worried about the stress test becoming over structured, which can negatively affect coverage. Stress is especially good for catching hard to predict scenarios, and over-structuring stress can reduce its ability to find those hard to predict scenarios.
There was a problem hiding this comment.
this also seems related to the todo you have in the pr description
There was a problem hiding this comment.
I agree. And precisely this is related to the todo.
| @@ -40,6 +41,7 @@ export interface SharedArrayInsert<T> { | |||
| export interface SharedArrayDelete { | |||
| type: "delete"; | |||
| index: number; | |||
| value: unknown; | |||
There was a problem hiding this comment.
why does delete need the value?
There was a problem hiding this comment.
same for the below. i don't see them used
There was a problem hiding this comment.
it was just for debugging purposes. Given how toggle ops only had entryIds, it was hard to back track what was going on. Had to add value to all ops to make some failing seed scenarios more clear. I think there is value keeping value as a field if there are failures in the future.
This PR contains a few non related changes:
valuefields to fuzz operations for better debugging, since some operations are not descriptive (based on entryIds)TODO: being able to toggle back from remove items.