Cram stanza

[rgrinberg]

Introduce a cram stanza. I'm presenting this PR as WIP to discuss the syntax.

The current stanza looks like this:

(cram
 (deps ..)
 (alias foo))

This will convert all .t files in the directory to individual tests, and all directories containing .t files into full directory tests.
For example:

$ ls -R .
  dune
  run.t
  foo/run.t
  foo/bar

Will contain 2 tests:

• run.t with no additional dependencies.
• foo/run.t which will depend on all the source files inside foo.

Does that make sense?

• define cram stanza
• sanitizer support
• port doc tests
• port blackbox tests
• port action plugin tests

[ghost]

A couple of questions:

What happens with something like this:

$ ls -R .
  dune
  run.t
  foo/run.t
  foo/bar/run.t
  foo/bar/baz/run.t

? i.e. which run.t are single file tests and which ones are full directory tests? and for directory ones, what sub-tree do they depend on?

Is the alias field mandatory or does it default to runtest?

[rgrinberg]

To cut down on the boilerplate, me and @jeremiedimino agreed on a slightly different system. The idea is that we first enable cram tests in the dune-project file:

(cram_tests)

This will make dune search for .t files and directories and interpret them as individual tests. A .t file will be a stand alone test, and a .t directory will need to include a run.t file. The .t directory will be treated as a data only directory and the test inside this directory will immediately depend on the contents of this dir.

To customize the various options of a test, we'll have a cram stanza. For example:

(cram
 (tests * \ foo) ;; apply this stanza to all tests but foo
 (alias ..) ;; defaults to runtest
 (deps ..)
 (enabled_if ..))

@jeremiedimino I know we discussed an inheritance rule for nested cram stanzas, but I'm not really sure that it's required for v1. Also, it would be adhoc to do it just for cram and not allow this for other stanzas. Shall we just skip it for now? For now the boilerplate isn't a big deal for us since we'll still have gen_tests.

@samoht @emillon if the above doesn't work for the tests that you're writing, let us know.

[ghost]

@rgrinberg we should aim at getting rid of gen_tests. I expect that other projects have similar needs, so if we can completely formalise our testsuite via the new cram stanza, this will be more useful for other projects. It'll also avoid back and forth on the design after the initial release.

[emillon]

@rgrinberg I'll try that and let you know of that goes. thanks!

[rgrinberg]

@jeremiedimino some issues we need to address:

• Do we raise an error if there's no .t file in a test directory (that ends with .t)?

• how do we handle multiple .t files in a single .t directory. For example:

$ ls test.t
run.t foo.t

• We initially talked about the cram stanza of working the following way:

