comparison forest#27660
Conversation
|
Hi! Thank you for opening this PR. Want me to review it? Based on the diff (448 lines, 12 files), I've queued these reviewers:
How this works
|
Bundle size comparisonBase commit: The PR's CI build failed — fix the build and the comment will update once the next run succeeds. |
| const reference = this.reference; | ||
| const mainVisitor = main.acquireVisitor(); | ||
| const referenceVisitor = reference.acquireVisitor(); | ||
| const visitor: DeltaVisitor = { |
There was a problem hiding this comment.
Can you implement this using our existing combineVisitors(visitors: readonly CombinableVisitor[]): CombinedVisitor util?
There was a problem hiding this comment.
maybe combine those two visitors with a third that just has the assertForestsEqual on free?
| @@ -0,0 +1,258 @@ | |||
| /*! | |||
| * Copyright (c) Microsoft Corporation and contributors. All rights reserved. | |||
There was a problem hiding this comment.
this file should be in our test code not in the production code, and should use existing tree comparison utils (I think we have an assertTreeEqual or some exiting way to compare forests) it can use.
There was a problem hiding this comment.
Actually, I guess hooking thus up via a exported Forest type (expensive debug) means this stays out of the bundle for people who don't use it. Maybe this is fine? That does leave us without access to our nice tree comparison utilities with their existing logic for making nice errors.
Maybe moving those into here and reusing them would be a better solution? I would like to avoid having an extra copy of tree compare with nice human facing error message logic.
| * This is implemented iteratively (using an explicit work stack rather than recursion) so that comparing | ||
| * deeply nested trees does not risk exhausting the call stack. | ||
| */ | ||
| function deepEqual(a: unknown, b: unknown): boolean { |
There was a problem hiding this comment.
Avoid trying to deep compare "unknown". Its always going to be problematic (cycles, identify of functions etc) and thus only work for more specific cases, which we should simply describe using the types.
| const mainContent = detachedFieldsContent(main); | ||
| const referenceContent = detachedFieldsContent(reference); | ||
| if (!deepEqual(mainContent, referenceContent)) { | ||
| fail( |
There was a problem hiding this comment.
this is invalid, and CI should detect it. You cannot use fail with a non constant string for the first parameter.
You can however use the API I added where you can have a debug build only second parameter which computes a nicer message
| } | ||
|
|
||
| public allocateCursor(source?: string): ITreeSubscriptionCursor { | ||
| return this.main.allocateCursor(source); |
There was a problem hiding this comment.
I imagine it would be a bit of a pain to implement, but this could validate the cursors match as well, by pairing together both options. Lets not do that yet, but note it as possible:
| return this.main.allocateCursor(source); | |
| // Currently we just use the cursor from "main", but we could use "reference" or a custom cursor ensuring both match in behavior. | |
| // TODO: Add a custom combined cursor type which validates two cursors match, and use it here. | |
| return this.main.allocateCursor(source); |
Description
Adds
ComparisonForestwhich wraps a main forest and a reference forest. Every delta is applied to both forests, and their contents are asserted to be equal after each delta.ForestTypeExpensiveDebugnow usesComparisonForest, and the top-level, move, and undoRedo fuzz tests also run it again it.