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

tests: migrate to insta binary snapshots #1616

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

CommanderStorm
Copy link
Collaborator

This PR migrates us to insta's binary snapshots.
These snapshots were added via

Currently, this PR is not mergable as the binary snapshots don't actually work. I encountered these two bugs:

  • the binary snapshots get created as FILENAME.snap.new.png. This naming is inconsistent with how other snapshots are created.
  • Re-running the tests creates different metadata files which contain different data:
    ---
    source: martin/src/sprites/mod.rs
    - assertion_line: 273
    expression: png
    - extension: png
    - snapshot_kind: binary
    ---
    +
    
    Yes, I looked if I am using the same versions of insta, I am. cargo test and cargo insta still provide really weird results

=> to be mergable, this PR would need work upstream.
I am going to try to write this up during the christmas holidays and provide a fix upstream.

@nyurik
Copy link
Member

nyurik commented Dec 15, 2024

Note that image (binary) snapshots might not be ... stable, i.e. SDF->PNG, especially when multiple images are packed as a single sprite might produce slightly different results on different platforms. So in short - this might cause more confusion than its worth. Maybe. I am not saying lets not do it, i'm saying we may not get what we want in some cases - so we should check multiple platforms on this on

@CommanderStorm
Copy link
Collaborator Author

CommanderStorm commented Dec 15, 2024

I don't quite see where platform issues might come into image encoding/optimisation.
They SHOULD encode the same way, right?

The conclusion does not change though.
This is blocked upstream first before we can even consider running it through our CI.

@nyurik
Copy link
Member

nyurik commented Dec 15, 2024

well, technically any hashmap-based implementation is already randomized - so even that may produce different results for image packing. Plus any kind of SVG->PNG encoding might in theory differ depending on arm64 vs x64 (in theory)...

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.

2 participants