Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generate stable IDs #664

Closed
matsprea opened this issue May 20, 2020 · 5 comments · Fixed by #1408
Closed

Generate stable IDs #664

matsprea opened this issue May 20, 2020 · 5 comments · Fixed by #1408
Assignees
Labels
developer-experience documentation Effects documentation enhancement New feature or request

Comments

@matsprea
Copy link

matsprea commented May 20, 2020

🚀 Feature Proposal

I'd like to have stable generated IDs instead of completely different at each execution.

Motivation

As a developer I want to control these IDs and be able to use snapshots in automation to check their values. At the moment I have to mock the IDs because the ones generated by uuid are changing at every execution.

@matsprea matsprea changed the title Stable IDs Generate stable IDs by 'uuid' May 20, 2020
@matsprea matsprea changed the title Generate stable IDs by 'uuid' Generate stable IDs May 20, 2020
@lychyi
Copy link
Contributor

lychyi commented May 22, 2020

@matsprea Thanks for the issue.

Can you add a bit more context or a code block? I'm curious if mocking uuid causing another downstream issue for you? In the past, that's how we recommend that people do this for stable snapshots.

Also, which components are you specifically talking about? Some components have a manual id override like FormField. Others are ids generated as part of the component itself and they're not overridable by design.

We're not closed to the idea entirely but some more context about the pain point would help us a ton!

@matsprea
Copy link
Author

My specific issue is with the popup component that is generating an id for the aria-labelledby to reference the tittle of the popup and every time we run the automation, the id changes.

`NotificationsPopup > › NotificationsPopup > › snapshot with open set to true

expect(received).toMatchSnapshot()

Snapshot name: `NotificationsPopup >  NotificationsPopup > snapshot with open set to true 1`

- Snapshot
+ Received

@@ -536,11 +536,11 @@
  
  <div
    class="emotion-20"
  >
    <div
-     aria-labelledby="0f7757d3-5286-459e-9c98-28de998ca51e"
+     aria-labelledby="58ae28cd-88d6-4158-996a-038fa8be04c7"
      aria-modal="true"
      class="emotion-19"
      role="dialog"
      tabindex="-1"
    >
@@ -580,11 +580,11 @@
      <div
        class="emotion-18"
      >
        <h3
          class="emotion-3"
-         id="0f7757d3-5286-459e-9c98-28de998ca51e"
+         id="58ae28cd-88d6-4158-996a-038fa8be04c7"
        >
          Mentions (1)
        </h3>
        <div
          class="emotion-17"

  52 | export const takeSnapshot = (component, state) => {
  53 |   const { container } = getRenderedResult(component, state);
> 54 |   expect(getTarget(container)).toMatchSnapshot();
     |                                ^
  55 | };
  56 | 
  57 | export const takeTableElementSnapshot = (component, state) => {

  at toMatchSnapshot (src/__tests__/testUtils.jsx:54:32)
  at Object.<anonymous> (src/components/common/header/notifications/popup/__tests__/NotificationsPopup.test.jsx:20:7)`

As a workaround we are mocking the Id in the test

jest.mock('uuid/v4', () => jest.fn(() => 'theIdToTestWith'));

but we would like to have it generated in a predictable/stable way or control how it's generated.

@NicholasBoll NicholasBoll added developer-experience documentation Effects documentation enhancement New feature or request labels Jun 4, 2020
@NicholasBoll
Copy link
Member

Thank you for creating this issue! I'd like to add a few notes to guide how we can improve this.

  1. Not all IDs can be overridden through props since there are cases where ids are used internally by components for ARIA attributes
  2. Our use of uuid often creates invalid IDREFs since uuid can produce values that start with a number
  3. uuid produces very long IDs that take extra bytes and are unnecessary

My suggestion is we create a utility function that will go into @workday/canvas-kit-react-common that produces valid IDREFs for use in all our components. We should have a mechanism for stable IDREF production for testing purposes (likely a prefix with an incrementing suffix similar to stable Emotion CSS class names). Generating the same string for all IDREFs is not an option as this may break valid queries like those use in React Testing Library. The utility function should have another function that should be used in global test configuration files to reset this increment for use in a beforeEach of said global test configuration. This should be documented within the common package so that we can point to it when needed.

@NicholasBoll NicholasBoll added this to Open in Backlog via automation Jun 4, 2020
@matsprea
Copy link
Author

matsprea commented Jun 4, 2020

Thanks @NicholasBoll , that will be awesome!

@jpante jpante added the p:1 label Jul 20, 2020
@project-bot project-bot bot moved this from Open to High in Backlog Jul 20, 2020
@jpante jpante moved this from High to Up Next in Backlog Jul 21, 2020
@lychyi lychyi added this to To-do (This Sprint) in Current Sprint (7/20 - 8/9) via automation Aug 24, 2020
@lychyi lychyi removed this from Up Next in Backlog Aug 24, 2020
@mannycarrera4 mannycarrera4 self-assigned this Aug 26, 2020
@lychyi lychyi removed this from To-do (This Sprint) in Current Sprint (7/20 - 8/9) Oct 12, 2020
@NicholasBoll NicholasBoll added this to Open in Backlog via automation Oct 5, 2021
@myvuuu myvuuu moved this from Open to High in Backlog Nov 18, 2021
NicholasBoll added a commit to NicholasBoll/canvas-kit that referenced this issue Jan 5, 2022
@NicholasBoll NicholasBoll linked a pull request Jan 5, 2022 that will close this issue
6 tasks
workday-canvas-kit pushed a commit that referenced this issue Jan 7, 2022
Our current id generation has issues.

- It generates very long ids that add noise to the page
- It is possible to generate a blank id
- Id generation is slow
- Snapshot tests require mocking the `uuid` module

Fixes: #1311, #1106, #664, #602

[category:Hooks]

Release Note:
**NOTE for jest snapshots**: This change removes the `uuid` package and instead will generate a one-time client seed and then create auto-incrementing ids. This change will not break UI or automated UI tests. It will break snapshot tests however. Previously, the only way to get stable ids for snapshot tests was to mock the `uuid` module. This was an implementation detail. To make snapshots work again, add the following to your jest setup file:

```ts
import {setUniqueSeed, resetUniqueIdCount} from '@workday/canvas-kit-react/common';

beforeEach(() => {
  setUniqueSeed('a'); // force set the seed
  resetUniqueIdCount(); // reset the unique id count
});
```

This will ensure each Jest snapshot has ids that look like `a0` and `a1` and will be the same every time the snapshot is run. Do not use these methods in production though - it may lead to inaccessible applications due to IDREF collisions.
@NicholasBoll
Copy link
Member

Closed by #1408

Backlog automation moved this from High to Done Jan 10, 2022
@NicholasBoll NicholasBoll removed this from Done in Backlog Jan 10, 2022
@NicholasBoll NicholasBoll added this to To-do (This Sprint) in Current Sprint (7/20 - 8/9) via automation Jan 10, 2022
@NicholasBoll NicholasBoll moved this from To-do (This Sprint) to Done in Current Sprint (7/20 - 8/9) Jan 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
developer-experience documentation Effects documentation enhancement New feature or request
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

5 participants