test(client-tree): unimplemented "minimize" post-processor#27633
test(client-tree): unimplemented "minimize" post-processor#27633jason-ha wants to merge 20 commits into
Conversation
Creates a "minimize" placeholder for transaction post-process functionality.
Unit tests are added to cover the intended new functionality with unsupported tests disabled. (Since implementation is not expected to change observable result tests for observable result can be enabled with the simple do-nothing implementation that is the placeholder.)
```text
transaction minimize post-processor
✔ can be supplied as a transaction post-processor without error
✔ preserves the observable result across an async transaction
self-tests - no minimization applicable
✔ embeds surviving markers but not transient marker for a purely additive scenario
✔ result carries no build when pre-existing content is only rearranged
✔ result carries no build when pre-existing content is only removed
✔ reflects the order of only-rearranged inserted nodes and keeps every build
preserves the observable result
✔ keeps inserted nodes
✔ nets a create-then-remove to no change
✔ keeps only the persisted node when a transient node is also created
✔ reflects only the final value of a node replaced within the transaction
✔ reflects only the surviving node when inserted content is relocated then removed
✔ keeps the surrounding nodes when a node in the middle of an inserted run is removed
✔ keeps the surrounding nodes when an inserted node is moved then removed
✔ keeps only the trailing node when a moved node and its successor from leading node are removed
✔ keeps only the leading node when a moved node and its insertion companion are removed
✔ leaves pre-existing content unchanged when a transient node is inserted then removed
✔ keeps pre-existing content and the surviving inserted node
✔ reflects only the final value of a field set multiple times
✔ reflects only the final empty array when only item's value of a field is set and then the item is removed
removes extraneous data from the squashed change (expected to fail until minimize is implemented)
- drops the build and destroy for a create-then-remove
- keeps only the persisted node's build when a transient node is also created
- keeps only the final node's build when a node is replaced
- keeps only the surviving node's build when inserted content is relocated then removed
- keeps the surrounding builds when a node in the middle of an inserted run is removed
- drops the build for an inserted node that is moved then removed
- keeps only the trailing node's [modified] build when a moved node and its successor from leading node build are removed
- keeps only the leading node's build when a moved node and its insertion companion are removed
- carries no build for a transient insert over pre-existing content
- keeps only the surviving inserted node's build over pre-existing content
- keeps only the final value's build when a field is set multiple times
- carries no build when only item's value of a field is set and then the item is removed
```
|
Hi! Thank you for opening this PR. Want me to review it? Based on the diff (1696 lines, 12 files), I've queued these reviewers:
How this works
|
| for (const inner of change.changes) { | ||
| if (inner.type === "data") { | ||
| total += inner.innerChange.builds?.size ?? 0; | ||
| } |
There was a problem hiding this comment.
Note that each build may be building 1+ nodes. I'm not sure if the current tests care about that, but we probably should have some tests that do, so we'll want to sum the top-level length of the chunk on each build entry.
There was a problem hiding this comment.
The fact that the tests actually look at the JSON for the changeset make this less of an issue. I'm fine leaving it as is. In fact, do we even care about the number of builds and destroys?
There was a problem hiding this comment.
Unfortunately inspecting JSON with hopes of not finding content (our goal) is unreliable. Negative tests are unreliable.
That is mitigated with some positive tests even if those are fussy.
Using the "sum the top-level length of the chunk on each build entry" seems like it should make things less fussy. Will attempt.
There was a problem hiding this comment.
I've added top-level length checking as well.
No too interesting at the moment, but could be with more complicated cases.
| } | ||
|
|
||
| // #region Scenario definitions | ||
| // Each scenario declares the initial document content and the edits applied to the strongly-typed root node |
There was a problem hiding this comment.
We need a systematic way to think about what scenarios are needed. Was the current set of scenarios based on such a thing?
There was a problem hiding this comment.
The collection should cover all fundamental cases where some content is added while under transaction but ultimately is not going to be exhaustive.
There was a problem hiding this comment.
I think the revised Object scenarios have good coverage now. Each scenario has a classification now.
A glaring omission from object tests are movements.
But I think these are good to get started with.
There was a problem hiding this comment.
Another set of test we want to run is checking that the end state of all peers is consistent when one peer minimizes a transaction. You can test that by having a test for each scenario where the test is applied on one peer and the peers are then synchronized. You can use validateViewConsistency to validate that both peers get the same state.
I expect these tests will start failing once you implement minimization. That's because we're going to need to roll back the changes applied to the local branch and apply the minimized change. I forgot about that when I reviewed #27610.
There was a problem hiding this comment.
You'll have to tell me more. Follow-up PR?
There was a problem hiding this comment.
#27673 has fixes for prior problem.
I have also added some view consistency checks into these cases.
…nge in a transaction Revamp test infrastructure to provide Tree access to new schema change transaction scenarios.
(add blank line before steps lists)
Fixes comma insertion between `ABC` (previously -> `A,BC`; now -> `A,B,C`)
|
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output |
Bundle size comparisonBase commit: Notable changes
Per-bundle deltas
|
Creates a "minimize" placeholder for transaction post-process functionality.
Unit tests are added to cover the intended new functionality with unsupported tests disabled. (Since implementation is not expected to change observable result tests for observable result can be enabled with the simple do-nothing implementation that is the placeholder.)
Additionally bumps
treetest lib to ES2022 so thatString.replaceAllis available.