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

Convert custom snapshot system to jest snapshots #133

Merged
merged 5 commits into from
Feb 20, 2024
Merged

Conversation

cmdcolin
Copy link
Contributor

I was unable to properly generate snapshots using CRAMJS_REWRITE_EXPECTED_DATA=true yarn test, running twice generated snapshot failures

As an alternative, there are built-in jest snapshots

This PR does no source code changes, just test changes, so should safely port over

The snapshots are fairly large, and the large dump.test.ts file is actually split into two files to avoid the github limit of 100mb. there is no concept of compressed jest snapshots, and it would limit snapshot effectiveness potentially to limit amount of data snapshotted, so this just stays the course

Copy link

codecov bot commented Feb 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (aee975e) 89.91% compared to head (0b3e215) 90.14%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #133      +/-   ##
==========================================
+ Coverage   89.91%   90.14%   +0.22%     
==========================================
  Files          39       39              
  Lines        1983     1968      -15     
  Branches      395      388       -7     
==========================================
- Hits         1783     1774       -9     
+ Misses        199      193       -6     
  Partials        1        1              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cmdcolin
Copy link
Contributor Author

note that the snapshots needed to get updated to support #132

@cmdcolin
Copy link
Contributor Author

the diff is very large, but that is because the old snapshot system was already very large (1million+ line json files, mostly from a couple large pre-existing ce_large c.elegans cram files)

@cmdcolin
Copy link
Contributor Author

if there are concerns let me know, but i think this should be ok to go :)

@cmdcolin cmdcolin merged commit 0baea2f into master Feb 20, 2024
3 checks passed
@cmdcolin cmdcolin deleted the jest_snapshot branch February 20, 2024 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant