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

Refactor tests that rely only on saved snapshots #11149

Closed
zharinov opened this issue Aug 7, 2021 · 5 comments · Fixed by #13324
Closed

Refactor tests that rely only on saved snapshots #11149

zharinov opened this issue Aug 7, 2021 · 5 comments · Fixed by #13324
Assignees
Labels
priority-4-low Low priority, unlikely to be done unless it becomes important to more people status:in-progress Someone is working on implementation type:refactor Refactoring or improving of existing code

Comments

@zharinov
Copy link
Collaborator

zharinov commented Aug 7, 2021

What would you like Renovate to be able to do?

It's easy to maintain incorrect behavior for pretty long periods, once snapshot is (re)written with bad data.
Snapshots are good when we have other assertions focused on particular aspects.
Otherwise, they may be abused as an easy way to escape mental effort required for writing better tests.

Did you already have any implementation ideas?

Identify and refactor such tests in small batches.

@zharinov zharinov added type:feature Feature (new functionality) status:requirements Full requirements are not yet known, so implementation should not be started priority-5-triage labels Aug 7, 2021
@zharinov zharinov self-assigned this Aug 7, 2021
@zharinov
Copy link
Collaborator Author

zharinov commented Aug 7, 2021

@rarkins @viceice What do you think?

@viceice
Copy link
Member

viceice commented Aug 7, 2021

Sound good. 👍

@HonkingGoose HonkingGoose added priority-2-high Bugs impacting wide number of users or very important features status:ready type:refactor Refactoring or improving of existing code and removed type:feature Feature (new functionality) status:requirements Full requirements are not yet known, so implementation should not be started priority-5-triage labels Aug 8, 2021
@HonkingGoose
Copy link
Collaborator

I think we're good to start refactoring now we've identified the tests that rely on a single snapshot?

I've labeled this priority-2-important. The reasoning is that by doing the refactoring we can check if we have any bad behavior right now, and prevent future bad behavior from slipping through our test safety net. 😄 Feel free to assign a different priority if you like!

@rarkins
Copy link
Collaborator

rarkins commented Aug 9, 2021

I'm not sure you need to move 100% of them to explicit asserts. I think inline snapshots are much less likely to get "accidentally" broken than non-inline ones, so I think in some cases they would be good enough and quicker to migrate to. We should reserve snapshots only for big results.

@rarkins rarkins added priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others and removed priority-2-high Bugs impacting wide number of users or very important features labels Aug 13, 2021
@rarkins rarkins added priority-4-low Low priority, unlikely to be done unless it becomes important to more people and removed priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others status:ready labels Aug 26, 2021
@renovate-release
Copy link
Collaborator

🎉 This issue has been resolved in version 31.15.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
priority-4-low Low priority, unlikely to be done unless it becomes important to more people status:in-progress Someone is working on implementation type:refactor Refactoring or improving of existing code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants