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

Disable dune cache for test suite #6138

Closed
wants to merge 1 commit into from

Conversation

emillon
Copy link
Collaborator

@emillon emillon commented Sep 8, 2022

The test suite does not behave correctly when DUNE_CACHE=enabled is present in the environment. So we set it to disabled locally or unset it in some cases.

@emillon
Copy link
Collaborator Author

emillon commented Sep 8, 2022

This is a prereq for #5903

@emillon emillon requested a review from rgrinberg September 8, 2022 10:22
@Alizter
Copy link
Collaborator

Alizter commented Sep 8, 2022

What exactly isn't behaving in the test suite? Is this perhaps a bug with cache/cram?

@emillon
Copy link
Collaborator Author

emillon commented Sep 9, 2022

I think it's inherent to the way the test suite is written. For example, some commands are run with --display verbose. Cached commands won't appear there if the cache is hot. So disabling the cache ensures we're reproducible. We could try to selectively disable it only for tests that require it I suppose.

@rgrinberg
Copy link
Member

We could try to selectively disable it only for tests that require it I suppose.

I think that's probably the way to go. Almost all tests that rely on --display .. are suspect and we should tweak them not to rely on this output. Running the tests is also super slow so taking away the cache takes the only tool that makes those bearable.

The test suite does not behave correctly when `DUNE_CACHE=enabled` is
present in the environment. So we unset the variable locally for tests
that do not pass.

This can be tricky to test. One way that works is by exporting
`DUNE_CACHE=enabled` and running `./dune.exe runtest -f`.

Signed-off-by: Etienne Millon <me@emillon.org>
@emillon emillon force-pushed the disable-dune-cache-in-testsuite branch from d565e8b to ccee9c0 Compare September 12, 2022 13:33
@emillon
Copy link
Collaborator Author

emillon commented Sep 12, 2022

I tried to take that approach. The issue is that many test cases have issues. One common pattern is that tests that rely on echo fail when the cache is used (the rule does not fire so nothing is printed).

I don't think that this is a good direction to take:

  • relying on the dune cache sounds like a good idea, and it indeed makes the test suite pretty fast, but the dune binary (under development) is part of the cache key so these cached entries are not going to be reused a lot.
  • that there's no easy way to determine if the annotation is required (at least until we enable the cache in CI)
  • more importantly there's no way to determine if the annotation becomes useless (if the test gets fixed). So these annotations are likely to stay there for a long time.

My suggestion is to proceed as follows:

  • for this PR, we just fix the metadata to reflect the status of the test suite. it doesn't work with the dune cache enabled. So we ensure it's disabled. this is a strict improvement.
  • enabling the dune cache should still improve the build times. this will help the opam deps, especially with coq 8.17 (I should try whether this works on 8.16 with an overridden opam file)
  • in the medium term, I think we can do a lot better than GHA or nix without even relying on the dune cache. a custom ocamlci-like pipeline with windows and macos workers is possible and can be provided by @tarides. let's discuss that at a next meeting.

@Alizter
Copy link
Collaborator

Alizter commented Sep 12, 2022

Nix will help with the build dependencies but only the Dune cache can improve the test suite times.

@Blaisorblade
Copy link
Contributor

I also had testsuite failures due to DUNE_CACHE, but unset DUNE_CACHE was enough even when keeping the cache enabled in ~/.config/dune/config; then a zero run of time CI=true make test takes 1.5s. (For some reason, it seems two runs are needed to get to this state, instead of 1).

@@ -1,3 +1,5 @@
$ export DUNE_CACHE=disabled

Copy link
Contributor

Choose a reason for hiding this comment

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

Why's this necessary (here and elsewhere)? Unsetting DUNE_CACHE seems enough here: time CI=true ./dune.exe runtest otherlibs/action-plugin/test/depends-on-directory-with-glob...

@Alizter
Copy link
Collaborator

Alizter commented Oct 24, 2022

What if we could do this from the cram stanzas?

@rgrinberg rgrinberg closed this Feb 28, 2023
@emillon emillon deleted the disable-dune-cache-in-testsuite branch July 25, 2023 09:17
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.

4 participants