[Rollup] Add unit tests for Job table #31561
Conversation
💔 Build Failed |
cjcenizal
added
Team:Kibana Management
|
Pinging @elastic/es-ui |
|
|
||
| return ''; | ||
| }, | ||
| render: job => ( |
x-pack/plugins/rollup/public/crud_app/sections/job_list/job_table/job_table.js
Outdated
Show resolved
Hide resolved
|
|
||
| import { Pager } from '@elastic/eui'; | ||
|
|
||
| import { registerTestBed } from '../../../../../__jest__/utils'; |
There was a problem hiding this comment.
Maybe we can move this __jest__ directory into the public directory, since it seems unlikely that we'll be writing server-side React in the near future? Then it will be closer to the code which consumes it, and we also won't need to go up so many parent directories.
There was a problem hiding this comment.
As I quickly explained over slack, this seems to be a pattern in other plugins to have the __jest__ folder at the root level. See for example license_management that also has a util file there. I think we should stick to that convention for consistency, what do you think?
x-pack/plugins/rollup/public/crud_app/sections/job_list/job_table/job_table.test.js
Outdated
Show resolved
Hide resolved
| }); | ||
|
|
||
| it('should set the correct job value in each row cell', () => { | ||
| const fields = [ |
There was a problem hiding this comment.
Can we rename this unformattedFields to emphasize that the fields display the original fields without doing any formatting on them?
There was a problem hiding this comment.
Not sure I understand the benefit of it. It could be named fieldIds or fieldNames to be more correct and match the implementation.
There was a problem hiding this comment.
Well, the comment on line 75 points out that this array isn't all of the fields, it's just the ones which are rendered without any special formatting. We test other fields here in different ways when they're rendered with formatting, e.g. status and groups. So we can make the code more self-explanatory by naming this array something which communicates its role within the test, which IMO would be unformattedFields or fieldsRenderedWithoutFormatting -- because it contains only the fields which aren't formatted. We could name it something else, but I think the ideal name would be one which makes the comment on line 75 unnecessary.
There was a problem hiding this comment.
I understand now what you mean 😊 Thanks for the explanation 👍
| : field.replace(/^\w/, char => char.toUpperCase()); | ||
| } | ||
| return text; | ||
| }, ''); |
There was a problem hiding this comment.
It makes me raise an eyebrow when our tests replicate the logic in the code being tested but I don't really have a better suggestion. I suppose we could hardcode our fixtures instead of dynamically generating them? 🤷♂️
There was a problem hiding this comment.
Yeah, I understand. But I guess what we are doing here is preventing an unexpected change on the component rendered. Just like a snapshot would do. What do you mean by hardcoding our fixtures ?
There was a problem hiding this comment.
Sorry, I phrased that wrong. What I meant to say was that we could just hardcode our assertion here:
expect(cellGroupsText).toEqual('Histogram, terms');That way we're not duplicating the implementation, and it's easier to read and understand what we expect.
There was a problem hiding this comment.
Good point, that makes sense.
x-pack/plugins/rollup/public/crud_app/sections/job_list/job_table/job_table.test.js
Outdated
Show resolved
Hide resolved
|
Thanks for the review @cjcenizal ! I added a few comments, let me know your thoughts |
💔 Build Failed |
💔 Build Failed |
💚 Build Succeeded |
sebelga
added
the
non-issue
This PR adds unit test coverage for the Rollup job table.