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

feat(update): add update by environment variable #1

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

nkxxll
Copy link

@nkxxll nkxxll commented Sep 9, 2024

Added the second option to update a snapshot test with an environment variable very inspired by the original snapshot testing library from TigerBeetle (see tigerbeetle/tigerbeetle/blob/main/src/testing/snaptest.zig#L109-L112).

This option was, tested but a unit test is kind of useless in this case. The test included the update of the CustomStruct structure like shown in the code snippet below (first snippet before update, second commands, third after update):

test "update env var" {
    const oh = OhSnap{};
    const foobar = CustomStruct{ .foo = 42, .bar = 23 };
    try oh.snap(
        @src(),
        \\
        ,
    ).expectEqual(foobar);
}
# fail on test "update env var"
zig build test
# command to update
SNAP_UPDATE=1 zig build test
# "update env var" test passes
zig build test
test "update env var" {
    const oh = OhSnap{};
    const foobar = CustomStruct{ .foo = 42, .bar = 23 };
    try oh.snap(
        @src(),
        \\ohsnap.CustomStruct
        \\  .foo: u64 = 42
        \\  .bar: u66 = 23
        ,
    ).expectEqual(foobar);
}

PS: I know the code is a little bit repetitive if you have an idea how make it look nicer I'm open for changes.

PPS: I know this is out of the blue. I looked at snapshot testing, then the blog entry from TigerBeetle, then thought I could outsource the snap lib from, them then saw your project and missed this feature while using it hope you like it. Like the idea of this project very much ❤️!

Added the second option to update a snapshot test with an environment
variable very inspired by the original snapshot testing library from
TigerBeetle. This option was, tested but a unit test is kind of useless
in this case. The test included the update of the `CustomStruct`
structure like shown in the code snippet below (first snippet before
update, second after update):

```zig
test "update env var" {
    const oh = OhSnap{};
    const foobar = CustomStruct{ .foo = 42, .bar = 23 };
    try oh.snap(
        @src(),
        \\
        ,
    ).expectEqual(foobar);
}
```

```bash
 # fail on test "update env var"
zig build test
 # command to update
SNAP_UPDATE=1 zig build test
 # "update env var" test passes
zig build test
```

```zig
test "update env var" {
    const oh = OhSnap{};
    const foobar = CustomStruct{ .foo = 42, .bar = 23 };
    try oh.snap(
        @src(),
        \\ohsnap.CustomStruct
        \\  .foo: u64 = 42
        \\  .bar: u66 = 23
        ,
    ).expectEqual(foobar);
}
```
@alexrp
Copy link

alexrp commented Nov 17, 2024

@mnemnion any thoughts on this one? It seems like it would be a nice improvement to the workflow.

@mnemnion
Copy link
Owner

Thanks for the PR! I'll take a look at it and give it a spin.

Philosophically, I'm somewhat resistant to a bulk-update option. However, I've been hoist on my own petard at least once. Especially as combined with pretty, there can be a lot of detail in the snapshots, and cosmetic refactors like changing a type name usually invalidates every snapshot using that type.

So given the interest, I'm inclined to set my dogma to the side.

Discussion: is an environment variable the best way to do it? My first inclination is to make it a command line option using -D, but I haven't actually tried getting build options to imports, as opposed to providing them to the root compilation unit directly. I think that would require user code to add something to their build.zig to thread the option through? I'll look into it.

Also, speaking from total ignorance here: is an environment variable Windows-friendly? If not that tilts toward a build option.

@mnemnion
Copy link
Owner

This also reminded me that @src() regressed on Zig master and ohsnap remains unusable on 0.14, because the information needed to open the source file is still missing.

We're expecting a release by the end of the year, so I need to follow through on that. Hopefully that affordance will be restored in some form, if not I'll need to figure out a way to keep the library functional.

I have a copy of 0.13 around, so that won't get in the way of testing the patch. I'll try to get to that by mid-week.

@alexrp
Copy link

alexrp commented Nov 18, 2024

Discussion: is an environment variable the best way to do it? My first inclination is to make it a command line option using -D, but I haven't actually tried getting build options to imports, as opposed to providing them to the root compilation unit directly. I think that would require user code to add something to their build.zig to thread the option through? I'll look into it.

FWIW, for my part, if this is doable (and I believe it is), I would definitely prefer it to an environment variable.

Here's how we do it in Zig itself:

To thread an option through, I believe you just pass it to b.dependency(...) and the build system will pass it along to the dependency's build.zig as if it was provided on the command line.

@mnemnion
Copy link
Owner

As long as I can keep setup beginner friendly (meaning a copy-pasteable block in the README), which it's starting to look like I can, then I'm definitely leaning in the direction of a config option. Any project which has more of those will be in a position to customize that code to their needs, I figure.

Thanks for the feedback, I'll look into this soon.

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