Binary snapshots (in-memory) by lasernoises · Pull Request #610 · mitsuhiko/insta

lasernoises · 2024-09-22T22:38:58Z

The biggest difference to the previous attempt in #489 is that the snapshot contents are now kept in-memory instead of being written into a temporary file directly.

For SnapshotContents I decided to make it an enum with two variants like in the previous attempt. I first attempted to change it to three variants so it would also replace the SnapshotKind enum but I realized that a lot of logic is still shared between inline and file text snapshots. That approach would definitely also work though and it's perhaps also a question of how much the logic between the two is expected to diverge in the future.

In runtime::assert_snapshot I replaced the combination of the refval and new_snapshot_value with an enum. In that case it is a three variant enum and some From impls for two value tuples for the macros. Maybe it would be better to make it a two variant enum there too for consistency and keep the ReferenceValue enum for the text case. That might be a bit weird because both binary and non-inline text snapshot have a name and that would then be in two different enums.

I should probably also add some tests in cargo-insta. I'll do that as soon as possible, but I need to figure out how the testing there works.

This is the somewhat simplified version of mitsuhiko#489 where we keep the full files in-memory during the assert and when reviewing.

…snapshots This merge is far from compiling. I just picked the side of each conflict that makes more sense to build upon.

…snapshots Still WIP like the previous merge.

This fixes the failing tests.

…snapshots

max-sixty

It's in impressive effort! And thank you for redoing on top of the new master.

It does add lots more data structures & some complications to the code. I'm going to think about how much we could simplify (I've already simplified the existing code somewhat..). If you have ideas, that would be great too.

…sion

max-sixty · 2024-09-23T17:27:02Z

For SnapshotContents I decided to make it an enum with two variants like in the previous attempt. I first attempted to change it to three variants so it would also replace the SnapshotKind enum but I realized that a lot of logic is still shared between inline and file text snapshots. That approach would definitely also work though and it's perhaps also a question of how much the logic between the two is expected to diverge in the future.

FWIW I think the ontology is quite an interesting case — FileText & FileBinary share some properties, but FileText & InlineText also share some completely different properties. So there's no perfect hierarchy.

The proposed structure seems reasonable (as above I'd like to see if we can reduce the number of data structures a bit, but I don't think there are any easy large clean-ups)

max-sixty · 2024-09-23T18:35:57Z

To sketch out what I think we need to do to merge:

Tests — we already have a couple good integration tests. Would be great to extend those to any corner cases — for example, what if someone uses the same extension for a text snapshot? What does it look like if someone has an older version of cargo-insta? (maybe those are too specific, but anything in that direction)
@mitsuhiko needs to review, I think focused on the external API
- Obv great if we can make the internal code nice, but also we can do that later, and would be a loss if this withered based on refinements.
We should decide whether we mark as experimental. If we don't mark it as experimental, we're stuck with the API, and in some ways insta has more stringent backward-compatibility constraints than other libraries, since it needs to work with older versions of itself in the form of cargo-insta)
Docs — we should add a short section on this in https://github.com/mitsuhiko/insta-website

max-sixty · 2024-10-04T18:03:58Z

Let's add a couple of examples to the docs and then hit the big button. Thank you very much for all your persistence here, I know it was a long road. It's a great feature for insta!

lasernoises · 2024-10-04T18:15:52Z

The deleting of unreferenced snapshots does not work for binary snapshots yet. I'm working on fixing that now.

Previously it only deleted the .snap file, but not the binary file next to it.

lasernoises · 2024-10-04T18:41:04Z

Now it works, but I'm not quite sure what the best error handling strategy there is. It needs to load the snapshot now to know whether it's binary and to get the extension. It still ignores errors when deleting the files like before. So I think it should probably also ignore errors when loading the snapshot and just skip it and maybe print a log message.

The message "snapshots are matching" was previously printed when replacing a binary snapshot with an empty string text snapshot.

lasernoises · 2024-10-04T21:31:18Z

