[tests] Migrate TraceIDSearchInput.test from Enzyme to RTL #1691
Conversation
Signed-off-by: Ansh Goyal <[email protected]>
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## main #1691 +/- ##
=======================================
Coverage 96.01% 96.01%
=======================================
Files 241 241
Lines 7560 7560
Branches 1985 1985
=======================================
Hits 7259 7259
Misses 301 301
☔ View full report in Codecov by Sentry. |
| @@ -36,7 +36,12 @@ class TraceIDSearchInput extends React.PureComponent<Props> { | |||
|
|
|||
| render() { | |||
| return ( | |||
| <Form layout="horizontal" onSubmit={this.goToTrace} className="TraceIDSearchInput--form"> | |||
| <Form | |||
| aria-label="form" | |||
There was a problem hiding this comment.
Form as HTML element is generally invisible to user, how does accessibility label help? I think it would make more sense on the <input> with label "trace ID"
There was a problem hiding this comment.
Agreed. Though, I am treating aria-label as a selector inside the rendered dom, we can have it on <input> too.
Should I go ahead adding another expect statement, to the input field as well? The more tests, the better testing regime would it be.
There was a problem hiding this comment.
I don't think testing needs is a good reason to add (permanent) attributes to HTML if they don't improve the HTML itself. I would suggest finding other ways to locate elements for tests.
There was a problem hiding this comment.
Also, because form itself is invisible for the user, if some test is written that requires access to the form then the test probably does not pass the principles of testing established by RTL, "test from user perspective".
There was a problem hiding this comment.
We can use data-test-id as a replacement for that. Going ahead with that approach.
There was a problem hiding this comment.
does not pass the principles of testing established by RTL, "test from a user perspective".
Yes, for that reason only, E2E tests work best in frontend testing, since they simulate the actual user behavior using a headless browser API.
Component testing always comes with such demerits.
Signed-off-by: Ansh Goyal <[email protected]>
Signed-off-by: Ansh Goyal <[email protected]>
Signed-off-by: Ansh Goyal <[email protected]>
Signed-off-by: Ansh Goyal <[email protected]>
| @@ -27,7 +27,7 @@ const packageNames = [ | |||
| ...babelConfiguration.plugins, | |||
| ]; | |||
|
|
|||
| const otherPackages = ['jest-environment-jsdom']; | |||
| const otherPackages = ['jest-environment-jsdom', 'babel-plugin-react-remove-properties']; | |||
There was a problem hiding this comment.
wait, why is it not auto-discovered from babelrc? I don't see babelrc change, isn't it necessary to make babel actually use the plugin, not just install it as dep?
There was a problem hiding this comment.
We are not using Babel plugins for production builds.
I was also confused earlier since we have a babel-transform.js file. I tried adding the plugin to that file, but it didn't work.
Later realized that it is only being used for tests, and not for builds.
For builds, we are using vite.config.js and depcheck can't find that out that we are using the babel plugin inside vite.config.js
There was a problem hiding this comment.
that means we should parse vite config, in addition to babel cfg, when generating exclusion list for depcheck
There was a problem hiding this comment.
Parsing it seems tricky. I applied another workaround, since depcheck by default supports .babelrc files.
Signed-off-by: Ansh Goyal <[email protected]>
yurishkuro
changed the title
Which problem is this PR solving?
TraceIDSearchInput.test.jsfrom Enzyme to RTLHow was this change tested?
Checklist
jaeger:make lint testjaeger-ui:yarn lintandyarn test