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

test: replace mocks with snapshots #1220

Merged

Conversation

markov00
Copy link
Member

Summary

We use mocks for checking the equality of very big objects.
The PR refactored that code using snapshot instead: they provide the same functionality with less burden.

Details

This can simplify the refactoring in the following cases:

  • a field is removed or slightly changed
  • a completely new complex field is added. In particular, it is helpful when the new field is derived from others part of the data structure
  • The mocks are themself code with some logic. They are not a guarantee to be bug-free.

If we are testing an object structure, we probably can benefit directly from snapshots instead of manually writing the object.
The only caveat is to threat snapshots as code, checking them during the review as they where written inline in the test

@markov00 markov00 added :test Missing or to be fixed test :xy Bar/Line/Area chart related labels Jun 28, 2021
@markov00 markov00 marked this pull request as ready for review June 28, 2021 16:58
Copy link
Contributor

@rshen91 rshen91 left a comment

Choose a reason for hiding this comment

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

LGTM - This changing to snapshots makes sense to me especially in terms of the context of changing the data logic for the null values in tooltips PR. I think it should be manageable as a team for us to check any snapshot changes in a PR in future reviews (to make sure snapshots aren't being changed for bad reasons).

Copy link
Collaborator

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

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

LGTM

@markov00 markov00 enabled auto-merge (squash) June 29, 2021 14:46
@markov00 markov00 merged commit 4fd27d1 into elastic:master Jun 29, 2021
@nickofthyme
Copy link
Collaborator

🎉 This PR is included in version 31.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@nickofthyme nickofthyme added the released Issue released publicly label Jul 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Issue released publicly :test Missing or to be fixed test :xy Bar/Line/Area chart related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants