test(tree): add tree change event ordering tests - #27852
Conversation
|
Hi! Thank you for opening this PR. Want me to review it? Based on the diff (518 lines, 2 files), I've queued these reviewers:
How this works
|
|
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output |
|
Jenn (@jenn-le) Looks like this PR has some extraneous changes FYI |
Add a new test file (treeChangeEventOrdering.spec.ts) that validates the ordering guarantees of nodeChanged, treeChanged, and rootChanged events: - Single node: nodeChanged fires before treeChanged on the same node - Parent+child: bottom-up ordering with nodeChanged before treeChanged at each level, tested with 2-level and 3-level hierarchies - rootChanged: fires after nodeChanged/treeChanged (afterBatch listener) - withBufferedTreeEvents: documents that rootChanged fires during the callback (not buffered by KernelEventBuffer) while node events are buffered and flushed after, inverting the normal ordering - Buffered events preserve nodeChanged-before-treeChanged on flush Also replaces the TODO comment in treeChangeEvents.ts with documentation of the bottom-up event ordering behavior. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Factor the repeated TreeBeta.on(node, 'nodeChanged'/treeChanged', ...) pattern into a subscribeToNodeEvents(node, log, prefix?) helper function, reducing boilerplate across all test cases. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…d test Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…adability Replace index-based and loop-based assertions with direct deepEqual comparisons of the full expected event log. This makes the expected ordering immediately visible while also being future-proof — any unexpected extra or reordered events will cause a clear failure. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
fc55d51 to
7eb201d
Compare
| // Both parent and child change in the same buffered batch | ||
| withBufferedTreeEvents(() => { | ||
| root.child.value = 42; | ||
| root.ownProp = "world"; |
There was a problem hiding this comment.
I tried running this test with the edit order switched, and the test failed, so we may have to find a different way to test this (which I don't know either). I chatted with Craig about this a while ago, so I think he may have some good ideas on how to test this
There was a problem hiding this comment.
Actually I made a mistake, it still orders nodeChanged before treeChanged, so this test is good :)
There was a problem hiding this comment.
I added another test and clarified some docs showing that the event order is based on edit order but nodeChanged comes before treeChanged for each node
When both parent and child are directly edited in a buffered batch, events fire in the order nodes were first edited — not strictly bottom-up by tree depth. Updates the doc comment in treeChangeEvents.ts to reflect this more accurate description. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Bundle size comparisonBase commit: Notable changesNo bundles changed by ≥ 500 bytes parsed. Per-bundle deltas
|
Description
Adds tests that assert the ordering guarantees of SharedTree's change events (
nodeChanged,treeChanged,rootChanged) and their interaction withwithBufferedTreeEvents. These tests document and lock in the following behaviors:nodeChangedfires beforetreeChangedon the same noderootChangedfires afternodeChanged/treeChanged(viaafterBatch)withBufferedTreeEventsinvertsrootChangedrelative to buffered node eventsAlso updates a TODO comment in
treeChangeEvents.tsto document the confirmed bottom-up ordering behavior.Reviewer Guidance
Reviewer Guidance Wiki
The test file is self-contained. The only production code change is replacing a TODO with documentation (line 69 of
treeChangeEvents.ts).