fix: dune test dirtest.t/run.t running cram test incorrectly #11869
Conversation
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. Signed-off-by: Ali Caglayan <alizter@gmail.com>
| let+ parent_dir = Path.Source.parent parent_dir_of_run in | ||
| parent_dir_of_run, parent_dir | ||
| else None | ||
| in | ||
| let open Memo.O in | ||
| Source_tree.nearest_dir parent_dir |
There was a problem hiding this comment.
Why do we use nearest_dir here? Shouldn't we be doing an exact search?
There was a problem hiding this comment.
We are taking a possible file, we want this to be the nearest directory above. Otherwise we have to handle the file and directory cases separately.
| @@ | ||
| 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 A3B5 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.
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.