open Rezn.Frontend
open Yojson.Basic
let%expect_test "parsing a minimal pod" =
let source = {| pod "x" { image = "nginx" } |} in
let prog = parse_string source in
let json = Rezn.Codegen.program_to_json prog in
json |> pretty_to_string |> print_endline;
[%expect {| [ { "kind": "pod", "name": "x", "fields": { "image": "nginx" } } ] |}]
Is this a good test?
I’d argue “no”:
Yojson.Safe than Yojson.Basic, because the former is the one that most people use. This is a fairly minor point however.Yojson.Safe.equal for comparisons.open modules. I know pretty_to_string comes from Yojson.Basic but a casual reader of the source might not and has to look through all the opens to figure out where the function comes from. This might be a lesser concern with go-to-definition by Merlin but generally I’d still prioritize readability of tests.Thank you so much for taking the time to write such a detailed reply!
A dune snapshot test wouldn’t need a PPX either: Bare-bones unit testing in OCaml with dune - DEV Community
I would argue it’s important to be able to inspect and easily diff the parsed printed JSON as that is the critical functionality here. Expect or snapshot tests make that easy, custom equality tests may not. The JSON format changing from one version to another would be annoying but imho it should not be a regular occurrence and it would be a small price to pay for the convenience of easy diffing.
Given we’re talking about best practices I’ll have to disagree here.
If you compare with Yojson.{Safe,Basic}.equal you get the same advantage but the comparison is on the parsed representation which is what the user code is outputting so it already tests the value from user code. Alcotest allows you to specify a prettyprinter, so in the failure case it is also able to show you the expected and received values. For a “friendlier” representation you can also define the prettyprinter to convert the value to JSON in case you prefer that over the more-ocamly AST representation that Yojson.{Safe,Basic}.pp gives you by default.
The downside you gain by turning it into the string representation string is added fragility and checking that Yojson does the right thing turning a Yojson.{Basic,Safe}.t into a string (which is already tested in the Yojson test suite).
The latter might have some advantages in rare cases, e.g. if your JSON parser is not very good and will fail to parse slightly differently formatted code so you need to make sure it doesn’t change but I’d say that risk is low and probably better resolved by fixing the consumer.
All of that said, the Yojson string representation is fairly stable, as there isn’t much innovation going on in the JSON-as-string space 
But it did change slightly in Yojson 2.0.x, when the easy-format dependency was removed.
You don’t get the advantage of automatic diffing in case of test failure, which snapshot/expect testing gives you.
Alcotest allows you to specify a prettyprinter, so in the failure case it is also able to show you the expected and received values.
But not in a diff format, which would have made it easy to pinpoint the exact differences.
if your JSON parser is not very good and will fail to parse slightly differently formatted code so you need to make sure it doesn’t change but I’d say that risk is low and probably better resolved by fixing the consumer.
The consumer isn’t a JSON parser, it’s a parser for a custom config language. The idea is to parse this custom language and output JSON.
the Yojson string representation is fairly stable
This is exactly my point, test breakages because of Yojson changing its JSON pretty-printing should be very rare and in fact it was even done correctly following the rules of SemVer because the format changed in Yojson v2+.
Thank you both for the valuable input. I’m still relatively new to OCaml and its ecosystem, so I appreciate the insights. Should either of you be interested in following up on what I’m building, feel free to check out:
But that’s the thing, there was a change in formatting between 1.x and 2.0.0 and then again in 2.0.1. This change would not break tests that use equal, but the exact representation is implementation defined, a bit like a hashmap does not provide guarantees with regards of the order of keys.
IMO, while this is a convenience for the programmer, it should not be an excuse to write (what I take to be) worse tests. Of the many downsides of basing tests around concrete string representations, the one I’ve had to slog thru most recently is that it makes cross-platform testing a real pain and can require all sorts of tedious and brittle workarounds to deal with path, encoding, and line ending normalization.
My rule of thumb is: don’t test raw string input/output unless what you are trying to tests it the parsing and generation of concrete string representations. Otherwise, I think you are inviting a lot of fragility and violating the proper abstraction boundaries of the code under test.
Testing CLI’s via dune’s cram testing is a special case here, in my view: if you are testing a CLI, you are necessarily testing the concrete string I/O.
Based on the name of this test, parsing a minimal pod, I would think that what we mainly want to focus on here is testing that we can read parse a string into the expected program. So I would probably inclined to do nothing with JSON here at all: rather, I would just want some tests that parsed into values of the program type and then compared that against my expected value of the time.
I would want a separate set of tests to check that, given a pre-determined program, I can generate the expected Yojson. This may actually be a nice place to employ property based testing.
I would only use expect tests for testing CLI interaction and/or pretty printing of my custom data (e.g., if you have a concrete representation of programs).
Nice looking project, thanks for sharing! The bits of code I looked over seem quite clean too Looking forwarding to seeing how it evolves
As an addendum, IMO whether or not this is “a good test” depends on what it is trying to test. IMO, this is a good test if you are trying to test round-trip parsing from string to string, and you care about the concrete JSON representation (e.g., it’s important that it all be on one line, or indented etc.). But IMO it’s not a great test of just the parsing, because it can fail for reasons that having nothing to do with the parsing (e.g., because Yojson changes something).
In the latter case, iou still do want to be testing the JSON but not the particular concrete details, I’d parse the expected JSON to a Yojson from a string, but not compare against a string.
2 posts were split to a new topic: Brittleness of snapshot tests
I just wanted to say thank you once again to everyone who contributed to this thread. I learned a lot simply by reading the replies, including the spin-off discussion around snapshot test brittleness, which I found especially insightful.
I’d honestly love to invite you all for a coffee somewhere and keep the conversation going!