Luke Boswell (Oct 08 2024 at 22:02): Luke Boswell (Oct 08 2024 at 22:02):I'd like to upgrade the roc_cli tests to use snapshots... because I'm going a bit crazy chasing minor changes. Is it ok to use https://rust-exercises.com/advanced-testing/02_snapshots/00_intro which describes the use of the insta library?
Luke Boswell (Oct 08 2024 at 22:03):Ah, actually it looks like we are already using it elsewhere... I'm going to give it a go
Richard Feldman (Oct 08 2024 at 22:05):sounds good to me! I've liked insta in the places we've used it :thumbs_up:
Luke Boswell (Oct 08 2024 at 22:12):I think it's going to dramatically simplify that test module too which is super nice
Luke Boswell (Jun 30 2025 at 22:14):Should the snapshots be getting checked for diffs in CI?
Luke Boswell (Jun 30 2025 at 22:15):It seems to be a common source of merge conflicts
Luke Boswell (Jun 30 2025 at 22:15):We should be checking these in the PR review
Kiryl Dziamura (Jun 30 2025 at 22:16):aren't they already checked for diffs? or what do you mean?
Luke Boswell (Jun 30 2025 at 22:16):They are checked for any diffs in CI
Luke Boswell (Jun 30 2025 at 22:17):And they are also easy to get merge conflicts
Luke Boswell (Jun 30 2025 at 22:17):I'm not sure it's a big problem or anything... just fishing for ideas/opinions
Richard Feldman (Jun 30 2025 at 22:34):I actually would love to just give them a naming convention that's enforced, like "if the snapshot's name ends with _err2 then it should report 2 errors , and similar with like _warn2err5 etc." and we enforce that in ci
Richard Feldman (Jun 30 2025 at 22:35):basically so if you make a change that causes snapshots to change the number of errors or warnings being reported, that's a CI failure
Luke Boswell (Jun 30 2025 at 22:35):Do we want fuzz crashes and the like in snapshot tests?
Luke Boswell (Jun 30 2025 at 22:36):I'm wondering if we should be maintaining a smaller number of higher quality snapshots, as opposed to the shotgun we currently have
Luke Boswell (Jun 30 2025 at 22:40):Richard Feldman said:
I actually would love to just give them a naming convention that's enforced, like "if the snapshot's name ends with
_err2then it should report 2 errors , and similar with like_warn2err5etc." and we enforce that in ci
I'm thinking of taking this idea a little further... adding "tags" or similar flags we can use in the META section to express the intent for the snapshot.
Luke Boswell (Jun 30 2025 at 22:46):One issue I know I want to fix is we print the Idx for patterns and type vars etc... except this isnt stable. If we change the number of nodes etc we get a really noisy diff.
I want to change the s-expressions to use Regions I think as the unifying thing as that is stable. So the type vars etc still use idx's under the hood but when displaying they lookup the region for the referenced node and use that.
Richard Feldman (Jun 30 2025 at 23:27):I think those will still change quite a bit when the node structure changes
Luke Boswell (Jun 30 2025 at 23:32):Maybe I didn't explain it very well. I've made the change and I think it looks much nicer.
module [foo]
foo : Str
foo = "one"
(can-ir
(d-let
(p-assign @4.1-4.4 (ident "foo"))
(e-string @4.7-4.12
(e-literal @4.8-4.11 (string "one")))
(annotation @4.1-4.4
(declared-type
(ty @3.7-3.10 (name "Str"))))))
(inferred-types
(defs
(d-assign @4.1-4.4 (type "Str")))
(expressions
(expr @4.7-4.12 (type "Str"))))
Now we don't have any node id's. I think this will reduce the noise a lot.
Luke Boswell (Jun 30 2025 at 23:42):@Jared Ramirez in the TYPES section above, would d-assign be clearer as pattern?
Luke Boswell (Jun 30 2025 at 23:42):Or even patt
Jared Ramirez (Jun 30 2025 at 23:43):Imo pattern of or d-pat is clearer
Luke Boswell (Jun 30 2025 at 23:44):I like patt it's the same length as expr so everything lines up.
Jared Ramirez (Jun 30 2025 at 23:45):I also have a todo in there for this: sometimes it's nice to print out the type var IDs and the entire type store when debugging. This definitely should not be committed but it would be cool if there was a way to easily enable that with a flag.
Luke Boswell (Jun 30 2025 at 23:45):d-pat would be similar, but we already have defs as the parent, so maybe we don't need the d- prefix
Luke Boswell (Jun 30 2025 at 23:45):Should be trivial to add a flag to debug print the type store. We have --verbose on the snapshot tool already.
Jared Ramirez (Jun 30 2025 at 23:46):ptrn? patt is fine tho
Luke Boswell (Jun 30 2025 at 23:47):Also what do you think of flattening it out and removing the type as well?
Luke Boswell (Jun 30 2025 at 23:48):(inferred-types
(defs
(d-assign @4.1-4.4 "Str"))
(expressions
(expr @4.7-4.12 "Str")))
Nvm I remembered we are using the parens for attributes, only region info is displayed without enclosing parens because it's so obvious.
Luke Boswell (Jun 30 2025 at 23:56):@Joshua Warner this should also make the side-by-side HTML version your working on much easier I think.
Luke Boswell (Jul 01 2025 at 00:13):And it's magical... we're back to diffs in PR's that are related to the changes :clap: :smiley:
Brendan Hansknecht (Jul 01 2025 at 03:19):Luke Boswell said:
Do we want fuzz crashes and the like in snapshot tests?
I think we definitely should keep them. They are minimal repros for bugs that were not caught elsewhere.
Brendan Hansknecht (Jul 01 2025 at 03:20):Unless snapshot tests take too long (which I don't think should ever happen), we should keep as many as possible, especially small ones that reproduce issues we find
Luke Boswell (Jul 01 2025 at 03:21):@Brendan Hansknecht did you have any thoughts on my upper bound perf idea? I mentioned you in the snapshot summary PR
Brendan Hansknecht (Jul 01 2025 at 03:21):Luke Boswell said:
Richard Feldman said:
I actually would love to just give them a naming convention that's enforced, like "if the snapshot's name ends with
_err2then it should report 2 errors , and similar with like_warn2err5etc." and we enforce that in ciI'm thinking of taking this idea a little further... adding "tags" or similar flags we can use in the
METAsection to express the intent for the snapshot.
That's a good idea.
Anton (Jul 01 2025 at 10:18):Once the reference counting pass gets added to the snapshots, the exact position of the incs and decs is definitely important. Example
Brendan Hansknecht (Jul 01 2025 at 15:13):Yes, though those are maybe better tested with valgrind and running code.... That said, having both is totally reasonable
Anton (Jul 07 2025 at 15:50):Can you explain how the newly introduced (#7949) EXPECTED sections work in the snapshots @Richard Feldman? I've been unable to make zig build snapshot fail by modifying the EXPECTED sections in .md snapshot files, which is unexpected :p
Richard Feldman (Jul 07 2025 at 17:08):zig build test verifies them
Richard Feldman (Jul 07 2025 at 17:09):zig build snapshot still just regenerates all the stuff under them; my thinking was that zig build snapshot should always succeed, and then zig build test is responsible for detecting and reporting failures
Anton (Jul 07 2025 at 17:14):Alright, sounds good
Last updated: Jul 26 2025 at 12:14 UTC