-
Notifications
You must be signed in to change notification settings - Fork 250
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
Use toMatchSnapshot inside expectMetricsJson #3359
Comments
The ideas comes from this thread : #3346 (comment) |
Great idea. I do think that using snapshots can result in lazy developers (me included) that update the snapshots without thinking too hard about it, resulting in bugs. But that is more something we will need to keep an eye on. Q: Do you want to create a custom matcher yourself? Or do you want to use chai-jest-snapshot, which we are already using in the instrumenter integration tests. Maybe developing it yourself is easier. |
I would argue the contrary, the fact that when you need to update these tests takes a long "human" time with very low value makes the task annoying and since you've already given X amout of time on this task, you take less time in the "analyzing" part.
I think creating a custom matcher would be easier, should I place this logic somewhere in the "helpers" directory and include it from each test ? If I understood correctly, all e2e tests use mocha for the verification of the metrics, is that correct ? |
Yes, we use mocha in the various await expectMetricsJsonEqualsSnapshot() |
I tried to add chai-jest-snapshot and it turns out it wasn't that complex after all. |
Fixed with #3360 |
Is your maintenance request related to a problem? Please describe.
When changing the number of generated mutators, all expectMetricsJson expectations need to be manually changed.
Describe the solution you'd like
I've tried to add .toMatchSnapshot() instead, so that it would instead be possible to "regenerate" all snapshot files of the metrics.
Describe alternatives you've considered
Haven't found other ideas
Additional context
Copy/Pasting the mocha output was quite tedious on my Pull Request, and took probably more time than developping the feature itself.
The text was updated successfully, but these errors were encountered: