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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Teach dune to focus on one particular test #366

Merged
merged 2 commits into from
Jul 6, 2023

Conversation

shym
Copy link
Collaborator

@shym shym commented Jun 12, 2023

This PR adds the possibility to use dune to focus on one particular test, in a similar way to what we do with github actions. This will make it easy to enable focus testing in our CI by using dune build @@runtest ... rather than dune runtest ....

The capability was tested with CI on commit a247a5e50f (thanks for the OPAM workflows which caught early the missing package annotation! 馃憤).

We could imagine using that same mechanism to run focused tests also in github actions:

  • Pros: this would use the same configuration across testing platforms,
  • Cons: this would introduce bash in the testing stack on Windows / Cygwin (I don鈥檛 know whether this could introduce noise or not); this would probably require setting some CI parameter to avoid double runs on Cygwin (which was patched in CI: Skip Cygwin part 2 when running only one test聽#357).

While doing this, I wondered whether it would be useful to split the test suite into more aliases than the single runtest, for instance to separate some tests that are prone to sporadic failures 馃

@jmid
Copy link
Collaborator

jmid commented Jun 12, 2023

Oh wow! 馃槷

Yes, I can definitely see this being useful - both for eliminating platform-specific logic and for enabling it across the two CI systems 馃槂 馃憤

  • Is there something special about the @@runtest alias? dune runtest is a shorthand for dune build @runtest - but I'm too dune-illiterate to know @@runtest... 馃
  • To anticipate a forthcoming MSVC port, I suppose we could avoid introducing a bash dependency by using a (not-so-pretty) generated dune file, akin to what I did here: https://github.com/c-cube/qcheck/pull/250/files - The nice thing is that we could generate the file from OCaml. I'm thinking a naive "repeat 20" generates a 20-line action clause (that would be simple - which is good in the context of testing, but something more clever may be possible).

@shym
Copy link
Collaborator Author

shym commented Jun 12, 2023

  • Is there something special about the @@runtest alias? dune runtest is a shorthand for dune build @runtest - but I'm too dune-illiterate to know @@runtest... 馃

@runtest is (alias_rec runtest) (so runtest is all subdirectories) whereas @@runtest is (alias runtest) (only in the current directory).

  • [...] using a (not-so-pretty) generated dune file, akin to what I did here [...]

I don鈥檛 understand how this works in that example, but that鈥檚 an idea. How useful do you find the feature to run all the tests and get a final count of the number of failures at the end? (I didn鈥檛 find how to get this without bash, but it might be possible with cmd?).

@shym shym marked this pull request as draft June 20, 2023 08:22
shym added 2 commits July 3, 2023 14:09
Add a dune `ci` alias to control what to build in CI; its meaning can be
defined by setting the `DUNE_CI_ALIAS` environment variable, and
defaults to `runtest`
Add `ci1` and `ci2` aliases to split the test-suite into two parts
(for slow runners such as Cygwin)
Add a `focusedtest` alias that will repeat one test multiple times,
using either `bash` or `cmd` to loop and using dune's `diff` action to
fail as soon as one of the run failed
Simplify CI workflows now that dune configuration supports all the
combinations (only one test or the full suite, on all platforms)
@shym
Copy link
Collaborator Author

shym commented Jul 5, 2023

I rewrote that PR last week to take your comment into account. It is now providing:

  • a new @ci alias to be used in CI (and configured to depend on what we want to run), instead of modifying the @runtest alias, since the goal is more explicit:
    • by default, @ci will build the alias in the $DUNE_CI_ALIAS environment variable if it exists, or @runtest,
  • support for Windows to focus on one test, using cmd rather than bash to repeat a test,
  • more of the logic for focused tests is converted to dune action, using diff to error out if at least one of the runs failed (listing the iterations that failed, by the way, so it will be faster to find them in the log),
  • for slow platforms (aka Cygwin), two aliases @ci1 and @ci2 are provided, to split up the test suite into manageable pieces,
  • the Github common workflow is greatly simplified.

The last commit on that branch shows how to focus on one test (and modify the repeat counts).

@shym shym marked this pull request as ready for review July 5, 2023 09:16
(write-file hoped "")
(write-file failed-runs "")
(run cmd /q /c
"for %G in (1,2,3,4, 5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20) do (echo Starting %G-th run && focusedtest.exe -v || echo %G >> failed-runs)")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit/OCD trigger warning: extra space after 4 in 1,2,3,4, 5,6,...

@jmid
Copy link
Collaborator

jmid commented Jul 6, 2023

Cool! I think I follow 馃槄
You're using the environment variable DUNE_CI_ALIAS only to support breaking up the Cygwin workflow in two IIUC?
That means it can be eliminated once we identify the reason for why Cygwin is taking so long and get back to running it in one workflow (which would again enable further dune simplifications...)

With this, if we switch Multicoretests-CI to run opam exec -- dune build "@ci" -j1 --no-buffer ... too we would enable 'focused tests' there too, right?

What happens if one runs dune runtest ... in the console on the PR's setup? 馃
Will it perform a full run as usual, while the CI system's @ci alias allow either a full run or a focused test run?

@shym
Copy link
Collaborator Author

shym commented Jul 6, 2023

You're using the environment variable DUNE_CI_ALIAS only to support breaking up the Cygwin workflow in two IIUC?

That鈥檚 its only use, at least as it stands. And as it is easy to override the environment variable only in Github CI, I have no other application in mind.

With this, if we switch Multicoretests-CI to run opam exec -- dune build "@ci" -j1 --no-buffer ... too we would enable 'focused tests' there too, right?

Exactly!

What happens if one runs dune runtest ... in the console on the PR's setup? thinking Will it perform a full run as usual, while the CI system's @ci alias allow either a full run or a focused test run?

The standard runtest alias is left unmodified, so no strange effect there.

@jmid
Copy link
Collaborator

jmid commented Jul 6, 2023

The standard runtest alias is left unmodified, so no strange effect there.

That's a beautiful solution! 鉂わ笍
We get both the benefit of optional 'focused tests' in the CI without affecting the "usual console workflow" 馃帀

@jmid
Copy link
Collaborator

jmid commented Jul 6, 2023

CI summary

  • macOS 5.1 failure to trigger both STM {int,int64} ref test parallel asymmetric
  • Cygwin 5.1 and trunk part 1 cancelled(?!)
  • Linux-arm64-5.1 and Linux-arm64-5.2 failure to trigger Lin DSL Out_channel test with Domain
  • Linux-s390x-5.2 lots of failures during thread_createtree - with Atomic with error message
    Thread 1175477 killed on uncaught exception Sys_error("Thread.create: Resource temporarily unavailable")

None of these are caused by the changes of the current PR.
The last one is however interesting as we haven't seen it before IIRC... 馃

@jmid jmid merged commit 9cb4f47 into ocaml-multicore:main Jul 6, 2023
20 of 24 checks passed
@shym shym deleted the dune-onetest branch July 7, 2023 07:51
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.

None yet

2 participants