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

Add (and use) a runner wrapping every test, to improve error reporting #303

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

shym
Copy link
Collaborator

@shym shym commented Feb 28, 2023

This supersedes #205, providing a custom runner written in OCaml

Add a custom runner that allows to display the result of a test in the same way on Unix and Windows (by mapping Windows error codes to their equivalent result on Unix)
It also uses GitHub CI formats when available so that test failures are referenced as such at their positions in the logs

As it is written in OCaml (rather than bash), it opens some possible improvements:

  • create a summary of all timings,
  • use per-test timeouts (the same timeout for all tests? or a custom timeout for some tests that we expect to run a long time?),
  • ensure that the global timeout is never reached by stopping the test suite a couple of minutes before, so that the last steps (post-*) can run

shym added 8 commits March 2, 2023 17:10
This custom runner allows to display the result of a test in the same
way on Unix and Windows (by mapping Windows error codes to their
equivalent result on Unix)
It also uses GitHub CI formats when available so that test failures are
referenced as such at their positions in the logs
Unfortunately, syntax such as `%{dep:%{test}}` is not understood in an
action, and simply using: `(action (run runner %{test}))` does not add
the test executable to the dependencies of the action, so dune does not
build it

We keep the standard runner for the _internal_ tests, as the custom
runner would bring no benefit there
Also add a rule to build a dummy file in case the TIMELOGDIR was never
set
Unix.time () does not fit inside a 31-bit int (which must be why it is
using float in the first place), so use floats for times
@shym
Copy link
Collaborator Author

shym commented Mar 2, 2023

I’ve added the suggested improvements because I’d like to use them to work on my cygwin branch.
What I’m undecided about:

  • whether a time-out should appear as a test failure (it could make sense to keep reporting the deadlocks we still see on Windows as failures rather than successes),
  • whether we should set a default per-test timeout, and what should it be (this already makes it possible to set TEST_TIMEOUT for a given test via the corresponding dune file).

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.

1 participant