Test API usability improvements

[robertknight]

This issue tracks a few ideas for improving the APIs we use for writing tests. This means reducing the amount of code needed for common uses, eliminating pitfalls etc.

This issue is a place to brainstorm. Separate issues can be created to flesh ideas out more fully. Some of these issues are also really about the underlying libraries, so the issues need to be created downstream.

waitFor method on wrappers

A common action is to trigger an async operation in a test, then wait for the UI to change in some way.
This is currently handled via a waitForElement function, but it would be useful if it was an operation on the wrapper.

Current:

import { mount, waitForElement } from '@hypothesis/frontend-testing';
...
const wrapper = mount(<Widget/>);
const items = waitForElement(wrapper, 'ul[data-testid="item-list"]');

Proposed:

import { mount } from '@hypothesis/frontend-testing';
...
const wrapper = mount(<Widget/>);
const items = await wrapper.waitFor('ul[data-testid="item-list"]');

Clarify or simplify use of act and wrapper.update()

It is often not intuitive when act and wrapper.update() are required. It would make writing tests easier if there was at least good documentation, or perhaps better yet changes that could be made to reduce how often this is needed.

• The act function runs a callback and then repeatedly flushes effects and state updates, until a flush triggers no further updates
• wrapper.update() synchronizes the Enzyme wrapper's snapshot of the rendered output to match the current output

A common pattern of usage is:

act(() => {
  wrapper.find('SomeChild').prop('onClick')();
});
wrapper.update();

Here the act callback ensures that state updates and effects triggered by onClick are flushed, then wrapper.update ensures that subsequent queries on wrapper reflect the current UI.

Often it is possible to omit the use of act if the onClick handler triggers state updates but no effects. This is because the wrapper.update() call will implicitly flush any pending state updates. Internally wrapper.update() calls the Enzyme adapater's getNode method and the MountRenderer.getNode implementation in enzyme-adapter-preact-pure calls flushRenders first.

Inspecting updates to child components

Another common sequence looks like:

// Get child and trigger an update
let child = wrapper.find('SomeWidget');
assert.equal(child.prop('value'), 'Original value');
act(() => {
  child.prop('onChange')('Updated value');
}); // nb. The `act` can often be omitted. See above.

// Refresh the parent component and re-find the child.
wrapper.update();
child = wrapper.find('SomeWidget');
assert.equal(child.prop('value'), 'Updated value');

Re-finding the child is necessary because the original child value was a snapshot from the initial render that doesn't change after refreshing the root wrapper. It would be more ergonomic if the above could be written as:

const child = wrapper.find('SomeWidget');
assert.equal(child.prop('value'), 'Original value');
child.prop('onChange')('Updated value');
assert.equal(child.prop('value'), 'Updated value');

Built-in data-testid support

We extensively use the pattern of adding a data-testid attribute to DOM elements and components, then use an attribute selector to find those in tests. This pattern is taken from Testing Library. eg.

wrapper.find('button[data-testid="foo-button"]')

These selectors are a bit awkward to write, especially if foo-button is partly dynamic. It would be useful to have some built-in support for them. A complication is that the data-testid value is often not enough on its own, because that attribute might be passed down multiple elements. eg. <Button data-testid="foo">...</Button> will render both a Button and button with data-testid props.

[acelaya]

Agree with all these points, and I have a suggestion: we could consider giving @testing-library a go, as it covers all these pain points (one way or another).

One fundamental difference between enzyme and testing-library is that the latter works with actual DOM nodes. In fact I think it would shine with our approach of testing in an actual browser, as my experience has been mainly using JSDOM.

Let me explain how it would address the cases above:

• waitFor method on wrappers:

  Testing-library does not return a wrapper you use to query for elements. Instead it renders the component in the DOM, and then provides a "global" screen utility you can use to query for DOM elements. By role, by text, by type, etc.

  For asynchronous operations it still provides its own waitFor utility, but since there's no wrapper anymore, it fits better with the overall recommended approach.

  In any case, the screen object provides three ways to query for things: screen.getBy..., screen.queryBy... and screen.findBy....

  The first one throws if the element is not found in the DOM, the second one returns an element or undefined, so it's useful to query for elements that may or may not be there. The last one returns a promise that resolves if the element is found, and rejects if not, and is frequently a valid replacement for waitFor (see https://testing-library.com/docs/dom-testing-library/api-async#findby-queries).

• Clarify or simplify use of act and wrapper.update().

  This is simply not needed anymore as testing-library uses act internally when needed.

  I have faced sometimes errors stating a call for act was missing, but this article explains how most of those should be addressed.

• Inspecting updates to child components

  Again, since we no longer have a wrapper but always query the actual DOM, there's no longer shallow references to take into consideration.

