test(tree): Clean up and expand toMapTree unit tests

[Josmithr]

Also cleans up the way MockNodeKeyManager is exposed and consumed.

AB#7597

[Josmithr]

This should not be exported. Tests should operate in terms of the root nodeDataToMapTree function.

[CraigMacomber]

It seems perfectly reasonable to explicitly unit test this function and the logic in nodeDataToMapTree separately. Its fine if the testing strategy chooses not to, but is there a reason tests "should" not be written that way or is it just that the tests are not written that way?

For example if unit testing how default field providing works, why not just test this function?

Also as this is the node kind specific part of this logic, I think we are planning to split it out to live with the objectNode code, which would result in it probably having its own dedicated tests, so it seems like taking such a pattern now might make that refactoring easier.

[Josmithr]

Generally it's just the "you should unit test the public API, and (generally) not private implementation details
rule of thumb. If we choose to organize the code differently, I suppose it makes sense to make more aspects of this "public". But all of the other tests go through the main entrypoint, and I think consistency is valuable.

[Josmithr]

I can put this back if we think it's important, but I would prefer not to if it can wait for the refactoring you mentioned.

[CraigMacomber]

Testing that things do what their docs say they do is my interpretation of that rule of thumb. This comment seems to imply we should do that here.

Name and documented functions with well defined behavior like this are what I think of as testable units, the thing we unit test.

Generally any function containing a lot of conditional logic (like this one does) ends up requiring quite a few tests, and delegating that coverage to some higher level thing ends up getting messy. For example if we end up replacing nodeDataToMapTree with something else, having it use something else in its implementation, or adding another caller to objectToMapTree, tests targeted at this function (ex: for things like object node optional fields) would be fine to keep as it, but ones aimed at nodeDataToMapTree might end up being insufficient in hard to notice ways.

Its also possible that objectToMapTree's docs imply some assumptions that nodeDataToMapTree currently doesn't rely on. If nodeDataToMapTree is changes to rely on those assumptions, or someone adds a new caller, how would we expect the author of that change to know they need to add more tests?

Testing that a documented thing does what it's docs say is a good way to keep assumption devs make about code and things we test about it aligned. Doing this in a location in /test parallel to the source location ensures you can easily tell what tests already exist and adjust them if needed when making a change.

Explicitly avoiding testing that this function behaves as documented to instead test a different non public function that currently happens to call it could be a valid testing strategy (and in that case removing the export to prevent new callers is good), but it seems less robust than testing that things do what their docs say they do, which is my interpretation of that rule of thumb. If we do want to go with this approach, I'd recommend including in the test for this file (possible in an empty suite for this function) a comment noting that the tests for nodeDataToMapTree are covering the documented behaviors of the converters it uses so its clear to someone updating either nodeDataToMapTree or these more specific functions which tests are responsible for what (ex: if someone generalizes objectToMapTree, they will discover they have to either add a new test suite or somehow test the new aspects via nodeDataToMapTree).

Also in this specific case, note that we expect the set of kinds nodeDataToMapTree has to delegate to to grow for compatibility and feature reasons (ex: if we make a breaking change to object nodes, we might add objectNodes2 or something). Most of simple tree is factored to better accommodate this by factoring code by node kind so legacy node kinds can be kept to the side without getting in the way of other code. As I expect this logic to eventually be removed from toMapTree and instead invoked by dispatching through the schema. When thats done tests for the logic in this function should move to objectNode.spec.ts, which is harder to do if they are not their own suite.

Anyway, thats my reasoning: hopefully you find it informative. While I have a lot of thoughts about this, I don't have a strong opinion: this case doesn't really matter much either way.

[Josmithr]

Yeah, I agree with that generally. And I think it's a dev's responsibility to add unit tests any time they make something module-exported. But there's a balance to be had there, and I (generally) don't think items should be exported just so they can be tested in isolation. If they are intended to be an implementation detail of something else that is exported, that exported item is what should be tested.

