Test Stanza Proposal

[rgrinberg]

Problem

Creating tests in dune requires quite a bit of extra boilerplate. To define n tests, one needs at least executables stanzas and n alias stanzas. For example, here's a snippet from ocaml-re that shows how this boilerplate can get out hand:

(alias
 ((name   runtest)
  (deps   (test_re.exe))
  (action (run ${<}))))

(alias
 ((name   runtest)
  (deps   (test_perl.exe))
  (action (run ${<}))))

(alias
 ((name   runtest)
  (deps   (test_emacs.exe))
  (action (run ${<}))))

(alias
 ((name   runtest)
  (deps   (test_glob.exe))
  (action (run ${<}))))

(alias
 ((name   runtest)
  (deps   (test_str.exe))
  (action (run ${<}))))

(alias
 ((name   runtest)
  (deps   (test_pcre.exe))
  (action (run ${<}))))

Proposal

Add a tests stanza that will allow us to define executables and attach appropriate runtest aliases to run them. A test suite composed of multiple executables would look like:

(tests
 ((names (foo bar baz))
  (libraries (str)))

The tests stanza would support fields common to alias and executables that make sense for tests. This would exclude fields like public_names for example. This would also allow us to add test specific abilities down the road. One such example would be an (expected_output <filename>) field that could make it easy to write expect tests.

[ghost]

The idea seems fine to me. I suggest to add a field (expected_output <filename>) that would be the same as capturing the output and comparing it with (diff ...). I believe that's a common pattern as well.

[rgrinberg]

@diml that would be really nice. Would this work for a tests stanza or a test stanza however? I expect that if we define multiple tests at once, then we can't really use this this field in its singular form.

[ghost]

I suppose for tests it would be expected_outputs.

[rgrinberg]

@diml Another problem to solve for expected_outputs is supporting correction files. Should we perhaps make 1 expected output of type:

type expected_output =
  | Output of string
  | Correction of stirng

A value without a constructor can be interpreted with Output.

[ghost]

What would the two constructors correspond to in the test stanza?

[rgrinberg]

The first one would correspond to the diff action, while the second would correspond to diff?.

[ghost]

diif? is for command that optionally produce a corrected file. In this case it would always exist, since we would create it by capturing the output of the command.

[ghost]

Thinking about this again, we could enable the diffing automatically if the there is a file <name>.expected in the source tree. It's probably how it will always be called.

[rgrinberg]

Jérémie Dimino <notifications@github.com> writes:

`diif?` is for command that optionally produce a corrected file. In this case it would always exist, since we would create it by capturing the output of the command.

Yeah, so I think that expected_outputs will not support such commands. I'd expect that we'd support both styles of expect tests in the tests stanaza.

[rgrinberg]

Jérémie Dimino <notifications@github.com> writes:

Thinking about this again, we could enable the diffing automatically if the there is a file `<name>.expected` in the source tree. It's probably how it will always be called.

That sounds like a good idea. It will also solve the problem of ordering the arguments in the diff action. I always forget if the expected file comes first or second. I'm still wondering if we should make this opt in by adding an (expect_tests) option to enable this.

[ghost]

I'm still wondering if we should make this opt in by adding an
(expect_tests) option to enable this.

I don't think it's necessary. Adding a file named <name>.expected seems like a good enough way to express the intent.

[rgrinberg]

I don't think it's necessary. Adding a file named .expected seems like a good enough way to express the intent.

Will this auto discovery work when we have a rule producing <name>.expected?

[ghost]

Will this auto discovery work when we have a rule producing .expected?

No it wouldn't. However, a .expected file that is not committed in the repository kinds of defeat the purpose of expectation testing.

[rgrinberg]

Jérémie Dimino <notifications@github.com> writes:

No it wouldn't. However, a .expected file that is not committed in the repository kinds of defeat the purpose of expectation testing.

I'm still a little uneasy about doing file IO during parsing like this. But will give this a try.

[ghost]

Note that for this case we would just consult the File_tree.t. The general idea in dune is that the rules are a pure function of the source tree and the installed world, so reading source files for generating rules is not a big deal.

[rgrinberg]

@diml should we make use of the parsing context to pass this File_tree parameter btw? Or should we just pass the parameter manually.

[ghost]

Why do we need it during parsing? Shouldn't we check the file tree while setting up the rules for the Test stanza?

[rgrinberg]

Jérémie Dimino <notifications@github.com> writes:

Why do we need it during parsing? Shouldn't we check the file tree while setting up the rules for the Test stanza?

The way the tests stanza currently works is that there's really nothing but a parsing stage. The tests stazna is simply immediately converted to an alias and an exe stanzas. I suppose I could just delay some of this process until gen_rules and then I will have access to the file tree that I need.

[rgrinberg]

okay @diml I've changed the approach to something that is more gen_rules heavy and also prototyped the expectation tests detection. That bit still needs some tests, but you can already review the general approach.

[rgrinberg]

The code works but unfortunately the restriction on aliases not having targets is biting me here. I'll change the code to generate a separate rule. Why did we have that restriction again?

@emillon btw, have a look at this PR as well.

[rgrinberg]

One thing that's slightly wonky is that user rule that I'm setting up isn't being accounted for in the calculation of text_files. I suppose the easiest way to fix this is to probably just eliminate the tests stanza altogether by transforming it to exe + alias or exe + rule + alias. This causes some information lose in the list of stanzas for a directory and hence is a bit unsatisfactory.

@diml do you have a suggestion on how to address this? AFAIK, this doesn't actually cause any harm apart from having the .output files generated invisible as mld or ml sources. Which wouldn't really make sense anyway.

[emillon]

About the feature itself, I think it's useful to add a short syntax for this, since it's a common use case.

The plural version (tests) has the same limitations as other plural constructs: namely, that it's impossible to define several targets that have different values (libraries). I don't think it's really a problem since in practice, these tend to go in different dune files.

A singular version (test) would probably be useful as well.

[emillon]

It might not be the best place to start a discussion about code style, but I don't think that aligning these (>>=) is a good idea. It will either require some maintenance effort every time we modify anything in that block, or become unaligned. This is not worth the minor readability benefit IMO.

[rgrinberg]

Agreed. I just try to follow whatever existing style is in place. Here I've failed because it is indeed troublesome without some sort of automation.

[emillon]

what do you think about using the "expect" and "regular" names for these two tests?

[rgrinberg]

So have 2 fields? E.g.

(tests
 (regular foo bar)
 (expect baz blue))

Yeah, I definitely like this proposal. Though I would switch regular to something like run. I think @diml has a preference for auto discovery here.

[emillon]

No, I meant using a-regular-test and an-expect-test instead of foo and bar.

As for autodiscovery vs constructor, I think that the idea of this proposal is to have a very light syntax to define test executables, so I lean slightly towards auto discovery here.

[rgrinberg]

Oops. I renamed all the tests. Okay, auto discovery it is.

[rgrinberg]

A singular version (test) would probably be useful as well.

Sure I'll add this.

[rgrinberg]

Okay, I added a singular form. Honestly, I think that these singular forms don't have benefits to justify their existence but for symmetry I guess we should allow them.

[rgrinberg]

I've added documentation for this stanza now. I think it's a pretty simple addition and hence can make it to 1.0

[ghost]

There is a word missing in this sentence

[rgrinberg]

Thanks. Fixed.