-
Notifications
You must be signed in to change notification settings - Fork 841
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[EuiDataGrid] Reorganize & add unit tests for row height utils #5354
Conversation
}); | ||
|
||
describe('resetGrid', () => { | ||
it('invokes grid.resetAfterRowIndex at the last cached/visible row', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The one thing I couldn't really figure out how to clearly unit test was the Math.min(this.lastUpdatedRow, visibleRowIndex)
line 😬 that was partially due to setTimeout shenanigans - if we think it's important (I still get a little worried about us resetting grid at Infinity) I'd definitely appreciate pointers!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what I extended this case into to cover the min logic; tests that resetAfterRowIndex
is only called once, and with the expected, lowest, value
it('invokes grid.resetAfterRowIndex at the last cached/visible row', () => {
rowHeightUtils.setRowHeight(99, 'a', 34, 99);
rowHeightUtils.setRowHeight(101, 'a', 34, 101);
rowHeightUtils.resetGrid();
expect(mockGrid.resetAfterRowIndex).toHaveBeenCalledTimes(1);
expect(mockGrid.resetAfterRowIndex).toHaveBeenCalledWith(99);
mockGrid.resetAfterRowIndex.mockReset();
rowHeightUtils.setRowHeight(99, 'a', 34, 99);
rowHeightUtils.setRowHeight(97, 'a', 34, 97);
rowHeightUtils.resetGrid();
expect(mockGrid.resetAfterRowIndex).toHaveBeenCalledTimes(1);
expect(mockGrid.resetAfterRowIndex).toHaveBeenCalledWith(97);
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, I'll add this in!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opted to split this out into a separate test block for clearer test naming/documentation
jenkins test this |
- For easier dev readability - Leave generic/shared utils at top of the class NB: no actual code should have changed in this commit, just locations
6e19d54
to
9282c59
Compare
Preview documentation changes for this PR: https://eui.elastic.co/pr_5354/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5354/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woo! What a great, comprehensive addition of tests!
Summary
This PR:
any
typeChecklist
N/A, tech debt/internal-only changes