ocamltest+testsuite: by default, only keep test data on failure

[gasche]

On my machine, running the whole testsuite generates 1.5Gio of test
data. I have 11 workdirs for the OCaml repository (versions from 4.08
to trunk, plus several experimental workdirs); at any point in time
there can be more than a dozen gibibytes of test data on my disk, and
this is too much. (I found this when I ran into space storage issue
on my development machine and looked at the culprits.)

I use the test data saved by ocamltest when a test fails, to diagnose
the failure. I don't remember ever looking at the test data of
a succesful test.

The present patchset changes ocamltest to, by default, discard the
test data of succesful tests. If one wishes to preserve the test
data on a succesful test, one should explicitly pass the
-keep-data-on-success flag. The testsuite/Makefile is then changed
to pass this flag if the user set the KEEP_DATA_ON_SUCCESS variable
to a non-empty value.

[gasche]

(cc @shindere @Armael @nojb)

[Armael]

(so shouldn't the title be "only keep test data for failing builds"?)

[gasche]

Duh. Thanks!

(cc also @dra27 who has weird testcases in mind that copy and generate data in interesting ways, and might have a different opinion on the right default.)

[shindere]

I very much like the idea, thanks a lot, @gasche.

Regarding the coding details:

• It may be better to stick to OCAMLTESTFLAGS (without an _) rather thn
  renaming to OCAMLTEST_FLAGS

• Regarding the name of the option, I'd suggest -keep-test-dir-on-success
  which seems more precise to me

• Presently, the PR somehow hardcodes the link between test result and
  summaries, e.g. a skipped test is considered not being a failure hence
  its directory would be removed, but this is something the user may want
  to control. I'm not necessarily saying the code should be further
  extended in the present PR, just thinking aloud here to collect
  others' opinions.

• In the same vein, the ocamltest option could actually be seen as a level.
  It could take the values all, skipped and failed to also cover the
  situation described above (skipped would mean to keep both skipped and
  failed tests, failed would mean to keep only the failed ones).

• Regarding KEEP_DATA_ON_SUCCESS: i'd rename to something like
  KEEP_TESTDIR_ON_SUCCESS. It would seem nice to me to include
  TEST in the variable name in case we would want to
  also make it available from the main Makefile. In such a context,
  the name would, I think, become more self-explanatory.

[gasche]

I'm not convinced by the idea of parametrization. In particular I don't see any situation where you would want to keep data for skipped tests (especially as, a priori, skipped tests did not run, so there should not be any data worth keeping). I would propose to wait until we see a clear need for it before making the interface more complex.

I will apply your renaming suggestions, expect that the variable will be KEEP_TEST_DIR_ON_SUCCESS (with an extra _) so that there is a direct mapping from the ocamltest variable for convenience/discoverability.

[gasche]

And: Thanks for the reviews!

[shindere]

Gabriel Scherer (2020/04/02 05:40 -0700):

I'm not convinced by the idea of parametrization. In particular I don't see any situation where you would want to keep data for skipped tests (especially as, a priori, skipped tests did not run, so there should not be any data worth keeping).

You are assuming that the skipped test runs first, which may indeed be the case rather often, but still not always. One may for instance compile a program as the test and then run it. If that run results in a skip, you'd want to be able to keep the program to reproduce.

I would propose to wait until we see a clear need for it before doing the interface more complex.

That's fine with me.

I will apply your renaming suggestions, expect that the variable will be `KEEP_TEST_DIR_ON_SUCCESS` (with an extra `_`) so that there is a direct mapping from the ocamltest variable for convenience/discoverability.

Fine with me, too. Just in case I don't get a notification whn you push the commits, please leave a note here when you feel ready for a new review. Best wishes and thanks again!

[gasche]

One question I think (for @shindere and @dra27) is whether we want to use the same default on the CI machines -- drop test data on successes -- or whether in that setting it is better to, when we inspect the output of a failed CI run, have all the gibibytes available. If the latter, then we want to set KEEP_..._ON_SUCCESS in the CI scripts.

[shindere]

It feels to me it would actually be very good also on the CI machines to drop the directories for successful tests. I think keeping those for failed tests is very much what we need and that this will be a big relief for the disks of our CI machines, especially at Inria, especially the windows ones.

[gasche]

I rebased the PR with the renamings.

[shindere]

Thanks. Approved. Feel free to merge as soon as CI passes.

[gasche]

Merged, thanks!

[xavierleroy]

We have new warnings reported by "make", and I think this PR is responsible for it:

Running tests from 'tests/typing-extensions' ...
Makefile:65: warning: undefined variable 'PROMOTE'
Makefile:71: warning: undefined variable 'KEEP_TEST_DIR_ON_SUCCESS'

Please add "define if not defined" assignments in the right places:

PROMOTE ?=
KEEP_TEST_DIR_ON_SUCCESS ?=

This is also the right place to add a comment explaining when we expect those variables to be set on the "make" command-line.

[gasche]

Guilty! (I don't think that we want to let users set the first variable, so I will just use =; they should use make promote to explicitly require test promotion.)

[gasche]

The parentheses above reveal that I'm still not very knowledgeable about how Make works; I would like the variable to be empty when the user invokes make (and ideally avoid pollution from the unix environment if there is a PROMOTE there), but recursive calls should use the setting passed at the recursive call-site. I'll play with an example to find out.

[gasche]

Testing indicates that setting PROMOTE= in the makefile will be overriden by explicit setting the make environment at invocation time (either by the user or recursively; so it has the correct semantics in the recursive call), but will override a setting from the unix environment (so name conflicts are avoided). This is exactly what I want for PROMOTE, whereas ?= is the right choice for KEEP_TEST_DIR_ON_SUCCESS, which we want to let the user override.

[gasche]

I went to the GNU make documentation and found out there is also override PROMOTE = foo which ignores invocation-time settings. The semantics of make variables is so exciting!

[shindere]

I think if you define the variable in all cases, maybe with the empty string, and use ?= you should be fine and getting what you are looking for.

[gasche]

Fixed in 7612a6d, thanks.