test(client-tree): unimplemented "minimize" post-processor#27633
test(client-tree): unimplemented "minimize" post-processor#27633jason-ha wants to merge 9 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 (1188 lines, 11 files), I've queued these reviewers:
How this works
|
| * "Minimizes" a {@link ModularChangeset} so that it contains no extraneous information, i.e. no data that has no net | ||
| * effect on the document. |
There was a problem hiding this comment.
| * "Minimizes" a {@link ModularChangeset} so that it contains no extraneous information, i.e. no data that has no net | |
| * effect on the document. | |
| * "Minimizes" a {@link ModularChangeset} so that it contains no extraneous content, | |
| * i.e. no content that was both created and removed within the same transaction. |
Here's my current view on key terms:
- "Information" could be anything in the changeset (content or not).
- "Content" is data that could exist in the document.
- "Data", without further qualifiers, is ambiguous to me: is it like information or is it like content?
- "Document", without qualifiers, includes all detached content. (I might say "document tree" to indicate just the attached tree.)
So, when read with that dictionary...
- "no extraneous information" is too strong of a statement. For example, changesets include IDs that link the detach and attach portions of a move. These IDs have zero impact on the document, but we're not going to strip that from the changeset.
- "no data that has no net effect on the document", if taken to mean "no content that has no net effect on the attached document tree", is also too strong of a statement. For example, we plan to leave moves that affect detached content.
- "no data that has no net effect on the document", if taken to mean "no content that has no net effect on the whole document, including detached content", is too weak of a statement: it is correct, but it doesn't really describe the impact of the minimizer.
| * This iterates over the change's constituent {@link ModularChangeset}s, | ||
| * replacing each with its {@link minimizeModularChangeset | minimized} form. | ||
| * Schema changes are left unchanged. | ||
| */ |
There was a problem hiding this comment.
Note that, in scenarios where there is more than one data change, we won't be able to minimize the overall change as much: if a data change creates content that a latter data change removes, that content won't be minimized away.
This means we have a choice to make:
Option 1: we throw a usage error if the number of data changes is > 1.
Option 2: we keep this code as is (no error) but we update the docs to weaken the contract.
I don't have a strong opinion.
There was a problem hiding this comment.
The advantage of throwing is that the contract remains simple, and there's no risk that an app author would end up with a solution that is less secure than they thought.
There was a problem hiding this comment.
?Option 3? (that I don't think we must do now) Track ids for all new content over the entire set and react to removed added content.
There was a problem hiding this comment.
Option 1 implemented (and tested)
| 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.
| const { view, stringifiedChange } = runStringArrayScenario(scenarioAThenBInserted); | ||
| assert.deepEqual([...view.root], ["A❤️", "B❤️"]); | ||
| assert.match(stringifiedChange, someSurvivingMarkerRegex); | ||
| }); |
There was a problem hiding this comment.
These tests are looking at the stringified changed. This is unexpected given how they whole describe block is advertised.
There was a problem hiding this comment.
updated describe description
| // observable result of a transaction, so these are expected to PASS regardless of whether minimization is | ||
| // actually implemented. | ||
| describe("preserves the observable result", () => { | ||
| it("keeps inserted nodes", () => { |
There was a problem hiding this comment.
Rather than having a test for each scenario and asserting a specific end state, consider putting all the scenarios in an array (or two arrays) and running them both with and without minimization, then comparing the resulting document tree state.
There was a problem hiding this comment.
both seem best. The current pattern helps ensure tests are good and the comparison give breadth and depth. What is the good way to compare two?
| // The created node is not present in the final document, so its build/destroy should be removed. | ||
| assert.equal(countBuilds(change), 0); | ||
| assert.equal(countDestroys(change), 0); | ||
| }); |
There was a problem hiding this comment.
If we stop worrying about the number of builds and destroys (see comment at the top), then we could put all the scenarios in one (or two) arrays and have a single test implementation that asserts there's no transient markers in the stringified change.
There was a problem hiding this comment.
small history aside: when I developed these, I did so from design that cares about builds and that was the known inspection point to me. Only later did I find path to inspect the serialized changes.
- And neither of the two checks are full-proof. The builds are implementation subjective beyond some point. The serialized change could use an encoding and ☠️ might not show anyway - the serialization logic is far beyond the immediate implementation's working area.
- More tests are generally better than a large one. We want to test small elements to aid in our development - easier to follow and easier to diagnose trouble. We also want (especially with agentic AI) to cover multiple small variations to avoid result hacking. One or two large complicated cases are desirable two.
So I keep 'em.
That said overnight I was thinking that we can use a single schema and there are more cases that only doable with more complicated schema.
I think once we can enable these cases, we could combine most or all of them and not need to double execute scenarios.
| } | ||
|
|
||
| // #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.
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?
…nge in a transaction Revamp test infrastructure to provide Tree access to new schema change transaction scenarios.
(add blank line before steps lists)
|
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output |
Bundle size comparisonBase commit: Notable changesNo bundles changed by ≥ 500 bytes parsed. 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.)