What I'm a bit more worried about is the internals module. I am changing some things that are exposed in there. The docstring does imply that it's primarily for documentation. So that might imply that it's not part of the stable API?

Yes, this is hidden and so not part of the public API (somewhat by convention). We only have it public so cargo-insta can access it.

I meant to reply to this earlier but forgot:
I think you meant the _cargo_insta_support module. Because the internals module is actually not hidden. I think given that it's called internals we can probably justify changing things exposed in it. Maybe it would make sense to add a comment there that explicitly states that this is not guaranteed to be stable.

lasernoises · 2024-10-04T22:11:52Z

I've started adding a section for the website: https://github.com/mitsuhiko/insta-website/compare/main...lasernoises:insta-website:binary-snapshots?expand=1. I think I'll wait with opening a PR until this is merged.

max-sixty · 2024-10-04T22:23:11Z

I think you meant the _cargo_insta_support module. Because the internals module is actually not hidden. I think given that it's called internals we can probably justify changing things exposed in it. Maybe it would make sense to add a comment there that explicitly states that this is not guaranteed to be stable.

Yes v good point. Would be great to add the comment (but no need to slow down this PR ofc)

max-sixty · 2024-10-04T22:24:16Z

@lasernoises anything else? Or we merge? :)

lasernoises · 2024-10-04T22:26:11Z

I think from my point of view we can merge.

lasernoises · 2024-10-04T23:56:23Z

🎉🎉🎉

Thank you so much for your patience and the many reviews/comments!

max-sixty · 2024-10-04T23:56:47Z

Thank you!!

As discussed in #610 it probably makes sense to be explicit about the `internals` module not being guaranteed to be stable. I also noticed that `TextSnapshotKind` gets exposed globally. This was added in #581 when it was still called `SnapshotKind`. I wonder if this is necessary to expose toplevel.

This PR migrates us to insta's binary snapshots. These snapshots were added via - #1563 - mitsuhiko/insta#610 <details><summary>previous blockers</summary> <p> <del>Currently, this PR is not mergable as the binary snapshots don't actually work. I encountered these two bugs:</del> - [x] <del>the binary snapshots get created as `FILENAME.snap.new.png`. This naming is inconsistent with how other snapshots are created.</del> - [x] <del>Re-running the tests creates different metadata files which contain different data:</del> ```diff --- source: martin/src/sprites/mod.rs - assertion_line: 273 expression: png - extension: png - snapshot_kind: binary --- + ``` <del> Yes, I looked if I am using the same versions of insta, I am. `cargo test` and `cargo insta` still provide really weird results </del> <del> => 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.</del> </p> </details> --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

lasernoises added 8 commits September 18, 2024 22:31

Add binary snapshots

e0612ae

This is the somewhat simplified version of mitsuhiko#489 where we keep the full files in-memory during the assert and when reviewing.

Merge remote-tracking branch 'upstream/master' into in-memory-binary-…

c6161ac

…snapshots This merge is far from compiling. I just picked the side of each conflict that makes more sense to build upon.

Merge remote-tracking branch 'upstream/master' into in-memory-binary-…

7ce931a

…snapshots Still WIP like the previous merge.

Clean up merge related issues

4aaf10e

Fix error when trying to clean up files in non-existent directory

4989161

This fixes the failing tests.

Rename some types for clarity

a970400

Merge remote-tracking branch 'upstream/master' into in-memory-binary-…

43398a7

…snapshots

Add more basic tests for binary snapshots

85dd8f9

max-sixty reviewed Sep 23, 2024

View reviewed changes

Comment thread insta/src/runtime.rs

max-sixty reviewed Sep 23, 2024

View reviewed changes

Comment thread insta/src/snapshot.rs Outdated

max-sixty reviewed Sep 23, 2024

View reviewed changes

Comment thread insta/src/snapshot.rs Outdated

max-sixty reviewed Sep 23, 2024

View reviewed changes

Comment thread insta/src/runtime.rs Outdated

max-sixty reviewed Sep 23, 2024

