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

ci: ensure snapshots are always cleaned up #903

Merged
merged 8 commits into from
Apr 18, 2024

Conversation

G-Rath
Copy link
Collaborator

@G-Rath G-Rath commented Apr 5, 2024

While doing #902 I realised we've got a couple of packages using snapshots but not cleaning them up afterwards - this adds a basic script to check for that, based on the existence of the __snapshots__ directory.

Here's what the annotations look like:

image

Note that because annotations are only shown for files, we annotate the first test file in a directory that doesn't contain testmain_test.go to ensure it is visible.

@codecov-commenter
Copy link

codecov-commenter commented Apr 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.37%. Comparing base (ca67f63) to head (e0807a6).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #903   +/-   ##
=======================================
  Coverage   63.37%   63.37%           
=======================================
  Files         145      145           
  Lines       11883    11883           
=======================================
  Hits         7531     7531           
  Misses       3885     3885           
  Partials      467      467           

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

@G-Rath G-Rath force-pushed the sort-snapshots branch 2 times, most recently from 5c6b065 to ae45387 Compare April 5, 2024 20:49
@G-Rath
Copy link
Collaborator Author

G-Rath commented Apr 5, 2024

This should be landed after #904

another-rex pushed a commit that referenced this pull request Apr 9, 2024
While doing #902 I realised we've got a couple of packages using
snapshots but not cleaning them up afterwards - this changes that; I've
also opened #903 adding a script to check all packages using snapshots
are cleaning them.
@another-rex
Copy link
Collaborator

It might be worth it to change the bash script to python to improve readability, the ubuntu-latest image should come with python preinstalled. (https://github.com/actions/runner-images/blob/main/images/ubuntu/Ubuntu2204-Readme.md).

@G-Rath
Copy link
Collaborator Author

G-Rath commented Apr 14, 2024

ubuntu-latest image should come with python preinstalled.

Yes, but local environments (such as Windows) might not - I'm also not sure if it'll actually be that much more readable given how small the script is (I think it's mainly the while and if statements that are not immediately obvious); but I'll rewrite it anyway cause we're not expecting this to be a major thing 😄

@G-Rath G-Rath marked this pull request as ready for review April 17, 2024 05:14
Copy link
Collaborator

@another-rex another-rex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks! I think having it in Python actually makes it easier to run on Windows right? As they won't have bash/grep unless they are running on Linux.

@G-Rath
Copy link
Collaborator Author

G-Rath commented Apr 17, 2024

As they won't have bash/grep unless they are running on Linux.

True though all of our other scripts are written in bash so Windows users will need "bash" installed already (and that doesn't mean they have to use Linux-on-Windows)

@another-rex
Copy link
Collaborator

How does the snapshot cleanups affect our acceptance tests if we have snapshots in them? Will it "cleanup" snapshots for those skipped tests?

@G-Rath
Copy link
Collaborator Author

G-Rath commented Apr 17, 2024

No, that's why we our skipping is done using snaps.Skip rather than t.Skip directly as then go-snaps still sees the tests we're skipping and doesn't clean them up.

https://github.com/gkampitakis/go-snaps?tab=readme-ov-file#skipping-tests

@another-rex another-rex merged commit 5979bb6 into google:main Apr 18, 2024
13 checks passed
@G-Rath G-Rath deleted the sort-snapshots branch April 18, 2024 19:46
josieang pushed a commit to josieang/osv-scanner that referenced this pull request Jun 6, 2024
While doing google#902 I realised we've got a couple of packages using
snapshots but not cleaning them up afterwards - this adds a basic script
to check for that, based on the existence of the `__snapshots__`
directory.

Here's what the annotations look like:

<img width="739" alt="image"
src="https://github.com/google/osv-scanner/assets/3151613/b85d4500-6d00-4faa-a8d0-6dc1358b4b80">

Note that because annotations are only shown for _files_, we annotate
the first test file in a directory that doesn't contain
`testmain_test.go` to ensure it is visible.
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.

3 participants