Skip to content

test(frontend): improves react-testing-library configuration#13079

Merged
rusackas merged 1 commit into
apache:masterfrom
michael-s-molina:improve-rtl-config
Feb 11, 2021
Merged

test(frontend): improves react-testing-library configuration#13079
rusackas merged 1 commit into
apache:masterfrom
michael-s-molina:improve-rtl-config

Conversation

@michael-s-molina

@michael-s-molina michael-s-molina commented Feb 11, 2021

Copy link
Copy Markdown
Member

SUMMARY

This PR improves current RTL configuration and contributes to SIP 56.

The changes made here aim to:

Avoid matchers conflict

Enzyme matchers were conflicting with jest-dom matchers. To avoid this I moved import '@testing-library/jest-dom/extend-expect'; from spec/helpers/setup.ts to spec/helpers/testing-library.tsx. This way Enzyme matchers are the default option (99% of the tests) and we can choose to use jest-dom matchers in our RTL tests using custom render functions from testing-library.tsx or importing @testing-library/jest-dom/extend-expect directly in each test file.

Improve user events simulation in our tests

With the use of @testing-library/user-event we have a better level of abstraction to simulate user events. Here are some examples of the new API that can be used in our tests:

click, dblClick, type, upload, clear, selectOptions, deselectOptions, tab, hover, unhover, paste, specialChars

Support additional ESLint rules

With the addition of plugin:jest-dom/recommended to ESLint plugins list we can enforce another set of tests best practices and improve the quality of our tests.

An important side note is that ESLint wasn't working properly for some files because of this configuration problem:

Error while loading rule 'jest/no-deprecated-functions': Unable to detect Jest version - please ensure jest package is installed, or otherwise set version explicitly

To fix this, I included a configuration option to automatically detect Jest version.

Another important point is that ESLint rules of eslint-plugin-testing-library are not currently working with the import of custom functions from a different library name.

In our case the rules will not be applied when using import { render, screen } from 'spec/helpers/testing-library';

If we use import { render, screen } from ‘@testing-library/react’; then ESLint works.

They are aware of this issue and are already working on a fix in version 4.

@ktmud @rusackas @geido @junlincc

TEST PLAN

1 - Execute all tests
2- They should pass

1 - Execute ESLint
2 - No errors should be found

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

- Adds plugin:jest-dom/recommended to ESLint plugins list to enforce tests best practices

- Moves import @testing-library/jest-dom/extend-expect; from setup.ts to testing-library.tsx to avoid matchers collision

- Adds @testing-library/user-event to help simulate users events
@ktmud ktmud changed the title chore: improves RTL configuration test(frontend): improves react-testing-library configuration Feb 11, 2021

@ktmud ktmud left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for the updates and the additional information. This is the gold standard of what PR description should look like! Informative, to-the-point, appropriate links, listing all the caveat and a good testing plan!

@ktmud

ktmud commented Feb 11, 2021

Copy link
Copy Markdown
Member

I always think of "right-to-left" (as in language localization) when I see "RTL" so I changed the PR title a little bit.

Not sure when will Superset support RTL languages, but it'd nice to keep the distinction for now to keep future GitHub/code searches easier.

@rusackas

rusackas commented Feb 11, 2021

Copy link
Copy Markdown
Member

This is the gold standard

So true! This is awesome work, @michael-s-molina!

I always think of "right-to-left" (as in language localization) when I see "RTL"

I actually found an old post-it note on my desk this morning with RTL written on it, and I was confused for the same reason.

@rusackas
rusackas merged commit 86807e4 into apache:master Feb 11, 2021
@junlincc

Copy link
Copy Markdown
Member

This is the gold standard of what PR description should look like!

+1 ! @michael-s-molina

@junlincc

junlincc commented Feb 12, 2021

Copy link
Copy Markdown
Member

😆 by merging this PR we just bumped lockfile npm 7 @ktmud @rusackas
added risk label

@junlincc junlincc added the risk:migration High risk PR, wait for(at least) a week to deploy after merging label Feb 12, 2021
amitmiran137 pushed a commit to nielsen-oss/superset that referenced this pull request Feb 14, 2021
- Adds plugin:jest-dom/recommended to ESLint plugins list to enforce tests best practices

- Moves import @testing-library/jest-dom/extend-expect; from setup.ts to testing-library.tsx to avoid matchers collision

- Adds @testing-library/user-event to help simulate users events
amitmiran137 pushed a commit to nielsen-oss/superset that referenced this pull request Feb 14, 2021
* master: (30 commits)
  refactor(native-filters): decouple params from filter config modal (first phase) (apache#13021)
  fix(native-filters): set currentValue null when empty (apache#13000)
  Custom superset_config.py + secret envs (apache#13096)
  Update http error code from 400 to 403 (apache#13061)
  feat(native-filters): add storybook entry for select filter (apache#13005)
  feat(native-filters): Time native filter (apache#12992)
  Force pod restart on config changes (apache#13056)
  feat(cross-filters): add cross filters (apache#12662)
  fix(explore): Enable selecting an option not included in suggestions (apache#13029)
  Improves RTL configuration (apache#13079)
  Added a note about the ! prefix for breaking changes to CONTRIBUTING.md (apache#13083)
  chore: lock down npm to v6 (apache#13069)
  fix: API tests, make them possible to run independently again (apache#13076)
  fix: add config to disable dataset ownership on the old api (apache#13051)
  add required * indicator to message content/notif method (apache#12931)
  fix: Retroactively add granularity param to charts (apache#12960)
  fix(ci): multiline regex in change detection (apache#13075)
  feat(style): hide dashboard header by url parameter (apache#12918)
  fix(explore): pie chart label bugs (apache#13052)
  fix: Disabled state button transition time (apache#13008)
  ...
@Belco90

Belco90 commented Feb 19, 2021

Copy link
Copy Markdown

Hi all, eslint-plugin-testing-library author here! Sorry for jumping into this out of the blue, but I saw this issue referred in one from our plugin.

Another important point is that ESLint rules of eslint-plugin-testing-library are not currently working with the import of custom functions from a different library name.

In our case the rules will not be applied when using import { render, screen } from 'spec/helpers/testing-library';

If we use import { render, screen } from ‘@testing-library/react’; then ESLint works.

They are aware of this issue and are already working on a fix in version 4.

I've seen you have this problem when re-exporting Testing Library utils from a custom module, which is a pretty common one from plugin users. On this comment from couple of days ago you can find some info about how we addressed this. I'd be really interested into knowing if this would be enough for you, or you think you would need more options to cover this.

From what I saw on your comment I think our approach will cover your use case, but just in case.

Thanks!

@michael-s-molina

Copy link
Copy Markdown
Member Author

Hi all, eslint-plugin-testing-library author here! Sorry for jumping into this out of the blue, but I saw this issue referred in one from our plugin.

Another important point is that ESLint rules of eslint-plugin-testing-library are not currently working with the import of custom functions from a different library name.
In our case the rules will not be applied when using import { render, screen } from 'spec/helpers/testing-library';
If we use import { render, screen } from ‘@testing-library/react’; then ESLint works.
They are aware of this issue and are already working on a fix in version 4.

I've seen you have this problem when re-exporting Testing Library utils from a custom module, which is a pretty common one from plugin users. On this comment from couple of days ago you can find some info about how we addressed this. I'd be really interested into knowing if this would be enough for you, or you think you would need more options to cover this.

From what I saw on your comment I think our approach will cover your use case, but just in case.

Thanks!

@Belco90 Thank you for the update! I was reviewing the thread and it seems that with this comment we'll have everything we need to setup ESLint nicely in our project. Thanks for this great plugin! 🚀

@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 First shipped in 1.2.0 labels Mar 12, 2024
qfcwell pushed a commit to qfcwell/superset that referenced this pull request May 12, 2026
- Adds plugin:jest-dom/recommended to ESLint plugins list to enforce tests best practices

- Moves import @testing-library/jest-dom/extend-expect; from setup.ts to testing-library.tsx to avoid matchers collision

- Adds @testing-library/user-event to help simulate users events
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels risk:migration High risk PR, wait for(at least) a week to deploy after merging 🚢 1.2.0 First shipped in 1.2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants