Add new [%%expect_in <version-range> {| ... |}] syntax for expect tests and fix 5.3 builds

[NathanReb]

This adds a new syntax for expect tests where code can be followed by multiple [%%expect_in <version-range> {| ... |}] instead of a single [%%expect{|...}|].

E.g. in the following example:

let x = 2
[%%expect_in 5.2 - {|
val x : int = 2
|}]
[%%expect_in 5.3+ {|
val x : int = 3
|}]

Depending on the compiler version, one of these blocks will be copied from the source file while the other one will be filled with the toplevel evaluation outcome of let x = 2.

Meaning if we run this test in 5.2, it will pass, while if we run it in 5.3, we'd get the following:

+++ b/_build/default/test/versioned_expect/test.ml.corrected
@@ -1,7 +1,7 @@
 let x = 2
 ;;
 [%%expect_in 5.2- {|
 val x : int = 2
 |}]
 [%%expect_in 5.3+ {|
-val x : int = 3
+val x : int = 2
 |}]

I've also proceeded to use this to fix the test for 5.3. The ones in metaquot and code_path look good to me as they are caused by changes in the compiler and how functor signatures and type aliases are printed out.

The one in quoter I'm not quite sure what to do with it as the original test is not very good, it's extremely hard to read the output and make out what's supposed to happen there.

I'm happy to split the test fixes with the new expect test feature but though it would be good to show they work!

[NathanReb]

I found a nicer fix for the quoter tests. The large AST values that were printed were just a side effect and not the actual test. That's because our expect runner did not offer a way not to see the toplevel output for a select set of phrases.

I added a [%%ignore] that can be used to evaluate the code but to discard the output and used that to fix the quoter tests instead. They don't need versioned expect blocks anymore.

[patricoferris]

Thanks @NathanReb -- this is a super useful fix which I'm sure we'll use a lot. One thought about the syntax (i.e. not blocking the PR at all!) was maybe we could match opam and dune's syntax for expressing version constraints. This means

• [%%expect_in 5.3 + could become [%%expect_in >= 5.3
• [%%expect_in 5.2 < could become [%%expect_in <= 5.2
• between could stay the same otherwise we need && or something...

I do like your syntax too in terms of not suggesting we support < and > as well!

Whatever you think :))

[patricoferris]

Maybe a comment that the expectation is the combination of ranges should cover the ranges for which the test will be run against -- I had initially thought we would have some kind of implicit ignore if a version was not found.

[NathanReb]

Yeah that's a good point, I'll change that!

The idea for not ignoring the output for versions not covered by the union of ranges was that we should try and run our tests for all versions as much as possible and that tests that absolutely don't work on certain versions should probably not be run for those at all. This will force us to isolate such tests.

[NathanReb]

Yes I think I'd rather not add something like && as contributors not well aware of how our expect test runner works could assume it's more powerful than it is and try to write more complex constraints.

I'm happy to switch to a more commonly used syntax though and use >= and <=. We can add support for < and > if we ever need to later. We could also use interval notations such as [5.1, 5.3] or something like [-, 5.2] and [5.3, +] for opened intervals which reads a little better to me than the opam notations and then the ocaml syntax make it clear you can't exclude the interval bounds because that would not parse.

Even though I prefer the interval notation I do agree consistency is probably the best here. For the interval we could maybe do something like 5.0 <=> 5.3 so it looks a bit more consistent with the rest.

[NathanReb]

Just pushed the changes in the latest commit if you want to take a look!

[patricoferris]

Awesome :))