Fix opamroot-versions.test glitch #5007
Conversation
|
Can’t we simply have a special rules that says "if we have a variable inside the regexp part, then take it as a static string and not as a regexp" ? |
|
yes, that would be better, but more tedious to implement |
| @@ -938,12 +938,12 @@ opam-version: "2.0" | |||
| roots: ["i-am-package.2" "i-am-sys-compiler.2"] | |||
| ### :V:1:e: reinit from 2.0 | |||
| ### ocaml generate.ml 2.0 | |||
| ### opam init --reinit --bypass-checks --no-setup | "(${OPAMROOTVERSION})" -> "current" | |||
| ### opam init --reinit --bypass-checks --no-setup | "[ (]${OPAMROOTVERSION}($|[ ,)])" -> " current" | |||
There was a problem hiding this comment.
| ### opam init --reinit --bypass-checks --no-setup | "[ (]${OPAMROOTVERSION}($|[ ,)])" -> " current" | |
| ### opam init --reinit --bypass-checks --no-setup |
Why not simply remove theme all together? We can simply update the tests when we bump the opam-root-version
There was a problem hiding this comment.
well, to make the update minimal, here is a example of update for bump
There was a problem hiding this comment.
i think it would be fine. It’s just one dune runtest --auto-promote away so I don’t see any downsides removing these. I only see downsides keeping them in
There was a problem hiding this comment.
The new output is not just taken as is, it need to be checked, updated, etc. ; and by reduced the diffs, it's simpler.
I'm missing the downsides of keeping them ? the regexp issue is fixed by #5009
There was a problem hiding this comment.
Keeping it you have to maintain them (e.g. what this PR is trying to do) and it makes it harder for us to read the reftests (e.g. you can’t tell if "current" comes from the sed or from whatever in opam) Also sometimes you might want something to not change for whatever reason and making it not change makes it harder to check that.
There was a problem hiding this comment.
For the name of the replacement, we can use another word that is not used and not meant to be used in opam, like current-opamroot-version-for-test that is human readable and should appear in opam messages (or if we are all in a hurry at that moment, but there will be more serious concerns than this test in that case :D).
For maintenance, reftests evolve, reftests engine evolves too. Something was merged with a buggy behaviour that nobody saw. My thought on keeping it is to try to have both world: fixed bug & enhanced reftest (ideally stable).
I agree with the no change that hide issues, It went into rabbit hole when debugging rec pin on windows with reftest because I didn't take into account path rewriting on reftest output. Maybe mistakenly, but here I assume that hiding is not so bug prone as it is consistent with the defined opam root version (the variable is set using opam output), and if something is broken in this test it's not just the version that will be changed, other lines will be highlighted in the diff.
There was a problem hiding this comment.
We're all overthinking this :D
Not certain it's worth it, but now that it's done let's merge, we keep our hands free to simplify in case of problems in the future.
I had to use `[)]` to escape rather than `\\)`, because backslashes interfere with the windows-path rewrite rules...
I had to use
[)]to escape rather than\\), because backslashes interferewith the windows-path rewrite rules...