[EuiDataGrid] Add unit tests for body/ components + minor refactors#5125
Merged
cee-chen merged 11 commits intoelastic:masterfrom Sep 2, 2021
Merged
[EuiDataGrid] Add unit tests for body/ components + minor refactors#5125cee-chen merged 11 commits intoelastic:masterfrom
cee-chen merged 11 commits intoelastic:masterfrom
Conversation
- Uses a non-inline snapshot, since this is a fairly complex component - NOTE: this is only ~80% coverage, as a lot of the class fns are extremely native DOM specific (focus, etc.) and is better tested in Cypress than Jest
- They're not great, but there's a lot going on here that probably needs to be split up for more comprehensive unit-focused tests
- `key` was a little nebulous and I was struggling to figure out what the data structure shoudl look like - I think tweaking the type def naming would help clarify
- It's used in two separate places with essentially the same definition - the `as any` cast doesn't seem to be needed anymore - I'm not seeing typescript errors
- DefaultColumnFormatter is repeated in DataGridBody and DataGridFooterRow - Moving the fns to their own util file lets us write unit tests for them instead of having to dive into props and also removes extra complexity from the already-long DataGridBody file
- since comparators are schema related, and data_grid_schema contains several other similar utility exports/fns - goal is to continue making data_grid_body more encapsulated/focused on (e.g.) display logic and letting other components handle the detailed sort logic + unit tests will be written later for data_grid_schema
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5125/ |
Contributor
|
💯 on all the refactors! Will take another pass to look at the tests. |
thompsongl
approved these changes
Sep 2, 2021
| }); | ||
|
|
||
| // TODO: This is likely better handled/asserted by Cypress. | ||
| // I'm leaving it in for now to provide example of props/inMemoryValues |
Contributor
Author
There was a problem hiding this comment.
Haha, I just realized there's a typo in that. doh, pushing up that fix, and also adding another quick .simulate() test for #5120 that just merged in
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5125/ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
I originally started writing unit tests on the top-level files and realized those were some of the most complex 😅 I ended up going from small files to large for my own sanity.
Code coverage of new tests:
I strongly recommend following along by-commit. Here's a tl;dr breakdown of changes:
EuiDataGridCellandEuiDataGridBodythat (IMO) makes the files a little simpler to read through and easier to unit test. I've laid out my reasoning for each change in the commit messages - would definitely appreciate folks' 2c on them!Checklist
N/A - added tests are internal/dev-only changes, and code refactors did not change functionality, but file locations only