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

[Lens] test suite - replace React specific props events with html events #113156

Merged
merged 1 commit into from
Sep 28, 2021

Conversation

mbondyra
Copy link
Contributor

@mbondyra mbondyra commented Sep 27, 2021

Summary

This is a small improvement to our test suite - most of our tests checking the correctness of the invoked events were directly invoking passed props, eg. element.props().onClick(). This can be improved by instead simulating user events, eg. element.simulate('click'). This has couple of advantages:

  • the test will work regardless of internal implementation. We check a bit more of a code (not just how it integrates with the middle component, but the path from the component we test to html. I know we don't have to check eui internals, but this way...
  • ...we test the way user uses our app
  • we don't have to assert types of the events, eg:
 wrapper.find('[data-test-subj="lnsIndexPatternFieldSearch"]').prop('onChange')!({
          target: { value: 'me' },
        } as ChangeEvent<HTMLInputElement>); 

I also simplified testing the Portal created by popover. I'll leave the comment close to the code.

@mbondyra mbondyra added Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:Lens auto-backport Deprecated - use backport:version if exact versions are needed v7.16.0 labels Sep 27, 2021
@mbondyra mbondyra requested a review from a team as a code owner September 27, 2021 18:39
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-vis-editors (Team:VisEditors)

.find('[data-test-subj="lnsIndexPatternActions-popover"]')
.first()
.prop('children') as ReactElement
).props.items[0].props.onClick();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks to mocking the portal: ReactDOM.createPortal = jest.fn((element) => element) we can just assume that the content of the popover will be placed directly inside the popover. And thanks to that we can avoid the ugly lines above.

@mbondyra mbondyra changed the title [Lens] replace react specific props events with html events [Lens] replace React specific props events with html events Sep 27, 2021
@mbondyra mbondyra changed the title [Lens] replace React specific props events with html events [Lens] test suite - replace React specific props events with html events Sep 27, 2021
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

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

// wait for indx pattern to be loaded
await act(async () => await new Promise((r) => setTimeout(r, 0)));
Copy link
Contributor Author

@mbondyra mbondyra Sep 28, 2021

Choose a reason for hiding this comment

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

I am a bit on the edge of replacing this with waitFor - I think it's more readable, but it comes from react/testing-library and we use in 99% enzyme. There are one more place where we use react-testing-library in lens (testing debouncing hook) and some other teams use it extensively in Kibana, so I am not sure. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

The behaviour is different between the 2 things.
I think waitFor is more reliable than a manual arbitrary waiting. So 👍

Copy link
Contributor

@dej611 dej611 left a comment

Choose a reason for hiding this comment

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

Code review 👍

// wait for indx pattern to be loaded
await act(async () => await new Promise((r) => setTimeout(r, 0)));
Copy link
Contributor

Choose a reason for hiding this comment

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

The behaviour is different between the 2 things.
I think waitFor is more reliable than a manual arbitrary waiting. So 👍

@mbondyra mbondyra merged commit 618d1e5 into elastic:master Sep 28, 2021
@mbondyra mbondyra deleted the lens/make_tests_user_perspective branch September 28, 2021 08:55
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Sep 28, 2021
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Sep 28, 2021
…#113234)

Co-authored-by: Marta Bondyra <marta.bondyra@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Lens release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.16.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants