reactUtils tests: Start testing useHasStayedTrueForMs.

[chrisbobbe]

This is our first use of react-test-renderer. It piggy-backs on
our incorporation of Jest's "modern" fake-timer implementation in
PRs #4754 and #4931. That was handy!

I haven't yet found any test cases that fail with our
implementation. (And I'd been hoping to, to debug an unexpected
error!)

But I did try pasting in an earlier iteration of the hook's
implementation, from #4940, that Greg had found bugs in by reading
the code. Many of these tests failed on that buggy implementation,
which is a good sign.

Might as well keep these new tests, then, if they're not an
unreasonable maintenance burden.

[chrisbobbe]

The current revision uses a very simple test-only function component.

It looks like we could use a third-party library, @testing-library/react-hooks (https://github.com/testing-library/react-hooks-testing-library), to avoid doing that, and just test the hook by itself.

I'm inclined to keep the current approach because the function component is easy to define, for our needs here. And react-test-renderer is maintained by React (it has a doc on the React website at https://reactjs.org/docs/test-renderer.html).

Edit: Although, @testing-library/react-hooks gets a small shoutout at https://reactjs.org/docs/testing.html#tools:

React Testing Library is a set of helpers that let you test React components without relying on their implementation details. This approach makes refactoring a breeze and also nudges you towards best practices for accessibility. Although it doesn’t provide a way to “shallowly” render a component without its children, a test runner like Jest lets you do this by mocking.

[gnprice]

Great! Thanks for exploring this.

These tests look good. Small comments below, then please merge at will.

[gnprice]

nit:

Suggested change

	const currentSequence = sequencesToTest[i];
	test(JSON.stringify(currentSequence, null, 2), async () => {
	await testSequence(sequencesToTest[i]);
	const currentSequence = sequencesToTest[i];
	test(JSON.stringify(currentSequence, null, 2), async () => {
	await testSequence(currentSequence);

Or is there something subtle here I'm missing?

[chrisbobbe]

Nope, you're right! Thanks!

[gnprice]

These test "names" get pretty long in the Jest output. Let's add something like a description property to each of these test-case objects, and use that here.

Another benefit of that is that it'd provide a good place for a few words explaining the scenario each of these is meant to cover.

For example:

    {
      description: "start false, wait short time",
      initialValue: true,
      sequence: [{ waitBefore: MS / 2, expectedOutput: false }],
    },
    {
      description: "start false, wait long time",
      initialValue: true,
      sequence: [{ waitBefore: MS * 2, expectedOutput: true }],
    },

[gnprice]

nit:

Suggested change

	sequence: [
	{ waitBefore: MS / 2, expectedOutput: false, thenUpdateInputTo: true },
	{ waitBefore: MS * 2, expectedOutput: true, thenUpdateInputTo: false },
	{ waitBefore: MS / 2, expectedOutput: false },
	sequence: [
	{ waitBefore: MS / 2, expectedOutput: false, thenUpdateInputTo: true },
	{ waitBefore: 2 * MS, expectedOutput: true, thenUpdateInputTo: false },
	{ waitBefore: MS / 2, expectedOutput: false },

and similarly the other MS * 2 instances.

That makes the "twice" and "half" more visually distinct, making it a bit easier to scan each of these scenarios and see what it's saying. Perhaps not coincidentally, it's also more conventional in math and physics: numeric coefficient first, then the variable or dimensional quantity it's multiplying goes after that.

[gnprice]

Yeah. Here's the error if I remove the act:

  console.error
    Warning: An update to _TestComponent inside a test was not wrapped in act(...).
    
    When testing, code that causes React state updates should be wrapped into act(...):
    
    act(() => {
      /* fire events that update state */
    });
    /* assert on the output */
    
    This ensures that you're testing the behavior the user would see in the browser. Learn more at https://reactjs.org/link/wrap-tests-with-act
        at _TestComponent (/home/greg/z/mobile/src/__tests__/reactUtils-test.js:54:71)

      70 |   useEffect(() => {
      71 |     if (value) {
    > 72 |       const id = setTimeout(() => setResult(true), duration);

I think the point is that fakeSleep causes that timer to run -- that's why we're calling it, after all -- which means we set the state. That causes an update to the test component, so we need it wrapped in act to make sure the updates get applied.

[chrisbobbe]

Yeah. It occurs to me after re-reading that error you quoted: I think they did a pretty good job raising this error and choosing its error message. It happens early, it's specific, and it's actionable.

[chrisbobbe]

Great! Thanks for exploring this.

These tests look good. Small comments below, then please merge at will.

Thanks for the review! Merged, with those tweaks.