Clean up end-to-end testing #178
Conversation
craigfe
force-pushed
the
refactor-tests
branch
from
d72898e to
4dd7783
Compare
craigfe
force-pushed
the
refactor-tests
branch
from
4dd7783 to
a6bd320
Compare
|
Rebased over #171. |
craigfe
force-pushed
the
refactor-tests
branch
from
a6bd320 to
20f3709
Compare
|
Rebased over #163. |
craigfe
force-pushed
the
refactor-tests
branch
2 times, most recently
from
1d4b9b3 to
c0186c7
Compare
craigfe
force-pushed
the
refactor-tests
branch
from
c0186c7 to
46d23a7
Compare
Co-authored-by: Jules Aguillon <juloo.dsi@gmail.com>
craigfe
force-pushed
the
refactor-tests
branch
from
46d23a7 to
717b5fb
Compare
|
Rebased. |
|
CI failing for unrelated reasons; can't re-trigger it myself. |
|
Restarted it and it's passing. |
| let options = options_of_file file in | ||
| let command = | ||
| let args = file::options in | ||
| Fmt.strf "ocaml-mdx rule %a" pp_options args |
There was a problem hiding this comment.
This is calling ocaml-mdx I have on my system if I dune clean. You should add (package mdx) on every rules calling this program.
There was a problem hiding this comment.
Thanks 🙂 It's only necessary in the test_fixpoint/dune case, since other rule generation patterns don't rely on ocaml-mdx rule. Have added it there.
There was a problem hiding this comment.
I'm not super convinced by this one. I feel like we should either test ocaml-mdx test or ocaml-mdx rule but not both at the same time.
If I want to test ocaml-mdx rule, I will compare its output with a .expected file.
None of the tests in test_fixpoint actually require calling ocaml-mdx rule and this could just be another case of test_expect from what I can see. I might be wrong so let me know if it is in fact required but if it's not, I think we could detect whether the .expected file exists and compare to input on if it doesn't!
There was a problem hiding this comment.
Another advantage of my suggested approach is that we'd get rid of the annoying execvp errors!
There was a problem hiding this comment.
Regarding testing ocaml-mdx rule separately: I agree completely, but think this is something we should do next. The test directories are structured so that a test/rule_expect will fit in there nicely. The current state of affairs is how it was being done before, and I kept it this way to keep the diff smaller (not introducing a bunch of additional tests in the middle of a refactor).
Regarding making test_fixpoint a conditional behaviour of test_expect in the case where the expected file doesn't exist: I'd rather not do this. I'd rather the behaviour was kept explicit, and that each test_foo be kept as simple as possible. The code can be simplified in another way by testing ocaml-mdx rule generation separately and then not depending on ocaml-mdx rule to generate tests of other properties of the binary.
There was a problem hiding this comment.
I agree about keeping the behaviour explicit!
My main point was that I think you could just make the current test_fixpoint_rule very similar to test_expect_rule. I agree it changes how things were done but I don't think it will change what's tested too much and we could add proper ocaml-mdx rule tests in a separate PR.
Let's merge it like this, I'll improve it later on, you already had to rebase this quite a few times by now!
There was a problem hiding this comment.
This is amazing work, thank you very much @craigfe, those test needed some polishing!
I have a few comments about the current implementation but nothing major.
Another thing I'd like to go is to go even further and move each test case-ish to a separate subfolder to avoid having test files depend on each other which is still the case currenlty. We can take care of that separately!
| let options = options_of_file file in | ||
| let command = | ||
| let args = file::options in | ||
| Fmt.strf "ocaml-mdx rule %a" pp_options args |
There was a problem hiding this comment.
I'm not super convinced by this one. I feel like we should either test ocaml-mdx test or ocaml-mdx rule but not both at the same time.
If I want to test ocaml-mdx rule, I will compare its output with a .expected file.
None of the tests in test_fixpoint actually require calling ocaml-mdx rule and this could just be another case of test_expect from what I can see. I might be wrong so let me know if it is in fact required but if it's not, I think we could detect whether the .expected file exists and compare to input on if it doesn't!
| let options = options_of_file file in | ||
| let command = | ||
| let args = file::options in | ||
| Fmt.strf "ocaml-mdx rule %a" pp_options args |
There was a problem hiding this comment.
Another advantage of my suggested approach is that we'd get rid of the annoying execvp errors!
Co-authored-by: Jules Aguillon <juloo.dsi@gmail.com> Co-authored-by: Nathan Rebours <nathan.p.rebours@gmail.com>
craigfe
force-pushed
the
refactor-tests
branch
from
1e89464 to
c1ef284
Compare
This cleans up the end-to-end test plumbing:
it categorises the tests according to the feature-under-test, rather than managing all of the tests in the same
dunefile;it splits up test files that were being used to test multiple features (in particular,
section.mdhas been gradually accumulating more responsibilities... see #161 and #167);it adds a new
gen_dune_rules.mlexecutable responsible for generating some of the more common patterns. This picks up any test-specific CLI options from<test>.optfiles;it adds
runtesttriggers for.mdfiles that were previously being ignored (e.g.environment_variable.md). The introduction of the auto-generation script prevents this problem from re-occurring in the future.This is based on #177 (unmerged) for convenience, and also tests the new functionality in that PR via the
test_fixpoint/cramtest case.