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', () => ({
  ...jest.requireActual('@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.

[github-actions]

👋 Hey there. This issue hasn't had any activity for 180 days. We'll automatically close it if that trend continues for another week. If you feel this issue is still valid and needs attention please let us know with a comment.

[cee-chen]

👋 Hey hey! Just wanted to drop my 2c in here. I did a quick timeboxed investigation into Jest's snapshot serializers and unfortunately I'm concerned by the complexity/level of effort it would take to write one.

Jest's serialize API is complex and most of their examples involve basic iterables and primitives like objects and arrays. The closest current snapshot plugins I could find that is close to doing what we want (finding an attribute and replacing it) is coincidentally those of CSS-in-JS libraries, that replace randomized CSS classNames with static ones. Examples:

• emotion
• glamor
• styled-components

However, just looking at the amount of code and complexity required to accomplish what they want gives a hint at how much extra work this would be. The issue is that when it comes to serializing JSX/components, what we would have to do is:

• Iterate into each and every React or DOM node, recursively, into the lowest level child
• Decide which attributes we want to inspect. For the CSS in JS plugins, it's easy - they just look at className, and that's it. For us, it's not so simple: Our generated IDs get used in everything from actual id props, to name, to aria-labelledby, to aria-describedby, to being concatenated into other attributes.
• Iterate through all of those, and try and detect our randomized ID and replace it (I'm not actually clear on how we'd do this honestly, we still might need a .testenv mock in order to prefix a regex-able string so that we know when we're actually looking at a generated ID)

Basically, what I'm trying to say is that to write a snapshot serializer for our generated IDs would be close in terms of annoyance to trying to regex html, and certainly very similar to trying to find a needle in a haystack. I'm not convinced this is worth the extra time and effort to do so vs. our current .testenv mock (which is just a few understandable lines of code) and the workaround Greg suggested above. @thomheymann, it sounds like your team may have figured something out since this thread was opened - do you have any updates or thoughts on the LOE vs priority of this work?

Also, I definitely could be missing something or haven't investigated deeply enough, and would love other opinions!

[thomheymann]

@constancecchen Amazing, thanks for looking into this!

We currently have to manually overwrite the EUI mock in every unit test to create a unique id.

I still think EUI should not mock this function at all since snapshot tests use shallow render and other unit tests are better off using the actual implementation.

[cee-chen]

Alright, that's super good to know! It sounds like the extra work would be worth the effort, although it will take a while to get to it.

snapshot tests use shallow render

For Jest/Enzyme this isn't quite true; we take plenty of mounted/rendered snapshot tests in our EUI codebase at least (e.g. DataGrid). Of course, that's internal within the EUI repo, I wonder what the effect would be on the Kibana repo if we removed the htmlIdGenerator testenv mock / how many (if any) snapshots would break 🤔 EDIT: the answer is at least 10+ snapshots 😅

[github-actions]

👋 Hi there - this issue hasn't had any activity in 6 months. If the EUI team has not explicitly expressed that this is something on our roadmap, it's unlikely that we'll pick this issue up. We would sincerely appreciate a PR/community contribution if this is something that matters to you! If not, and there is no further activity on this issue for another 6 months (i.e. it's stale for over a year), the issue will be auto-closed.

[cee-chen]

Commenting to remove the stale check. This is something the EUI team wants to own as part of writing a custom snapshot serializer and reducing snapshot churn in Kibana:

• How do we break less tests when upgrading EUI in Kibana? #6691
• Custom EUI jest snapshot serializer #6676

[drewdaemon]

My 2 cents: at this point RTL (the future of UI testing) is more important than supporting UI snapshots (questionable value).

[cee-chen]

I 100% agree, but the unfortunate tech-debt-laden reality is that Kibana continues to have many snapshots on Enzyme. Us recently removing .defaultProps (in preparation for React 18) alone led to 47 snapshot updates 🥲 Removing the static replacement for randomly generated IDs will cause Kibana snapshots to fail, so we need to figure out a way to support both snapshots and RTL in the interim.

Testing tech debt (converting all EUI enzyme tests to RTL, etc) is my/EUI's next big priority after the Emotion migration and I hope to be able to sit down and resolve issues like this then - but unfortunately that won't be for a few months, unless I can invent a time machine 🤞

[github-actions]

👋 Hi there - this issue hasn't had any activity in 6 months. If the EUI team has not explicitly expressed that this is something on our roadmap, it's unlikely that we'll pick this issue up. We would sincerely appreciate a PR/community contribution if this is something that matters to you! If not, and there is no further activity on this issue for another 6 months (i.e. it's stale for over a year), the issue will be auto-closed.