If we end up factoring the code differently, such that these specific case functions are module-exported, then they need to get their own unit tests in addition to the tests for nodeDataToMapTree. Some of the existing tests could be moved and modified to use the lower-level entry-points, but we also need to ensure the common root function remains tested, especially since it includes the dispatch logic.

[CraigMacomber]

I think exporting from file for testability is fine: just don't export it from the folder in the index file or give it a release tag. Thiis is a subjective opinion, but its one that has been followed a lot in this package already (at least by me). Perhaps we should see alignment on this topic at a higher level: do we have a place we discuss dev best practices?

This approach of exporting function for testing is a requirement to follow the testing approach I have been using (I try and document the invariants of my functions and test they are met: I want it to be possible to tell if there is a bug in the function without having to consult its callers to see what they expect from it: this pairs well with functions being testable). I often take it a step further was well: lot of the time I factor out a function it is because I want that logic spilt out for more targeted testing. Testability is a large focus for me on how code is factored and it seems expected that many functions will exist as functions and be exported for the sole purpose of improved testability. For a concreate example, in this function (objectToMapTree) I would consider factoring the filed processing body of the loop into its own function so I could unit test all the different field configurations in a much more targeted way. I think that would be a good change, but is something that would be discouraged/banned if we take the approach of not exporting this just for testing.

If a function is entirely just an implementation detail of something else that does not need testing and is only ever intended to be used by that thing, it should be put inside that thing (ex: an inner function) or be documented as part of that thing (ex: "object case for handler for nodeDataToMapTree " instead of "Transforms data under an Object schema."). This helps prevents someone in the future from making assumptions about the implementation that we do not intend to maintain. (I'd prefer we have and document useful properties of functions and test that, but if we want to decide that correctness of a function is defined by is usage, we should make that clear).

[Josmithr]

Perhaps we should see alignment on this topic at a higher level: do we have a place we discuss dev best practices?

Testing council maybe? I think their main focus right now is on improving testing infrastructure, but this topic seems appropriate for them.

[Josmithr]

Moved to its own module.

[alexvy86]

Test changes look ok to me, with one comment below. On the MockNodeKeyManager move, since it's not exported I like the change until we explicitly decide how to expose that kind of functionality if necessary.

[msfluid-bot]

⯆ @fluid-example/bundle-size-tests: -14 Bytes

Metric Name	Baseline Size	Compare Size	Size Diff
aqueduct.js	453.96 KB	453.96 KB	■ No change
azureClient.js	551.21 KB	551.21 KB	■ No change
connectionState.js	680 Bytes	680 Bytes	■ No change
containerRuntime.js	257.82 KB	257.82 KB	■ No change
fluidFramework.js	357 KB	356.99 KB	⯆ -7 Bytes
loader.js	132.91 KB	132.91 KB	■ No change
map.js	41.43 KB	41.43 KB	■ No change
matrix.js	143.66 KB	143.66 KB	■ No change
odspClient.js	519.75 KB	519.75 KB	■ No change
odspDriver.js	97.3 KB	97.3 KB	■ No change
odspPrefetchSnapshot.js	42.16 KB	42.16 KB	■ No change
sharedString.js	160.18 KB	160.18 KB	■ No change
sharedTree.js	356.98 KB	356.97 KB	⯆ -7 Bytes
Total Size	3.19 MB	3.19 MB	⯆ -14 Bytes

---

Baseline commit: 6b67b0e

Generated by 🚫 dangerJS against 6d7aac3

[CraigMacomber]

Test changes look ok to me, with one comment below. On the MockNodeKeyManager move, since it's not exported I like the change until we explicitly decide how to expose that kind of functionality if necessary.

I also think its good for now. Its just a reminder that in the future we will need to come up with better testing stories for apps built on tree since our old plan doesn't work anymore with our much smaller API