Skip to content
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.

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

Initial end-to-end test suite #178

Merged
merged 1 commit into from
Aug 24, 2019
Merged

Conversation

craigfe
Copy link
Member

@craigfe craigfe commented Aug 13, 2019

This initialises an end-to-end test suite that compares example executions against expected outputs. The process is driven by auto-generated dune files, allowing a workflow of dune runtestdune promote.

Currently this does not allow for passing options to the executable under test, which would be a nice addition. For this, we could use extra *.opt files similarly to how it's being done in OCamlformat.

Frustrations

  1. The dune action DSL has no way of negating the exit code of a command, so it's being done manually with a little script -- I might request this as a Dune feature...

  2. It's necessary to sanitize the output of the tests to remove environment dependencies / randomness. Again, this is being done by an extra script.

  3. I initially added symlinks to the examples/ directory to verify their output, but these appear to break the Windows CI, so I removed them.

@craigfe craigfe force-pushed the test-expected-output branch 7 times, most recently from fba4052 to 8674cbd Compare August 14, 2019 18:39
@craigfe craigfe marked this pull request as ready for review August 14, 2019 18:39
@craigfe craigfe requested a review from NathanReb August 14, 2019 18:39
Copy link
Member

@NathanReb NathanReb left a comment

Choose a reason for hiding this comment

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

I think this is a great improvement!

I don't think the amount of boilerplate we need for this will feel like too much once we've added more tests and as we've discussed offline, I even think parts of this could be extracted into a cram-test-like framework or upstreamed to an existing one!

I added a few minor comments but generally speaking it's 👍, thanks for your time on this!

@craigfe craigfe force-pushed the test-expected-output branch 2 times, most recently from bf94305 to 0f1f0ec Compare August 19, 2019 13:47
@samoht
Copy link
Member

samoht commented Aug 19, 2019

I don't think the amount of boilerplate we need for this will feel like too much once we've added more tests and as we've discussed offline, I even think parts of this could be extracted into a cram-test-like framework or upstreamed to an existing one!

out of curiosity, did you consider using mdx for this?

@samoht
Copy link
Member

samoht commented Aug 19, 2019

Looking good! Feel free to merge it whenever you think that's ready. That's a huge improvement over the current (empty) state.

@NathanReb
Copy link
Member

out of curiosity, did you consider using mdx for this?

I hadn't but it's true we could make it work. I'm just not sure if that's a very good idea as it's a bit outside of mdx's scope and I think it's better for mdx to focus on ensuring manuals or articles are correct than trying to also turn it into a testing framework.

I agree it wouldn't take much but I still think some features wouldn't quite fit. For instance, the random/non-deterministic output bits replacement logic we have here is quite different from what we want for mdx right now. Here we just want to replace it with a placeholder because we don't care about what it actually is as long as it matches what we expect it to be. In mdx we want a significant value in the output instead because that's clearer for the reader. I'm not quite sure how we can achieve both with a reasonable API and workflow. We could probably hack something together and make it work somehow but I'm afraid that by trying to be two different things at once the code base would suffer and we might regret it in the long run.

What I had in mind was something simpler than mdx, focused on end to end testing for CLI binaries similar to what dune uses for its blackbox tests. I was actually hoping we could extract that into a tool which we could use in both dune and alcotest.

We should probably also look into existing tools as there's probably one around!

Copy link
Member

@NathanReb NathanReb left a comment

Choose a reason for hiding this comment

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

Looks good!

Thank you very much for that, makes me want to contribute to alcotest more!

I pointed a couple unnecessary %dep in the dune rules but feel free to merge once they're fixed!

@samoht
Copy link
Member

samoht commented Aug 22, 2019

This is clearly off-topic, but mdx testing logic was extracted from dune blackbox tests :-) And merlin is using the alternate "cram" syntax of mdx to for its own blackbox testing so this is definitely what mdx is designed for.

@craigfe craigfe force-pushed the test-expected-output branch from 0f1f0ec to 14b620f Compare August 23, 2019 15:24
@craigfe craigfe force-pushed the test-expected-output branch from 14b620f to c24bf79 Compare August 23, 2019 15:27
@craigfe craigfe merged commit 17ab65d into mirage:master Aug 24, 2019
@craigfe craigfe deleted the test-expected-output branch August 18, 2020 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants