You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2021/02/11 17:50:41 UTC
[GitHub] [superset] michael-s-molina opened a new pull request #13079: chore: improves RTL configuration
michael-s-molina opened a new pull request #13079:
URL: https://github.com/apache/superset/pull/13079
### SUMMARY
This PR improves [current](https://github.com/apache/superset/pull/11771) RTL configuration and contributes to [SIP 56](https://github.com/apache/superset/issues/11688).
The changes made here has 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](https://github.com/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](https://github.com/testing-library/eslint-plugin-jest-dom) 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](https://github.com/testing-library/eslint-plugin-testing-library/issues/173) 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.
- [x] Introduces new feature or API
- [ ] Removes existing feature or API
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] Belco90 commented on pull request #13079: test(frontend): improves react-testing-library configuration
Posted by GitBox <gi...@apache.org>.
Belco90 commented on pull request #13079:
URL: https://github.com/apache/superset/pull/13079#issuecomment-781954786
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](https://github.com/testing-library/eslint-plugin-testing-library/issues/173#issuecomment-780519874) 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!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] junlincc edited a comment on pull request #13079: test(frontend): improves react-testing-library configuration
Posted by GitBox <gi...@apache.org>.
junlincc edited a comment on pull request #13079:
URL: https://github.com/apache/superset/pull/13079#issuecomment-778250194
๐ by merging this PR we just bumped lockfile npm 7 @ktmud @rusackas
added `risk` label
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] rusackas edited a comment on pull request #13079: test(frontend): improves react-testing-library configuration
Posted by GitBox <gi...@apache.org>.
rusackas edited a comment on pull request #13079:
URL: https://github.com/apache/superset/pull/13079#issuecomment-777862860
> 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on pull request #13079: test(frontend): improves react-testing-library configuration
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on pull request #13079:
URL: https://github.com/apache/superset/pull/13079#issuecomment-781989152
> 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](https://github.com/testing-library/eslint-plugin-testing-library/issues/173#issuecomment-780519874) 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](https://github.com/testing-library/eslint-plugin-testing-library/issues/173#issuecomment-780524507) we'll have everything we need to setup ESLint nicely in our project. Thanks for this great plugin! ๐
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on pull request #13079: test(frontend): improves react-testing-library configuration
Posted by GitBox <gi...@apache.org>.
ktmud commented on pull request #13079:
URL: https://github.com/apache/superset/pull/13079#issuecomment-777726239
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's nice to keep the distinction for now to keep future GitHub/code searches easier.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] junlincc commented on pull request #13079: test(frontend): improves react-testing-library configuration
Posted by GitBox <gi...@apache.org>.
junlincc commented on pull request #13079:
URL: https://github.com/apache/superset/pull/13079#issuecomment-777866858
> This is the gold standard of what PR description should look like!
+1 ! @michael-s-molina
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] rusackas commented on pull request #13079: test(frontend): improves react-testing-library configuration
Posted by GitBox <gi...@apache.org>.
rusackas commented on pull request #13079:
URL: https://github.com/apache/superset/pull/13079#issuecomment-777862860
> This is the gold standard
So true! This is awesome work, @michael-s-molina!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud edited a comment on pull request #13079: test(frontend): improves react-testing-library configuration
Posted by GitBox <gi...@apache.org>.
ktmud edited a comment on pull request #13079:
URL: https://github.com/apache/superset/pull/13079#issuecomment-777726239
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] junlincc edited a comment on pull request #13079: test(frontend): improves react-testing-library configuration
Posted by GitBox <gi...@apache.org>.
junlincc edited a comment on pull request #13079:
URL: https://github.com/apache/superset/pull/13079#issuecomment-778250194
๐ by merging this PR we just bumped lockfile npm 7 @ktmud @rusackas
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] rusackas merged pull request #13079: test(frontend): improves react-testing-library configuration
Posted by GitBox <gi...@apache.org>.
rusackas merged pull request #13079:
URL: https://github.com/apache/superset/pull/13079
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] junlincc commented on pull request #13079: test(frontend): improves react-testing-library configuration
Posted by GitBox <gi...@apache.org>.
junlincc commented on pull request #13079:
URL: https://github.com/apache/superset/pull/13079#issuecomment-778250194
๐ by merging this PR we just bumped log file npm 7 @ktmud @rusackas
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org