8000 fix: dune test dirtest.t/run.t running cram test incorrectly by Alizter · Pull Request #11869 · ocaml/dune · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix: dune test dirtest.t/run.t running cram test incorrectly #11869

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: dune test dirtest.t/run.t running cram test incorrectly #11869

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 20 additions & 8 deletions bin/runtest.ml
View file
Open in desktop
8000
Original file line number Diff line number Diff line change
8000 Expand Up @@ -33,6 +33,20 @@ let runtest_info =
;;

let find_cram_test path ~parent_dir =
(* We correct the [path] and [parent_dir] if we have a directory test and the
user has given a path to "run.t" rather than the directory cram test itself. *)
let path, parent_dir =
Option.value ~default:(path, parent_dir)
@@
let open Option.O in
let* basename = Path.Source.basename_opt path in
if String.equal basename Source.Cram_test.fname_in_dir_test
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Given a path foo, look up the source directory for the parent of this path. Let's call it parent foo.
  2. If parent foo contains a cram test with chop_prefix (basename foo) ~suffix:".t", then we found our test.
  3. Otherwise, we check if foo is the run.t inside a cram test. For that, we now look for a test parent foo.
  4. 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

then
let* parent_dir_of_run = Path.Source.parent path in
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we use nearest_dir here? Shouldn't we be doing an exact search?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

>>= Dune_rules.Cram_rules.cram_tests
Expand All @@ -42,13 +56,11 @@ let find_cram_test path ~parent_dir =
>>| List.filter_map ~f:Result.to_option
(* We search our list of known cram tests for the test we are looking
for. *)
>>| List.find ~f:(fun (test : Source.Cram_test.t) ->
let src =
match test with
| File src -> src
| Dir { dir = src; _ } -> src
in
Path.Source.equal path src)
>>| List.find_map ~f:(fun (test : Source.Cram_test.t) ->
let open Option.O in
let* cram_basename = Source.Cram_test.path test |> Path.Source.basename_opt in
let* basename = Path.Source.basename_opt path in
Option.some_if (Filename.equal cram_basename basename) (test, parent_dir))
;;

let explain_unsuccessful_search path ~parent_dir =
Expand Down Expand Up @@ -92,7 +104,7 @@ let disambiguate_test_name path =
let open Memo.O in
find_cram_test path ~parent_dir
>>= (function
| Some test ->
| Some (test, parent_dir) ->
(* If we find the cram test, then we request that is run. *)
Memo.return (`Test (parent_dir, Source.Cram_test.name test))
| None ->
Expand Down
20 changes: 10 additions & 10 deletions src/source/cram_test.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,11 @@ type t =
; dir : Path.Source.t
}

let is_cram_suffix = String.is_suffix ~suffix:".t"
let fname_in_dir_test = "run.t"
let suffix = ".t"
let is_cram_suffix = String.is_suffix ~suffix

let dyn_of_t =
let to_dyn =
let open Dyn in
function
| File f -> variant "File" [ Path.Source.to_dyn f ]
Expand All @@ -19,19 +21,17 @@ let dyn_of_t =
[ record [ "file", Path.Source.to_dyn file; "dir", Path.Source.to_dyn dir ] ]
;;

let path = function
| File file -> file
| Dir d -> d.dir
;;

let name t =
String.drop_suffix
~suffix:".t"
(match t with
| File file -> Path.Source.basename file
| Dir { file = _; dir } -> Path.Source.basename dir)
|> Option.value_exn
path t |> Path.Source.basename |> String.drop_suffix ~suffix |> Option.value_exn
;;

let script t =
match t with
| File f -> f
| Dir d -> d.file
;;

let fname_in_dir_test = "run.t"
23 changes: 20 additions & 3 deletions src/source/cram_test.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,31 @@
open Import

(** [t] represents the source file associated to a cram test. *)
type t =
| File of Path.Source.t
| Dir of
{ file : Path.Source.t
; dir : Path.Source.t
}

val is_cram_suffix : string -> bool
val dyn_of_t : t -> Dyn.t
val name : t -> string
val to_dyn : t -> Dyn.t

(** Checks if a filename has the ".t" suffix for a cram test. *)
val is_cram_suffix : Filename.t -> bool

(** The "run.t" filename for directory cram tests. *)
val fname_in_dir_test : Filename.t

(** The [name] of a cram test. If this is a file test, then it will be the file
name without the cram suffix. If this is a directory test, then it will be
the directory name without the cram suffix. *)
val name : t -> string

(** The [path] associated to a cram test. If this is a file test, then it will
be the file. If this is a directory test, then it will be the directory. *)
val path : t -> Path.Source.t

(** The [script] of a cram test. If this is a file test, then it will be the
file. If this is a directory test, then it will be the "run.t" file inside
that directory. *)
val script : t -> Path.Source.t
38 changes: 17 additions & 21 deletions test/blackbox-tests/test-cases/runtest-cmd.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,54 +6,50 @@ Here we test the features of the `dune runtest` command.

$ cat > mytest.t <<EOF
> $ echo "Hello, world!"
> "Goodbye, world!"
> "Goodbye, world!"
> EOF
$ mkdir -p tests/myothertest.t
$ echo 'Hello, world!' > tests/myothertest.t/hello.world
$ cat > tests/myothertest.t/run.t <<EOF
> $ cat hello.world
> "Goodbye, world!"
> "Goodbye, world!"
> EOF
$ cat > tests/filetest.t <<EOF
> $ echo "Hello, world!"
> "Goodbye, world!"
> "Goodbye, world!"
> EOF


This should work:

$ dune test tests/myothertest.t
File "tests/myothertest.t/run.t", line 1, characters 0-0:
Error: Files _build/default/tests/myothertest.t/run.t and
_build/default/tests/myothertest.t/run.t.corrected differ.
[1]

There's no diff produced because the test passes

$ dune promotion diff tests/myothertest.t/run.t

This should not work

$ dune test myotherttest.t
Error: "myotherttest.t" does not match any known test.
[1]
We use the promotion diff to make sure that a promotion is being registered. If
there is no promotion it will warn.

This is a bug. Running the test this way does not correctly include the
dependencies.
If the user writes the run.t file of a directory test, we should correct it to
be the corresponding directory cram test.

$ dune test tests/myothertest.t/run.t
File "tests/myothertest.t/run.t", line 1, characters 0-0:
Error: Files _build/default/tests/myothertest.t/run.t and
_build/default/tests/myothertest.t/run.t.corrected differ.
[1]

$ dune promotion diff tests/myothertest.t/run.t

$ cat _build/.promotion-staging/tests/myothertest.t/run.t
$ cat hello.world
cat: hello.world: No such file or directory
[1]
"Goodbye, world!"
We cannot give the name of a cram test in a subdirectory and expect Dune to
find it.

$ dune test myothertest.t
Error: "myothertest.t" does not match any known test.
[1]

$ dune promotion diff tests/myothertest.t/run.t
Warning: Nothing to promote for tests/myothertest.t/run.t.

Passing no arguments to $ dune runtest should be equivalent to $ dune build
@runtest.
Expand Down Expand Up @@ -128,7 +124,7 @@ messages are informative enough.
Error: "testt" does not match any known test.
Hint: did you mean tests?
[1]
- Note that this doesn't handle the case where the path is mostly correct but
- Note that this does not handle the case where the path is mostly correct but
the directory is mispelled.
$ dune test testss/myothertest.t
Error: "testss/myothertest.t" does not match any known test.
Expand Down
Loading
0