test: make tests less fragile

[marcelltoth]

Summary

When publishing #109 I found out that unfortunately the current tests give me approximately zero confidence when changing structure like in that PR. Unfortunately even a single whitespace change breaks the entire suite, not to mention adding a level of nesting (like another div). It makes me have to go through every single snapshot to see if the change is really just as much as intended or more.

We have a pretty solid looking testing strategy at @stoplgihtio/undefined for Elements, see here. The aim of this PR is to rewrite the test suite to use these newer techniques which are supposed to be less fragile, so we can be confident that something like #109 doesn't break anything.

Goals

• I have not changed component code at all.
  There are techniques lot better than these getByText queries I have here, but those would require changes in the component code. (Fixing accessibility problems mainly.) As the goal is to provide a solid baseline for future changes, these were ruled out.
• I have not changed the test cases. I assume the test cases were designed to cover the tricky use-cases in a solid manner. I may have combined some into it/describe.each-es, or changed titles where they were misleading, but the cases should in general be the same.
• Where unsure what the real assertion is, I followed the title of the test case. Another downside of snapshots is that it's not always clear what is being tested (well... everything?), I sometimes had to make assumptions based on the scenario titles.

Results

Everything should still be passing, which is a win!

What's even nicer is that replaying these commits on top of refactor/jsv I get the same green lights (for the rewritten tests at least), proving that the change really makes the test suite robust against even major implementation changes.

Oh, and 1000 LOC lost 💪 (mostly snapshots though).

Questions / to-do

I haven't yet figured for sure what to do with the generic fixture based snapshot tests. I'm leaning towards converting them to .innerText based snapshots, for the time being at least, that feels like the best balance between safety and sturdiness. What do you think?

[P0lip]

Yeah, I added these tests prior to switching to JST.
The goal was to verify the basic output, so that I could easier compare potential differences between JST <-> non-JST based version of JSV.
The outputs meant to be equal or almost equal back then, so it didn't quite matter.
Right now, when you're essentially making a major UI redesign, I'd probably drop existing snapshots, and also revise some schemas we use.

Where unsure what the real assertion is, I followed the title of the test case. Another downside of snapshots is that it's not always clear what is being tested (well... everything?), I sometimes had to make assumptions based on the scenario titles.

so I hope the above explains.
AFAIR I didn't mention that before, but you can probably nuke these output tests that we have.
They were added to help me migrate to JST to highlight any potential significant differences that occur right away when you render the component for the first time, so they didn't quite cover any user interactions such as expanding and so on.

[P0lip]

I'd refrain from such tests tbh.
At the very least we should verify the order they appear in component.
The fact that text is in the document does not imply they're render in the correct position, and with correct nesting.
If I swap the order of properties in JST or do something similar, this test won't catch it.

[P0lip]

As mentioned earlier, this not validate nesting correctly.
You'll certainly want to assert the validation, colors (if we still use different colors for types, don't recall), etc. are all in all in the right state.

[mallachari]

As for the colors we abandoned them for types in new design
https://www.figma.com/proto/IkNc59AU2JN5Kc1qbd2AiB/Elements?node-id=681%3A242&viewport=302%2C436%2C0.980905294418335&scaling=min-zoom

[marcelltoth]

It looks like this textContent snapshot thing does it's job surprisingly well. (See #112)

UPDATE

I see your comments now @P0lip. Let me think about them reply to them tomorrow.:)

[P0lip]

Where unsure what the real assertion is, I followed the title of the test case. Another downside of snapshots is that it's not always clear what is being tested (well... everything?), I sometimes had to make assumptions based on the scenario titles.

To add a bit more context to my previous comment.
It tested:

• nesting
• colors (each type had a different color)
• the expandability of a node (well, just the presence of the expand button)
• an actual content -> mostly title and type

[marcelltoth]

@P0lip I get it. I agree that those things need to be tested. My problem is, I can't figure out any better way to test, a way which also stands the test of time (and a big change in structure).

You said your goal was to ensure safety when extracting JST, now mine is to ensure it when dropping tree-list and virtualization. While testing the structure would be nice I agree, keeping the structural tests then having to rebuild all the snapshots would give me way less confidence than these.

Unfortunately right now - because of virtualization - there isn't really an accessible DOM structure I could test nesting & stuff with.

After merging #109 I plan to update the DOM tree to use nested uls and lis with correct roles and attributes set, like this example. That will allow us to test all that you are asking for here, without snapshotting the DOM. But I really need something for this one step. 🤔 I'm open to ideas.

Again, not to convince you but I think it's not even as bad as it seems like:

• nesting

There are tests explicitly for nesting in place, so there is some protection on that side.

• colors (each type had a different color)

As @mallachari said we eliminated colors already, so no need.

• the expandability of a node (well, just the presence of the expand button)

There are specific tests for this. So while maybe not everywhere but it is already covered.

• an actual content -> mostly title and type

That is being tested with textContent.

So the question is: is this good enough for now? Or can you suggest some alternatives that will still provide value when being rebased on top of the structural changes?

[mpodlasin]

curried assertions <3

[P0lip]

While testing the structure would be nice I agree, keeping the structural tests then having to rebuild all the snapshots would give me way less confidence than these.

I mean, the idea is that when you update the snapshot you do verify the output.
Basically, what I used to do was to go to storybook, see how a particular schema is rendered, and then I would update the snapshot for that given schema.
it's a bit time consuming, but that actually does give you confidence, that at least the initial schema that's get rendered is correct.
Obviously, this won't cover any interactions, like hovering over a tooltip, or expanding (the most error prone bit cause of flattening combined with stepIn), but it's arguably better than verifying just content.

There are tests explicitly for nesting in place, so there is some protection on that side.

Yeah, I get it, but what I'm saying is that these updated tests bring little value in general.
I mean, I'd rather just disable the tests until everything's implemented, and then rewrite them.
Will be probably easier.
So, my suggestion is to disable the existing tests, and keep on going with the UI revamp.

One could also try to use https://www.npmjs.com/package/treeify. We leverage that lib in a number of projects.

[marcelltoth]

Closing in favor of updating snapshots for now, then later developing stronger tests on top of my structural changes. (Which will allow easier testing of nesting & such).