(cram
 (alias foo)
 (enabled_if bar)
 (per_test
   (foo
    ((alias foo)
     (enabled_if bar)))

But we could easily rewrite the above as:

(cram
 (per_test
  (*
   ((alias foo)
    (enabled_if bar)))
   (foo
    ((alias foo)
     (enabled_if bar)))

Essentially, any time we want to apply fields to all tests, we just need to put them under *. Perhaps we should entertain a simpler form of cram:

(cram
  (* (.. global fields)))
  (foo (..specific fields)))

[rgrinberg]

@jeremiedimino ready for review.

Just to confirm, the force_alias feature that you've proposed, it's going to be depend on the evaluation order of the cram stanzas, right? That is, the last matched cram_stanza that has force_alias will match?

As a counter argument, we could turn alias into an OSL field and just have :standard mean the currently evaluated set of aliases.

[ghost]

What I had in mind is the following:

(cram
  (force_alias foo)
  (applies_to <pattern>))

means that @foo always executes the tests mached by applies_to, independently of any enabled_if attached to this stanza or any other stanza with an overlapping applies_to. So the order doesn't matter.

[ghost]

Please also add a documentation page in the manual. You should follow the style of test.rst. i.e. go from the most common and simple stuff to the more export one.

[rgrinberg]

@emillon you can try this feature out now.

[rgrinberg]

@jeremiedimino In the applies_to field, did we decide to match the full filename foo.t or just without the extension foo?

[rgrinberg]

Seems like there's something wonky in how we generate dune constraints.

[ghost]

Indeed

[ghost]

@jeremiedimino In the applies_to field, did we decide to match the full filename foo.t or just without the extension foo?

I don't think we decided. But thinking about it, if we match on the full filename, for the recursive scheme we could allow things like this:

(cram
 (applies_to foo bar)
 ...)

which would be equivalent to:

(subdir foo
 (cram
  ...))

(subdir bar
 (cram
  ...))

and would avoid the repetition.

[rgrinberg]

@jeremiedimino I've added some high level documentation. Will document the stanza, once we sort out the last details. By the way, I'm getting some second thoughts about standalone file tests. The reason being is that if you have a set of tests like this:

$ ls tests
test1.t
test2.t
test3.t

It becomes too easy for these to observe each other's effects when they run. Worse still, writing something like this is also error prone:

$ ls -R tests
  bin/
    dune
    run.ml # some helper script
  test.t

Now the commands in test.t can observe the dune file and the source for this binary.

Perhaps we should sandbox such tests by default? Otherwise, I think we should consider getting rid of them as they lead to fragile test suites.

[ghost]

Sandboxing such tests feels like the right default in any case. So we should do that.

[rgrinberg]

What I had in mind was that a cram stanza with applies_to would apply to the whole sub-tree where it appears. In practice, do we need anything more fine-grained than that? It's simple and I feel like it would be enough for Dune for instance, and would probably be enough for other projects as well. WDYT?

Did you mean without applies_to? That seems fine to me. Another option is (applies_to (predicate ..)) vs. (applies_to all_tests_in_this_dir_recursively). To make things a bit more explicit.

I don't think we decided. But thinking about it, if we match on the full filename, for the recursive scheme we could allow things like this:

Would we allow globs? E..g (applies_to foo/*/bar*)?

It can be done, but then we'd need to look for cram stanza in every single parent directory of the tests if I'm understanding your proposal correctly.

[ghost]

Did you mean without applies_to?

Indeed

That seems fine to me. Another option is (applies_to (predicate ..)) vs. (applies_to all_tests_in_this_dir_recursively). To make things a bit more explicit.

That's more explicit indeed, however the additional (predicate ...) feels quite heavy. I feel like documenting the behaviour properly is enough here.

Would we allow globs? E..g (applies_to foo//bar)?

I don't see why not. However, I'm not too sure about matching of the whole path rather than just the sub-directory. If we match on the whole path, then there is a performance impact; for each test in the hierarchy, we need to match it with all cram stanzas in all parent directories instead of just the current directory.

It can be done, but then we'd need to look for cram stanza in every single parent directory of the tests if I'm understanding your proposal correctly.

Yes. But I had in mind to match only on the basenames, which means that you can still compute a single default per directory, which can then be memoized. So it's almost the same cost as the non-recursive version.

[rgrinberg]

@jeremiedimino I've added sandboxing.

[rgrinberg]

@jeremiedimino I've expanded the documentation.

[emillon]

@rgrinberg I started to test this in ocamlformat. It looks like this is going to work very well!

For now, the tests are executed in a different dune-project, so the following feedback might not apply to all configurations.

I tested 8f40638. You can see the diff at https://github.com/ocaml-ppx/ocamlformat/compare/master...emillon:dune-cram?expand=1.

I encountered a few bugs:

• Promotion does not work:

     % dune runtest
         patdiff (internal) (exit 1)
    (cd _build/.sandbox/9e6b4512daad652344733a3b4a11067b/default && /home/etienne/src/ocamlformat/_opam/bin/patdiff -keep-whitespace -location-style omake -unrefined test/cli/run.t test/cli/run.t.corrected)
    ------ test/cli/run.t
    ++++++ test/cli/run.t.corrected
    File "test/cli/run.t", line 4, characters 0-1:
     |When there is no argument, an error message is displayed:
     |
     |  $ ocamlformat
    +|  ocamlformat: Must specify at least one input file, or `-` for stdin
    +|  [1]
     % dune promote
     % dune runtest
         patdiff (internal) (exit 1)
    (cd _build/.sandbox/9e6b4512daad652344733a3b4a11067b/default && /home/etienne/src/ocamlformat/_opam/bin/patdiff -keep-whitespace -location-style omake -unrefined test/cli/run.t test/cli/run.t.corrected)
    ------ test/cli/run.t
    ++++++ test/cli/run.t.corrected
    File "test/cli/run.t", line 4, characters 0-1:
     |When there is no argument, an error message is displayed:
     |
     |  $ ocamlformat
    +|  ocamlformat: Must specify at least one input file, or `-` for stdin
    +|  [1]
     % dune promote test/cli/run.t
    Warning: Nothing to promote for test/cli/run.t.

(note that only test/cli/dune-project contains (cram enabled))

(but copying the missing parts make dune runtest succeed)

• If a .t file is empty (or has only expectations or comments), the whole build errors with:

  Error: /tmp/dune.cram.b1a192err_default_several_file.t/cram.metadata: No such
  file or directory

• dune runtest -w exits when there is a diff.

• I didn't declare any dependency, but it found the ocamlformat binary in PATH, so there might be a sandboxing issue (but maybe it's just picking the installed one). I'll investigate more once all tests have been ported.

I'll keep you updated once I have ported more, but so far this looks very promising!

[rgrinberg]

Promotion does not work:

This seems to be an issue in the master branch. @jeremiedimino perhaps this is related to the finalize change?

If a .t file is empty (or has only expectations or comments), the whole build errors with:

Thanks. Will fix.

dune runtest -w exits when there is a diff.

Will investigate thanks. We really need better tests for watch mode.

I didn't declare any dependency, but it found the ocamlformat binary in PATH, so there might be a sandboxing issue (but maybe it's just picking the installed one). I'll investigate more once all tests have been ported.

I think it's probably picking the installed binary. @aalekseyev does sandboxing scrub environment variables?

[rgrinberg]

dune runtest -w exits when there is a diff.

Weird, I just tried this out in master and I get the same behavior.

[ghost]

I can reproduce it as well. Passing --debug-backtrace helped trace the raising to the | None -> Exn_with_backtrace.reraise exn branch in forward_error in fiber.ml. The exception is Memo.Cycle_error _, which is odd. This needs more debugging. I'm away for the rest of the day, so I won't be able to look deeper into this.

[rgrinberg]

@jeremiedimino I got rid of runtest-no-deps in this PR. It's easier than doing it separately and then rebasing this massive PR.

[ghost]

Seems reasonable.

[ghost]

About the size of this change; we know it's going to get in and rebasing it is painful. So I suggest that we proceed as follow:

• we merge it now
• we continue opening tickets for things that comes up as @emillon has been doing
• we make sure to address all the relevant tickets before the release, to minimise the amount of versioning boilerplate afterwards

[ghost]

Docs look good BTW :)