[makeId] Makes it difficult for consumers to create Jest tests

[cjcenizal]

(edit: see this comment below for how to solve this for makeId and htmlIdGenerator)

Any component which consumes makeId or a similar dynamic ID-generator will create non-deterministic IDs every time it's instantiated (e.g. EuiDescribedFormGroup, EuiFormRow, EuiToolTip). This is problematic for consumers who want to create jest tests for any UI which is built with these components. EUI solves this problem by mocking makeId in its own unit tests, but this isn't an option for consumers.

Maybe we could we use an environment variable to make makeId return a stub value?

[chandlerprall]

If it's a global solution, such as an environment variable, I think it'd make sense to have the random generators return a hard-coded answer so test execution order doesn't affect it. However, this breaks the idea that an id is unique, though I find it unlikely there's a valid reason for a test to care about any of these random ids.

The other way I'd go is expose a mocking function through the module itself and then the consumer can decide if they want to mock at a global level (Jest setup), per suite, or even per test.

The global mock via flag sounds easiest and nicest for the consumer, as long as having duplicate generated ids is fine. Thoughts?

[cjcenizal]

The only situation in which I can imagine the id mattering to a test would be one in which we're mounting the component and doing some heavy interaction with it, e.g. clicking on a label in order to give focus to an input. But this seems pretty far-fetched since it's arguably beyond the realm of unit testing.

The global mock via flag sounds easiest and nicest for the consumer, as long as having duplicate generated ids is fine.

Let's give it a whirl! My only suggestion would be to name the flag something super specific and scoped to EUI, e.g. euiMockedGeneratedIds.

[sebelga]

I managed to get a deterministic ID by simply adding this line add the top of the test file

jest.mock('@elastic/eui/lib/components/form/form_row/make_id', () => () => 'my-id');

I think it's a good enough solution and I don't think our unit tests need the aria ids for anything.

The only thing that is strange to me is why the makeId() utility function is under form, when it is used by many other components.

[chandlerprall]

While it is certainly possible for Kibana tests to mock the implementation, I don't think EUI should rely on the functionality, nor force consuming applications into considering EUI's internals when writing tests. However, it does make addressing this lower priority - and thank you for providing the code necessary to work around the issue!

[cjcenizal]

If we implement the global mock flag, then we could also update components which use portals (e.g. EuiOverlayMask, EuiPortal) to conditionally render their children when the flag is set to true, instead of creating a portal. This would make it easier for unit tests to find modals and things. This would expand the responsibility/meaning of the global mock flag, so maybe naming it something like "EUI test mode" would make more sense.

  render() {
    if (IS_EUI_TEST_MODE) {
      return this.props.children;
    }

    return createPortal(
      this.props.children,
      this.portalNode
    );
  }

[chandlerprall]

FWIW, latest versions of enzyme support portals (and Kibana does as of last week); we should be upgrading Enzyme in EUI soon(ish).

[cjcenizal]

@chandlerprall Great point! Thanks, I take back my comment. :)

[BentoumiTech]

It seems that now we need to mock htmlIdGenerator since #3129

jest.mock('@elastic/eui/lib/services/accessibility/html_id_generator', () => ({
  htmlIdGenerator: () => () => 'htmlId',
}));

It's also mocked inside the codebase for snapshot testing

eui/src/components/popover/popover.test.tsx

Line 38 in 42c7ced

jest.mock('../../services/accessibility/html_id_generator', () => ({

[chandlerprall]

We now have a test-env build (docs) which we should create an auto-mock for the htmlIdGenerator. Interestingly, I think this can be done without affecting any applications' existing jest mocks for htmlIdGenerator as they'd still be targeting the same file(s).

[chandlerprall]

Extending the above thought on including a testenv version of htmlIdGenerator: we could hash the stack trace - maybe removing the line numbers - to generate a consistent, yet unique, ID.