Use coloured output by default when running INSIDE_DUNE

[craigfe]

This changes the default value of the --color flag to always rather than auto when the INSIDE_DUNE variable is set.

As discussed before (see #207 and ocaml/dune#145), Alcotest output is not coloured by default if run via dune runtest, since Dune's buffering mechanism keeps the test process from having direct access to the terminal.

We now have the ALCOTEST_COLOR variable for experienced users to override this behaviour, but I suspect the better solution is to default to always showing colours, so that new users are not surprised by this interaction. People who really don't want colours can still disable it with the environment variable or the command-line option.

Travis CI appears to filter out any ANSII colour codes in its logs (see https://travis-ci.org/github/mirage/irmin/jobs/686951739#L3684).

[craigfe]

Appveyor handles this change just fine:

as does Travis:

as does OCaml-CI:

(well, no worse than usual there...)

[talex5]

The output in ocaml-ci is a bit hard to read, but I guess this was the error:

2.894 test_color test/e2e/alcotest/inside-dune/color-default.actual (exit 125)

(https://ci.ocamllabs.io:8100/job/2020-05-17/173931-ci-build-d08ed7)

I rebuilt the jobs and they passed the second time :-/

[craigfe]

The errors on 4.03 and 4.04 seem repeatable at least. I can reproduce the diff on my machine. Here's an excerpt:

ᐅ diff <(xxd color-default.4.08) <(xxd color-default.4.03)

-00000000: 5465 7374 696e 6720 1b5b 316d 7465 7374  Testing .[1mtest
+00000000: 5465 7374 696e 6720 1b5b 3031 6d74 6573  Testing .[01mtes
-00000010: 5f63 6f6c 6f72 1b5b 306d 2e0a 5468 6973  _color.[0m..This
+00000010: 745f 636f 6c6f 721b 5b6d 2e0a 5468 6973  t_color.[m..This

So, the escape codes look to be the same modulo differences in ANSI escape resetting that have no bearing on the rendered output.

Sure enough, Fmt.0.8.7 (which dropped support for 4.03.0 and 4.04.0) contains a change that breaks compatibility of ANSI escapes: dbuenzli/fmt@594a1d7 (sadly not highlighted in the CHANGES).

I rebuilt the jobs and they passed the second time :-/

@talex5: No idea what's causing the flaky failures post 4.04.0 though; ran the tests 100 times or so on my machine and got only passes. OCaml-CI is caching a bit too aggressively for me to prod it further: shouldn't clicking rebuild invalidate the cache layer for the dune build @runtest stage?

[talex5]

OCaml-CI is caching a bit too aggressively for me to prod it further: shouldn't clicking rebuild invalidate the cache layer for the dune build runtest stage?

Good idea - I've filed an issue here: ocurrent/ocaml-ci#186

[craigfe]

I would suggest just dropping this test, but I have future improvements in mind that require heavier usage of the styled formatter (including styles that I now see weren't introduced until 0.8.7).

I think the options here are:

• drop support for 4.03 and 4.04;
• vendor the style formatters (or all of fmt);
• use a different formatting dependency.

Does anyone have thoughts on this? (@samoht, @avsm)

[samoht]

It's fine to vendor fmt (as long as there is no name clashes when people will try to link them to their project).

[craigfe]

I think I've tracked down all of the non-deterministic failures now. (At least, I've killed all the ones that I can reproduce on my machine...) Our handling of symlinks in alcotest.ml has race-conditions. When running the test suite, each case contends for the latest symlink, exposing those TOCTOU errors. I have a diff to fix that; will PR separately on Friday.

Our use of Cmdliner throws away helpful information that we should probably be logging somewhere (an `Exn being returned is almost always an internal error). I'll make a PR to fix that too.

[craigfe]

Now depends on #245.

[craigfe]

This can now be considered for merge.

Vendoring is slightly non-trivial as we export the Fmt.t type in our interface. It's possible to inline the definition of that type everywhere that it occurs, but I suspect it's more bother than it's worth. Happy to adjust if others disagree.

[hannesm]

I'm a bit sceptical of this change: it hardcodes behaviour of the output depending on other environment variables (INSIDE_DUNE), which makes it not very obvious (to me) when colours are used.

At least, the behaviour of coloured output should be documented in the documentation (and how it behaves depending on the environment variables and command-line arguments, esp. what the order of precedence is).

[craigfe]

I'm a bit sceptical of this change: it hardcodes behaviour of the output depending on other environment variables (INSIDE_DUNE)

"hardcodes" seems a bit harsh to me. It changes the default behaviour, but never overrides behaviour requested explicitly by the user. This can be done in 4 different ways:

1. --color command line option,
2. ALCOTEST_COLOR environment variable,
3. runtime ?argv option,
4. Fmt.setup_std_outputs.

At least, the behaviour of coloured output should be documented in the documentation (and how it behaves depending on the environment variables and command-line arguments, esp. what the order of precedence is).

The man-page output is now:

       --color=WHEN (absent ALCOTEST_COLOR env)
           Colorize the output. WHEN must be one of `auto', `always' or
           `never'. Defaults to `always' when running inside Dune, otherwise
           defaults to `auto'.

This to me suggests the priority ordering --color > ALCOTEST_COLOR > [default as specified], but it's not very explicit. What do you think?

If by 'documentation' you mean the Odoc output, our explanation of the command-line arguments (and their run-time equivalents) is currently very lacking and could certainly do with some general improvement. (The priority ordering is also fairly untested.)

[hannesm]

oh sorry I missed the man page update, thanks for doing that.

[craigfe]

Rebased. Will merge this tomorrow if there are no outstanding comments / objections.