(tree) Added end-to-end test for some basic scenarios in shared tree

[agarwal-navin]

Added some basic end-to-end tests for shared tree. There are a lot of unit tests in the shared tree package and there are examples that use shared tree. However, they are very few tests that use legacy APIs which are primarily consumed by customers using the encapsulated model. Moreover, these tests enable testing real collaboration between clients along with nuances around asynchronous nature of communication between them.
These tests create two Fluid clients (containers) and validates that changes made by one client is successfully synced to the other client.

[Copilot]

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

[CraigMacomber]

This needlessly created a shared directory (as all DataObjects do). You could avoid an extra unrelated DDS using test utils if #23949 was merged (I don't think thats the right approach, but another example of a pattern which adds extra ddses and how to fix it).

#23943 Adds what this should probably use, so again something that doesn't exist yet.

Given the current state of things on main, and that this is called "DataObjectWithTree" so its clearly testing a data object that has a tree, (not is a tree) its probably fine as is, but I wanted to point out that this is involving SharedDirectory when it could be avoided.

[CraigMacomber]

This is going to pull in an unnecessary map in addition to the directory used above. #23949 provides an option to avoid that.

[agarwal-navin]

I don't think it will because I am using my own runtime factory with a DataObject that should only pull in a SharedDirectory.

[CraigMacomber]

I had assumed TestObjectProvider would provide a TestFluidObject. Looks like I was incorrect with that assumption.

[CraigMacomber]

I'm not a big fan of this pattern.

Errors in hooks like beforeEach often aren't reported very well, and it makes it harder to do several kinds of future changes we might want.

For example imagine of one test in here wanted to pass an extra option to the tree configuration, or replace the default point content: thats hard to accomudate with this design.

Additionally it relies on side effects mutating variables, so its not obvious that nothing other than this hook assigns these variables, and from their definitions or use its not always easy to find what sets them.

I'd prefer putting the content of this beforeEach hook into a function which returns its output and is called by the tests that need it.

Something like:

	async function twoViews() {
		const trees = twoTrees();
		const output = {
			...trees,
			treeViewClient1: trees.tree1.viewWith(treeViewConfig),
			treeViewClient2: trees.tree2.viewWith(treeViewConfig),
			initialPoint: new Point({ x: 1, y: 2 }),
		};
		output.treeViewClient1.initialize(initialPoint);
		await trees.provider.ensureSynchronized();
	}

Or even just have both relevant tests inline the relent stuff so each one really looks like a full end to end usage.

That said, while I personally dislike the hook based variable reassigning pattern, I understand that it's pretty common in our tests like this, so I'm not categorically opposed to it, I'd just like to suggest using alternatives.

[CraigMacomber]

I think this would make way more sense as a method on the shape rather than a free function taking in a view.

[CraigMacomber]

Same for removeSide below

[CraigMacomber]

I think this example could use some more documentation. Is "sides" supposed to match the length of points? If so, then that's redundant information we wouldn't normally store in a tree and also this method is wrong because this line is commented out. Also it fails to ensure this invariant across merges, so its a real anti-pattern of how to use tree. If an app does want to maintain a data invariant that's not enforced by schema, it needs transactions and constraints which I don't think is what you are testing there (but might make a good test).

Looks like you have some constraint tests below: might be nice to set up a little documented scenario about it (show the sides invariant doesn't hold then use transactions to fix it). Sadly I think it might be impossible with our current constraints to even maintain this basic invariant since our constraints APi are missing any sufficient restrictive constraints you can use to be conservatively correct.

[CraigMacomber]

this test is about inserting into an array. I don't think using helpers which hide the array insertion behind a separate function is helpful here.

Suppose you wanted to test something very similar like inserting a subsequence of two items, or inserting at the beginning? You would have to rework this significantly.
I think its best when testing a specific thing to do that operation explicitly in the test so you can easily make variants that test similar things, and its easy to see what is being done when reading the test.

Also I'm not sure we really want to bother testing things as specific as inserting into an array in the end to end tests. We have unit tests in the package for every kind of edit:

maybe what makes sense to test here is simply performing an edit and syncing that? In that case calling this method is fine (though I'd still prefer something more direct and simple), but the test should be renamed.

[CraigMacomber]

Wont this pass even if the edit was not applied?

Its generally hard to tell if the test has these kinds of bugs by just looking at it since the edit is hidden behind addSide.

Just taking a tree with a number in it that starts at 1 and setting it to 2 would make for a way easier to understand and validate test.

Personally, though I think if this test contained the explicit initialization of the tree to one state (so you could see the state inline in the test), then made an edit, then asserted the content of the second tree matched an expected result (I recommend using exportConciseTree and comparing that to a JS literal object with assert.depEqual) it would be much easier to follow and tell if its robust.

[CraigMacomber]

How do these tests relate to the ones in packages/service-clients/end-to-end-tests/azure-client/src/test/tree.spec.ts ?

[CraigMacomber]

The doc comment on describeCompat says:

There are three compatVersion options to generate different combinations, depending of the need of the tests:

• FullCompat: generate test variants with compat combinations that varies the version for all layers.
• LoaderCompat: generate test variants with compat combinations that only varies the loader version.
• Specific version (String) : specify a minimum compat version (e.g. "2.0.0-rc.1.0.0") which will be the minimum version a
• test suite will test against. This should be equal to the value of pkgVersion at the time you're writing the new test suite.

"NoCompat" is not an allowed string based on that. Looks like the docs on describeCompat are just wrong?

[microsoft-github-policy-service]

This PR has been automatically marked as stale because it has had no activity for 60 days. It will be closed if no further activity occurs within 8 days of this comment. Thank you for your contributions to Fluid Framework!