-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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(mocks): move mocks to shared folder (closes #16192) #17117
Conversation
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
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.
Looks good except still a NaN here
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.
Whoops.. fixed it for real now. Problem was that the insight short_id was reused across the insights in the cards. They have unique short_ids now.
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
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.
Looks good now
Problem
This is a rework of #16192 on top of current master (was easier than resolving the merge conflicts).
The issue mentioned in the previous review with bold number popping up as NaN in the visual regression test doesn't occur any more, since the test was removed in #16108 (comment).
Jest has been complaining about duplicate manual mocks like the following. Additionally we're re-creating these mocks in multiple places throughout the app.
Changes
This PR moves the mocks to a central folder for re-use between different tests. It would be nice to even auto generate those and associated mock dates.
How did you test this code?
CI run