fix: dune test dirtest.t/run.t running cram test incorrectly #11869
Conversation
bin/runtest.ml
Outdated
| @@ | ||
| let open Option.O in | ||
| let* basename = Path.Source.basename_opt path in | ||
| if String.equal basename Source.Cram_test.fname_in_dir_test |
There was a problem hiding this comment.
This looks fishy to me, there should be a way to look up tests without having to replicate dune's own logic for discovering them. The logic should be roughly like this:
- Given a path
foo, look up the source directory for the parent of this path. Let's call itparent foo. - If
parent foocontains a cram test withchop_prefix (basename foo) ~suffix:".t", then we found our test. - Otherwise, we check if
foois therun.tinside a cram test. For that, we now look for a testparent foo. - if we find such a test, we return it if the "script" of the said test is equal to the user provided path.
All of this logic should hopefully be shared as much as possible with the logic for discovering cram tests in the rules.
There was a problem hiding this comment.
I think there is a subtle issue with this logic which is why I have to correct things here. If the user provides a.t/run.t then by (1) we have parent foo = "a.t". Now we also satisfy (2) because run.t looks like a file cram test to us. Even if we load the cram rules in a.t we will have run.t reported erroneously as a cram test. When we load rules we don't go into a.t/ subdirectories for cram tests so we don't have this issue.
I'm also not sure what logic you would like to share with loading the cram tests, there we are given a parent directory and simplify check if there are cram tests there, whereas here we are given directory that could ambiguously be a file or directory cram test (or the script of a directory cram test), we then have to disambiguate and check that its parent directory actually contains such a cram test.
There was a problem hiding this comment.
I pushed out a commit sketching out what I had in mind. Seems to be working
rgrinberg
force-pushed
the
fix-foo_t_run_t
branch
2 times, most recently
from
97fa2e0 to
a30a138
Compare
Alizter
force-pushed
the
fix-foo_t_run_t
branch
4 times, most recently
from
a22f777 to
66f6c26
Compare
Alizter
force-pushed
the
fix-foo_t_run_t
branch
from
66f6c26 to
2acaf2c
Compare
We fix an issue where a user can write ``` dune test dirtest.t/run.t ``` and the corresponding cram test was being run incorrectly. This was due to the buggy parent detection code in runtest.ml. We fix how the parent directory of a cram test is actually detected and do some cleanup for cram source related code. The runtest.ml test is updated to better reflect what we expect the command to do. Co-authored-by: Rudi Grinberg <me@rgrinberg.com> Signed-off-by: Ali Caglayan <alizter@gmail.com>
Alizter
force-pushed
the
fix-foo_t_run_t
branch
from
2acaf2c to
ab6d23b
Compare
2 participants
We fix an issue where a user can write
and the corresponding cram test was being run incorrectly. This was due to the buggy parent detection code in runtest.ml. We fix how the parent directory of a cram test is actually detected and do some cleanup for cram source related code.
The runtest.ml test is updated to better reflect what we expect the command to do.