MNT Use React Testing Library

[emteknetnz]

Issue silverstripe/silverstripe-admin#1419

[GuySartorelli]

The original test explicitly checked for 1 item. We should do the same here, if we only expect one item. I think this is important because there's no other way for us to detect duplicate buttons (behat won't flag it, for example)

[GuySartorelli]

This also applies with many other assertions throughout this PR

[emteknetnz]

Not doing

[GuySartorelli]

This seems more like a mashup of "renders a button" and "renders the title and class" - what it isn't is "renders the wrapped component" since renders the wrapped component would be checking if the div is there.

I don't think it's worth checking for the wrapped component, but if you are gonna have a test for it, you'll need to add a css class or ID to the div and check that the div is rendered.

The button, class, and title checking tests can be mixed together, but the name of the test needs to make it clear that that's what we're testing for - and it should include the length === 1 check.

[GuySartorelli]

This applies in other test files as well (e.g. for publish action)

[emteknetnz]

Not doing

[GuySartorelli]

I think it's important here to check for an actual img element here, since we want to check an image is actually rendered in the DOM. Bonus points for validating the correct src even though the original didn't do that.

[emteknetnz]

Presumably thumbnail-image is an img

[GuySartorelli]

We can be explicit instead of presuming - this is meant to be a test of what the user experiences, isn't it? It feels important to validate that the user is experiencing an image, and not just some random DOM element.

[emteknetnz]

Not doing

[GuySartorelli]

The space at the end there makes me wonder if you had meant to do some more work on this one (either come up with a more descriptive title or split this back out into the individual well-described tests from the original)?
If not, then this is fine.

[emteknetnz]

Not doing

[emteknetnz]

@GuySartorelli Rather than respond to all of the feedback individually, I've responded below to some common types of feedback below. This is applicable to all the other PRs because the same patterns of things will show up many times so I'm highlighting them now before all the other peer review is done.

This means I'll be leaving lots of your individual pieces of feedback unanswered

Using querySelectorAll() + .length .toHaveLenth(n) rather than querySelector() + .not.toBeNull();

RTL philosophy is for tests to resemble how users interact with the application, which is somewhat similar to behat. If you have a look at their recommended priority for querying the rendered dom at the top there are things like getByRole() e.g. getByRole('button', {name: /submit/i}). I mostly haven't implemented these simply because it was easy to translate the tests to use .container() which then allows the use of .querySelector() meaning I could reuse the easy css selectors. Also done this way because when I started the process I was brand new to RTL, and this was the easiest method. If this method was listed in the priority then it would probably be ranked #4 below getByTestId(). With RTL's philosophy in mind, I don't think we should be using 'all()' style queries and then testing the length, it's not in the spirit of RTL. We also don't test '.length()' in behat. Eventually I'd expect that we'll migrate off the .container() approach to a more accessibility first query approach, probably at the same time as we're doing an accessibility first CMS epic.

Missing assertions e.g. original test tested a prop value, or a state value. The new test doesn't test this.

I made a best effort to try and test what the prop or state was rendered to. In all cases this makes the test more complex because you need to look inside the original component and work out how the prop/state is finally rendered. There is simply a very large amount of tests and I think we need to accept there will be some loss of fidelity due to the fundamentally different way of testing. These tests are honestly quite low value (jest tests catch very few regressions) so I think we're better off just accepting the lower fidelity rather than fussing over things that seem basically unimportant.

Test title is non-descriptive

I used the original test title most of the time so that it was easier to match up the before and after in peer review time. I think it would probably be easier to open a new card to bulk update the test titles if we think it warrants it, though personally I'm not sure they're important enough to warrant this.

Things such as "value passed to makeProps() is already in default props", we can could the value of "key", etc

These sorts of changes are so low value IMO I'm not going to spend time fixing them. Normally I wouldn't push back on these and I'd just implement them because it's simply much faster then arguing on github over trivial things, however because there will be hundreds of examples of these in other PRs, I'd much much prefer to have this level of feedback to simply not be provided.

[GuySartorelli]

Using querySelectorAll() + .length .toHaveLenth(n) rather than querySelector() + .not.toBeNull();

The fact is, querySelector() doesn't work the same way that the getBy() functions do. The getBy versions of the functions will throw an error if more than one item is found. But querySelector() does not throw that error, so it does not do the same thing that the old test did, and it also doesn't do the same thing that the getBy() functions will do.

Please either change these tests to use querySelectorAll() and check for a length of 1, or to use the appropriate getBy() functions - or provide a convincing argument why checking for duplicate rendered items isn't worthwhile.

We also don't test '.length()' in behat.

These aren't behat tests - they're a much lower level set of tests checking smaller "units" of functionality for individual components. We have the opportunity to test things here that we wouldn't test in behat and I don't think we should look at end-to-end tests for a measure of how we write our unit tests.

Missing assertions e.g. original test tested a prop value, or a state value. The new test doesn't test this.

Please reply to the individual comments for these - this is far too big a blanket, it's impossible for me to know which things I've flagged are covered by this and which you either didn't notice or forgot, etc.

Test title is non-descriptive

The test titles are important, as they describe what is being tested and therefore give developers a clue as to how the test should behave. If a test starts failing suddenly, the name of the test can be an important clue for what needs to change so that the test passes while still testing what it is meant to test.

In the cases where I have flagged the name of a test, these are either scenarios where the new test does not test the same thing as the old test, or where the test is new, or where the title is duplicated (two tests with the same title).

In all of the scenarios above, I think it is fair for me to expect a title that is descriptive of what the test is doing, since in these cases it's not just a one-to-one match with the old tests.

Things such as "value passed to makeProps() is already in default props", we can could the value of "key", etc

I would like to know if these were done intentionally, and if it's desirable to do it this way, since that will help me know in the future if these are intentional patterns that should be followed or if they were simply left as they are because it's not worth the effort to change. But I won't request any more changes or information about this so that we can avoid some ping pong on them.

[emteknetnz]

Using querySelectorAll() + .length .toHaveLenth(n) rather than querySelector() + .not.toBeNull();

At this point there's a vast number of these assertions to fix (there's 2x other issues containing multiple PRs each). I'd rather programmatically fix everything in one go then have to fix everything individually. If you want this done then please raise a new card to do everything in one go after all the PRs are merged.

Missing assertions e.g. original test tested a prop value, or a state value. The new test doesn't test this.

OK. I'll reply 'Not doing' to the things I'm not doing.

This may seem like I'm just being lazy, though there's over 10,000 lines added so far in all the PRs so I'm genuinely concerned about getting burnt-out here by too many minor changes being asked for in peer review.

Test title is non-descriptive

Again, there's far too many to change at this point. When I was transcribing these tests I don't really know what a lot of the tests were really doing other than usually testing a class method. So I usually just copy paste the old test title and use that. At this point I really don't know what the tests do as I would transcribe a test and then move on to the next. You probably have just as good an idea what these tests do just by looking at the PR.

If you feel these need to be fixed, please raise a new card and it can be done retrospectively, though I doubt this will every get prioritized because it's simply too low value and because it's simply a long and boring task that I doubt anyone will want to do.

Things such as "value passed to makeProps() is already in default props .. I would like to know if these were done intentionally"

Maybe? Maybe at the time I thought it would make the tests easier to read? In all instances though stuff like this is of trivial importance and should be ignored.

[GuySartorelli]

I've raised silverstripe/.github#43 for the outstanding changes. I'll action it.