Eui: EuiFormRow does not work with react-testing-library

[thomheymann]

EuiFormRow automatically generates an id to link together field label with field input.

This works fine in production and dev. However, in a test environment this functionality is disabled and instead a static string is returned for every field: https://github.com/elastic/eui/blob/master/src/services/accessibility/html_id_generator.testenv.ts

This means each input element in a form with multiple fields now has the same id during testing which is invalid. It also breaks expected behaviour since clicking any field label will now focus the same input element (whichever is the first one in the form) instead of the actual input element wrapped by the EuiFormRow.

This behaviour stops us from using react-testing-library which approach is to force developers to query elements by role or label text instead of DOM elements to ensure accessibility requirements are met:

await findByLabelText('Username'); // Will throw since 'Username' label resolves to 'generated-id' which matches multiple inputs

What is the purpose of the static generated-id ids during testing?

Is there a way to disable this behaviour?

My only option right now seems to be to manually set each field's id, which defeats the purpose of having automatic id generation in the first place, and runs the danger of clashing with other ids on the page.

It would be great if this could be opt-in via a mock so that devs who require static ids (e.g. for snapshot testing) can easily opt into this behaviour without breaking other tests.

[thompsongl]

Hey @thomheymann,

The purpose of mocking html-id-generator to return static values is for Jest snapshot testing. Without it, snapshots become outdated on every run as the ids change. It is a fairly recent change for EUI & Kibana to apply this mock globally, which we did to prevent the need for manually mocking every snapshot test file in Kibana that uses the id generator. We saw the benefit for Jest snapshot tests as greater than the hindrance for less used react-testing-library tests.

You can re-mock html-id-generator and provide some other unique id generation function, like is done in a few spots in Kibana already:

jest.mock('@elastic/eui/lib/services/accessibility/html_id_generator', () => ({
  htmlIdGenerator: () => () => `id-${Math.random()}`,
}));

[thomheymann]

Thanks for explanation and fix Greg!

I'm happy to use this for the time being but am a bit worried about having two different approaches in Kibana now (auto mocking and manual mocking). We're not using auto-mocking elsewhere as far as I'm aware.

It took me quite a while to debug this issue and I'm worried about other dev's potentially wasting time on this as well.

Jest has the ability to define static mocks. We could provide one for the htmlIdGenerator so that developers could mock it without having to implement it themselves:

jest.mock('@elastic/eui/lib/services/accessibility/html_id_generator');

Alternatively we could create a custom jest serialiser that replaces id attributes that have been generated using htmlIdGenerator with a static string so that snapshots work without having to manually mock the function without breaking other tests:
https://jestjs.io/docs/en/configuration#snapshotserializers-arraystring

[chandlerprall]

TIL about snapshotSerializers, that's awesome and does seem a better fit for why this module was mocked. It does force the solution to be jest specific, but this mock exists only for jest snapshots anyway.

Branching a bit, is there something we could do for our other mocked modules (which are done for non-snapshot reasons) that would make it obvious they differ from the non-test implementation? Perhaps a data-is-mocked prop or similar?

[thompsongl]

+1 to snapshotSerializers; I was also unaware that existed

[thompsongl]

Rereading this and responding to the more fundamental point

We're not using auto-mocking elsewhere as far as I'm aware.

EUI has a separate build that was created specifically for the Kibana Jest testing environment, and is consumed via nodeModuleMapper. This is definitely a unique case of trying to hide some implementation details from Kibana tests, and we haven't yet clearly defined the detail line.

It took me quite a while to debug this issue.

Yep, we have some docs in EUI (linked above), but docs in Kibana are lacking. We can do better there and the point about "make it obvious they differ from the non-test implementation" is a good idea.

[thomheymann]

Awesome, happy to go with the serializer approach!

To be honest I'm not sure a data-is-mocked attribute would have helped in this case. The id 'generated-id' was pretty clearly named once I found it and made the connection that that's going to break the labels and testing library but when writing tests using jsdom it's a bit of a blackbox so the less "interference" the better I think.

When developing in Kibana there are a lot of services and contract that have to be mocked for testing irregardless so I don't think it's too much of an ask to have to mock time / random based functions, especially if we make it easy by providing mock implementations.

[thomheymann]

I might be missing some context but one other issue I thought about when discussing this with my team is that we might be trying to solve a problem here that doesn't exists.

When writing snapshot tests we should be shallow rendering the component in which case the id wouldn't even get generated.

If we mount the entire component tree then we're not talking about unit tests anymore but "component integration" tests (for the lack of a better word) in which case snapshots are not a good fit (they would grow out of hand and change with every update to a dependent component). The use case for mounting components are interaction based tests for which we'd use jsdom (enzyme or react testing library).

[thompsongl]

I think you're correct on all those points. Writing tests in Kibana, especially as they relate to EUI components/internals, has been something of an ongoing discussion and one that we plan on continuing this year. There are some good ideas in this thread to bring into the fold.