View reviewed changes

Comment thread insta/src/runtime.rs Outdated

max-sixty reviewed Sep 23, 2024

View reviewed changes

Comment thread insta/src/output.rs

lasernoises added 4 commits September 23, 2024 04:04

Add integration tests for binary snapshots

d3bad2a

Use Self type as parameter for matches* methods

5bb0f70

Improve panic message for forbidden .new binary snapshot file exten…

7607518

…sion

Fix MSRV related issues

58f459c

max-sixty reviewed Sep 23, 2024

View reviewed changes

Comment thread cargo-insta/tests/main.rs

max-sixty reviewed Sep 23, 2024

View reviewed changes

Comment thread insta/src/snapshot.rs Outdated

max-sixty reviewed Sep 23, 2024

View reviewed changes

Comment thread insta/src/runtime.rs

Add docstrings and rename Text to FileText in SnapshotValue enum

5aa7a13

max-sixty reviewed Sep 23, 2024

View reviewed changes

Comment thread cargo-insta/tests/main.rs Outdated

max-sixty and others added 4 commits September 23, 2024 17:30

Merge branch 'master' into in-memory-binary-snapshots

9a714ba

Merge branch 'master' into in-memory-binary-snapshots

5bab3b7

Improve error message for integration tests

907dd63

Update docstrings

931016e

max-sixty reviewed Oct 4, 2024

View reviewed changes

Comment thread insta/src/runtime.rs Outdated

Update insta/src/runtime.rs

f59925e

This was referenced Oct 4, 2024

Support serializers for images #355

Closed

Binary Snapshot Support #196

Closed

Using insta for schema generation and updates #475

Open

API to read snapshots #353

Open

Fix deletion of unreferenced binary snapshots

0a639ae

Previously it only deleted the .snap file, but not the binary file next to it.

lasernoises added 2 commits October 4, 2024 21:07

Fix snapshot printing bug when changing between binary and text

f437077

The message "snapshots are matching" was previously printed when replacing a binary snapshot with an empty string text snapshot.

Add examples to assert_binary_snapshot doc comment

e0a61c9

lasernoises commented Oct 4, 2024

View reviewed changes

Comment thread cargo-insta/src/cli.rs Outdated

max-sixty reviewed Oct 4, 2024

View reviewed changes

Comment thread insta/src/snapshot.rs

Improve error handling for deletion of unreferenced snapshots

62637b2

max-sixty merged commit 0b81773 into mitsuhiko:master Oct 4, 2024

This was referenced Oct 7, 2024

Add section on binary snapshots mitsuhiko/insta-website#28

Merged

Add warning about stability to doc comment for internals module #648

Merged

indietyp mentioned this pull request Nov 1, 2024

H-3528, H-3529: Set up Pdfium & preprocess PDFs as images hashintel/hash#5512

Merged

3 tasks

CommanderStorm mentioned this pull request Dec 15, 2024

tests: migrate to insta binary snapshots maplibre/martin#1616

Merged

2 tasks

PookieBuns mentioned this pull request Jan 9, 2025

feat: Support diffing text "binary" snapshots #708

Open

Uh oh!

Conversation

lasernoises commented Sep 22, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

max-sixty left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

max-sixty commented Sep 23, 2024

Uh oh!

max-sixty commented Sep 23, 2024

Uh oh!

Uh oh!

Uh oh!

max-sixty commented Oct 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lasernoises commented Oct 4, 2024

Uh oh!

lasernoises commented Oct 4, 2024

Uh oh!

Uh oh!

Uh oh!

lasernoises commented Oct 4, 2024

Uh oh!

lasernoises commented Oct 4, 2024

Uh oh!

max-sixty commented Oct 4, 2024

Uh oh!

max-sixty commented Oct 4, 2024

Uh oh!

lasernoises commented Oct 4, 2024

Uh oh!

lasernoises commented Oct 4, 2024

Uh oh!

max-sixty commented Oct 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

max-sixty commented Oct 4, 2024 •

edited

Loading