• Built-in data-testid support

  This is supported out of the box. You can do screen.getByTestId('foo-button') and it will look for the element with the data-testid attribute with provided value.

  Since an actual DOM is rendered, there's also no ambiguity, and there will only be one element with that data-testid.

Other benefits

On top of that, testing library provides other benefits:

1. A nice way to handle events https://testing-library.com/docs/user-event/intro/, which exposes an asynchronous API that usually behaves very intuitively (if you do await user.click(screen.getByRole('button', { name: 'Open menu' })), once that resolves the menu is in the DOM and you can query for it).
2. Feels closer to just interacting with the DOM, and in fact you can do it in most cases.
3. It's widely adopted and well maintained. Enzyme has worked well so far, but does not receive a lot of attention anymore.
4. Manually unmounting is no longer needed. The DOM is cleared after every test.

Drawbacks

The main drawback is that testing-library is not a drop-in replacement for enzyme at all.

In my experience migrating projects, you have to basically rewrite everything in many cases, and very frequently, you have to re-think the test because things are simply done differently.

A good thing though is that both can coexist, so we could write new tests with testing-library, while the old ones are slowly migrated when capacity allows it. Same as we did with the TypeScript migration.

It is also perfectly possible to continue using karma, sinon and mocha with testing-library, although I'm not sure if some assertions would be a bit less "semantic".

When used with jest or vitest, they provide integrations to do things like expect(screen.getByTestId(...)).toHaveClass() or .toHaveAttribute(), .toBeInTheDOM(), .toBeDisabled(), etc. We would have to write those ourselves.

[robertknight]

Again, since we no longer have a wrapper but always query the actual DOM, there's no longer shallow references to take into consideration.
Since an actual DOM is rendered

To clarify, our use of Enzyme always renders actual DOM. The difference compared to Testing Library is that you get access to the component tree as well as the DOM tree. I feel this is important to highlight because a lot of the worst pain points other projects had with Enzyme were due to use of shallow rendering, which inevitably coupled the test to implementation details of the component. To clarify what I mean, consider a Widget.js module:

function InternalHelper({ children }) {
  return <div>{children}</div>;
}

export default function Widget() {
  return (
    <InternalHelper>
      <button>foo</button>
    </InternalHelper>
  );
}

With both our use of Enzyme and Testing Library, you can remove InternalHelper without affecting the tests (usually). With shallow rendering which Enzyme and React used to push people towards, that's not the case.

The main drawback is that testing-library is not a drop-in replacement for enzyme at all.

In my experience migrating projects, you have to basically rewrite everything in many cases, and very frequently, you have to re-think the test because things are simply done differently.

Right, that's because we're effectively talking about different points on the unit test <-> integration test spectrum. For leaf components, this doesn't make a huge difference. For components higher up the tree however, our current tests can query the rendered output in terms of child components and props if they want to. A Testing Library test always queries the final DOM output.

Take Via's video player app for example. The tests for the VideoPlayerApp component check that the Transcript component was rendered and passed the expected transcript value (as a JS object), but are not coupled to the details of how the Transcript renders that value (eg. does it use a ul with list items or a table with rows). With Testing Library, you'd either render the real Transcript component and have the VideoPlayerApp be coupled to those details, or render a mock of some kind with a known DOM structure.

To give another example from the same test, VideoPlayerApp renders a HypothesisClient component which wraps a <script> that loads the client. The VideoPlayerApp tests can easily verify that this component was passed the correct configuration as a JSON object.

For me, I think this is the most substantial issue to consider.

This is simply not needed anymore as testing-library uses act internally when needed.

Testing Library is in the same situation as Enzyme here. If you make a change via the test library's API call (fireEvent/render in TL, simulate/mount in Enzyme) then it will call act for you. If you make a change outside of TL, then you might still need to call act. The difference is that you don't have the option of invoking a callback prop directly, so a test in our code where you write act(() => someChild.prop('onCallback')()) would have to be written differently.

It's widely adopted and well maintained. Enzyme has worked well so far, but does not receive a lot of attention anymore.

On this point, I agree. Some other things Testing Library has going for it:

• It has a small API surface area which makes it easy to learn
• The documentation is good
• It has built-in support for async
• Its element selection APIs encourage developers to ensure there are appropriate ARIA roles and labels

[acelaya]

This is simply not needed anymore as testing-library uses act internally when needed.

Testing Library is in the same situation as Enzyme here. If you make a change via the test library's API call (fireEvent/render in TL, simulate/mount in Enzyme) then it will call act for you. If you make a change outside of TL, then you might still need to call act. The difference is that you don't have the option of invoking a callback prop directly, so a test in our code where you write act(() => someChild.prop('onCallback')()) would have to be written differently.

Yeah, I was probably too simplistic on that statement. You are definitely right here.