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.

Sign up for GitHub

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

Jump to bottom

add snapshot names to all snapshot asserts #1656

Closed
ethanoroshiba opened this issue Oct 15, 2024 · 0 comments · Fixed by #1690
Closed

add snapshot names to all snapshot asserts #1656

ethanoroshiba opened this issue Oct 15, 2024 · 0 comments · Fixed by #1690
Assignees
@ethanoroshiba
Labels
code-quality testing

Comments

@ethanoroshiba
Copy link
Contributor

ethanoroshiba commented Oct 15, 2024 •
edited
Loading

As noted here, many snapshots throughout the codebase do not have an associated name, and are hence based on line number. If assertions are moved around, then the snapshots will break, which should not happen. We should add names to all snapshot tests.

┆Issue Number: ENG-916
@ethanoroshiba ethanoroshiba mentioned this issue Oct 15, 2024
Merged
@ethanoroshiba ethanoroshiba self-assigned this Oct 15, 2024
@ethanoroshiba ethanoroshiba mentioned this issue Oct 18, 2024
Merged
github-merge-queue bot pushed a commit that referenced this issue Dec 6, 2024
@ethanoroshiba
chore(core, sequencer): add names to all snapshot tests (#1690)
7e00378 
## Summary
Added names to all snapshot tests which did not have them.

## Background
If a certain test had multiple snapshot assertions, they would be named
based on their ordering. Hence any reordering, removal, or addition of
the snapshots would break the test, which should not be the case. This
change aims to fix this problem as well as establish a best practice of
naming snapshots moving forward.

## Changes
- Added names to all snapshot tests and regenerated all snapshots not
within Conductor (due to code freeze)

## Testing
Passing all tests.

## Related Issues
closes #1656
ethanoroshiba added a commit that referenced this issue Dec 6, 2024
@ethanoroshiba
chore(core, sequencer): add names to all snapshot tests (#1690)
1ac7e0a 
## Summary
Added names to all snapshot tests which did not have them.

## Background
If a certain test had multiple snapshot assertions, they would be named
based on their ordering. Hence any reordering, removal, or addition of
the snapshots would break the test, which should not be the case. This
change aims to fix this problem as well as establish a best practice of
naming snapshots moving forward.

## Changes
- Added names to all snapshot tests and regenerated all snapshots not
within Conductor (due to code freeze)

## Testing
Passing all tests.

## Related Issues
closes #1656
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ethanoroshiba ethanoroshiba

Labels
code-quality testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

chore(core, sequencer): add names to all snapshot tests astriaorg/astria
1 participant
@ethanoroshiba