Skip to content
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

[Dashboard Navigation] Unit tests #166297

Merged

Conversation

nickpeihl
Copy link
Member

@nickpeihl nickpeihl commented Sep 12, 2023

Part of #161287

These unit tests were designed to test complex cases for the components. Please review carefully and suggest any test cases that I may have overlooked.

undefined,
undefined,
savedObjectId
);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like that we can use buildMockDashboard from other plugins. But I really needed it to accept a savedObjectId parameter. Unfortunately, this looks awful IMO. I wonder if it would be ok for the DashboardContainer constructor to receive an object of properties rather than a series of arguments?

Or I could just make my own dashboardMock function. But that isn't as much fun. 😛

@ThomThomson

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is definitely ugly, but it's a mock and they don't have to be pretty! I had to do the exact same thing here.

If you'd like to make the Dashboard Container take an object of arguments that would be great! Alternatively, you can just make buildMockDashboard optionally take an object of arguments, then pass them into the Dashboard container constructor with undefined as the defaults. That way it's more flexible.

expect(link).toHaveTextContent('https://example.com');
const clickEvent = createEvent.click(link, { ctrlKey: true });
const preventDefault = jest.spyOn(clickEvent, 'preventDefault');
fireEvent(link, clickEvent);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test logs a console error like Error: Not implemented: navigation (except hash changes). This is expected since jsdom does not handle navigation. The tests pass, but the error is annoying. Mocking the window.location methods did not resolve it. I think the way to fix this is to call preventDefault, but we need to check that it isn't called. 😖

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me! Maybe we should add a comment describing this too? 👀

@nickpeihl nickpeihl marked this pull request as ready for review September 19, 2023 19:59
@nickpeihl nickpeihl requested a review from a team as a code owner September 19, 2023 19:59
@kibana-ci
Copy link
Collaborator

kibana-ci commented Sep 19, 2023

💔 Build Failed

Failed CI Steps

Test Failures

  • [job] [logs] Jest Integration Tests #3 / checking migration metadata changes on all registered SO types detecting migration related changes in registered types
  • [job] [logs] FTR Configs #4 / Controls Range Slider Control create and edit edits title and size of an existing control and retains existing range selection
  • [job] [logs] FTR Configs #4 / Controls Range Slider Control create and edit edits title and size of an existing control and retains existing range selection
  • [job] [logs] FTR Configs #52 / dashboard app - group 1 Changing field formatter to Url applied on dashboard
  • [job] [logs] FTR Configs #52 / dashboard app - group 1 Changing field formatter to Url applied on dashboard
  • [job] [logs] FTR Configs #42 / dashboard app - group 5 embed mode non-default URL params renders as expected when scrolling
  • [job] [logs] FTR Configs #42 / dashboard app - group 5 embed mode non-default URL params renders as expected when scrolling
  • [job] [logs] Jest Tests #18 / DashboardGrid removes panel when removed from container
  • [job] [logs] Jest Tests #18 / DashboardGrid renders expanded panel
  • [job] [logs] FTR Configs #67 / discover/group2 discover esql view test should render esql view correctly
  • [job] [logs] FTR Configs #25 / Options list control Dashboard options list creation and editing Options List Control creation and editing experience renames an existing control and retains selection
  • [job] [logs] FTR Configs #25 / Options list control Dashboard options list creation and editing Options List Control creation and editing experience renames an existing control and retains selection
  • [job] [logs] Jest Tests #18 / renders DashboardGrid
  • [job] [logs] Jest Tests #18 / renders DashboardGrid with no visualizations
  • [job] [logs] FTR Configs #23 / serverless search UI Importing an existing dashboard does not show the unsaved changes badge in edit mode
  • [job] [logs] FTR Configs #23 / serverless search UI Importing an existing dashboard does not show the unsaved changes badge in edit mode
  • [job] [logs] Jest Integration Tests #2 / SO type registrations does not remove types from registrations without updating excludeOnUpgradeQuery

Metrics [docs]

‼️ ERROR: no builds found for mergeBase sha [91113c7]

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Contributor

@Heenawter Heenawter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests look great, thanks so much for tackling it!! 💃 Can't think of anything off the top of my head that wasn't covered.

@nickpeihl nickpeihl merged commit 9b268f3 into elastic:navigation-embeddable Sep 20, 2023
16 of 29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants