I don't understand why this change is necessary, If TERM=dumb was set as with the previous Makefile, there should be no regression in the first place, so this PR shouldn't be necessary, Is TERM=dumb not set? Is it set somewhere but ineffective?

@sliquister, how can I reproduce the issues you observed with color outputs?

Could someone explain or point to an explanation of the issue in question? It sounds like it would be better to fix the root cause rather than change the code so that the problem is not triggered.

Personally, in general I think we should not be reducing the use of color, but rather increasing it, since it can be quite a useful aid to readability.

It's just that this PR makes possible something that was not really possible before, i.e. running expect tests manually.

It was already possible to go in a specific test directory and run make, which I suppose is the workflow @sliquister tried to imitate, and raised the issue?

I have a preference for using TERM=dumb rather than setting Clflags because (1) it more closely mirrors the way the previous code worked, and thus reduces the chances of regressions and (2) the TERM setting affect not only colors but also, for example, location printing in Toploop.

Note that ocamltest could set this value in the environment globally for its invocation (this is what I had understood from your answer) instead of just locally for the expect_test invocation -- because it may be useful for other kinds of toplevel-based tests as well.

@nojb: various parts of the compiler that are exercised by the tests depend on whether they have a "rich terminal" attached or not. When the tests are run in batch mode, the answer is always "no", but when the tests are run on a case-by-case basis, it can happen that you are in a setting where the answer is "yes", and then the output may differ from the reference file because of that -- because the toplevel tried to use curses-style location printing, or because color escape codes end up in the output, etc.

Right now we don't have tests in the compiler for color in the compiler output. If/when we decide to add some, we would have to carefully understand how to test them and when to skip them, and they would be explicitly excluded from the general principles above.

I haven't looked at the details, but generally colors are disabled when the output device is not a terminal, in particular when outputting to a .results file; this logic is independent of the setting of the TERM variable.

Which brings us back to the original question: how can we reproduce the regression that @sliquister observed?

I would propose to wait until @sliquister lets us know how to reproduce the failure he found, because that may suggest at which level it is best to have a fix.

Yeah, I was running ocamltest directly, without Makefile. I don't see how else to run a single test. There's this one phony target in testsuite, but I think that's just there to confuse the enemy.

@sliquister I cannot reproduce the issue on my machine, in the current trunk, picking a random test directory that contains expect tests.

[gasche@zigomar typing-misc (trunk)]$ pwd
/home/gasche/Prog/ocaml/github-trunk/testsuite/tests/typing-misc
[gasche@zigomar typing-misc (trunk)]$ ../../../ocamltest/ocamltest.opt *.ml
 ... testing 'constraints.ml' with 1 (expect) => passed
 ... testing 'labels.ml' with 1 (expect) => passed
 ... testing 'occur_check.ml' with 1 (expect) => passed
 ... testing 'polyvars.ml' with 1 (expect) => passed
 ... testing 'pr6939-flat-float-array.ml' with 1 (flat-float-array) => passed
 ... testing 'pr6939-flat-float-array.ml' with 1.1 (expect) => passed
 ... testing 'pr6939-no-flat-float-array.ml' with 1 (no-flat-float-array) => skipped
 ... testing 'pr6939-no-flat-float-array.ml' with 1.1 (expect) => skipped
 ... testing 'pr7103.ml' with 1 (expect) => passed
 ... testing 'pr7228.ml' with 1 (expect) => passed
 ... testing 'pr7668_bad.ml' with 1 (expect) => passed
 ... testing 'printing.ml' with 1 (expect) => passed
 ... testing 'records.ml' with 1 (expect) => passed
 ... testing 'variant.ml' with 1 (expect) => passed
 ... testing 'wellfounded.ml' with 1 (expect) => passed
[gasche@zigomar typing-misc (trunk)]$ echo $TERM
xterm-256color

And to clarify a bit: I think it only affects certain tests because ocaml-expect has its own printing function for positions, so the problem shows up with error printing functions that print locations, which happens in includemod.ml and perhaps in very few other places.

I think that it is just a matter of calling Misc.Color.setup !Clflags.color at the beginning of Expect_test.Compiler_messages.print_loc, like it is done in Location.default_printer. This will deactivate colors if it sees stderr is redirected.

@nojb: Yes, that sounds right.

(Edit: what follows is not right, see next message.)

I checked: this error is not, as we first thought, a regression due to @shindere's work on migrating the expect tests to ocamltest. I can reproduce it also when using the old Makefile harness on top of the current trunk. I think that the regression is due to @Octachron's a035ee9 , which must have changed some of the ways expect_tests interacts with OCaml flag settings.

Scrap that: I my message above is wrong because I tested the things the wrong way. With more testing, here is what I observe:

• running the typing-modules test through the root testsuite makefile works correctly
  (feature request: being able to run the tests from a single directory, instead of having to run all new tests)

cd testsuite
make new

• running ocamltest directly from the directory fails as @sliquister reports

cd testsuite/tests/typing-modules
../../../ocamltest/ocamltest.opt applicative_functor_type.ml

• if I apply the change that @nojb suggests and I pass TERM=dumb, then the failure goes away

cd testsuite/tests/typing-modules
TERM=dumb ../../../ocamltest/ocamltest.opt applicative_functor_type.ml

Surprisingly, if my testing is correct, then removing the TERM=dumb setting in the new target of the makefile does not make the test fail (while removing the TERM=dumb setting when running ocamltest directly fails):

@nojb: thinking about this more (sorry for the back-and-forth), I don't think that setup_colors will change anything. We have a problem because the compilers calls a color-aware location printer during the test, in an improperly initialized environment. All color-aware location printers are already protected by setup_colors calls, so I think that the problem is that the environment at the moment this happens is not the one we want.

Now that I can reproduce the issue, I still think that we should pass TERM=dumb to the expect_test invocation ( @shindere: do you know how to do this? ) because this affects not only colors, but I also think that it is safe and correct to forcefully set Cflags.color in expect_test. But this should be done in let main (), rather than let (), so that it goes after command-line parsing.

I apologize for the mess I produced this morning.

I investigated the issue in more details now and I understand what is happening better. Here is the set of reasons that explain why we get color escape codes when running sliquister's new test:

• In the eval_expect_file function, expect_test calls Location.report_error ppf exn, where ppf is a temporary string buffer, when running test raised an exception (which may or may not be the expected output)
• this function calls Misc.Color.setup !Clflags.color, and at this point !Clflags.color is None: it has not been initialized, and at this point Misc.Color.should_enable_color () returns true, because:
• should_enable_color only returns false when TERM is the empty string or "dumb", or when stderr is not a TTY (note that this shouldn't matter in our case as we are printing to a string buffer in ppf, not stderr, but that's how the test is defined; there is a hard-to-fix design bug here)
• if I am not mistaken (@shindere, can you confirm?) ocamltest redirects neither the standard nor the error output of expect_test to a file, so when ocamltest is invoked directly from a rich user terminal isatty(stderr) is true

Now, to your excellent questions/remarks:

As I already said, if expect needs a precise configuration, I really believe it should set things up as it needs them to be, rather than relying on the external world to do so, especially if configuring things the required way can be achieved.

From the point of view of an OCaml program, I don't think that it is safe to reason on when the should_enable_color (or other environment-related heuristics) will run. This may very well be tested whenever the runtime is initialized, and by the time user code starts running it is too late to set that environment. Now, expect_test may have been written with two executables, the first that set the environment, and then invoke the seconds in a controlled environment. If it proves too hard to tell ocamltest to pass TERM=dumb, we may still want to do this now.

because [TERM] affects not only colors,
Just to improve my understanding: wuld you be able to elaborate on this
and explain precisely what exactly this affects?

You can use git grep -C20 "\"TERM\"" **/*.ml to get an exhaustive list. It is used in Misc.setup_colors, but also in Location (to decide whether to print locations "smartly" or not in the toplevel) and in Terminfo (to decide whether the terminal is "good" or not).

My intuition is that, if the configuration is done before command-line
parsing, then it makes it possible to override it by means of
command-line options, if that becomes necessary, one day.

Yes, you are completely correct and my intuition was dead wrong. In fact, "one day" may come close as @nojb (I think?) is already questioning how we test color output -- answer: we don't. Testing color output can now be done "easily" by just passing -color always as an expect_test command-line flag, and we could think of writing such tests now. (It is a coincidence that this has been enabled just a few days ago by @Octachron's work on passing options to expect_test.)

.

All in all, I am still of the opinion that setting TERM=dumb from ocamltest (or in a wrapper program that controls the expect_test invocation and is called by ocamltest) is the better fix, and that the proposed fix is incomplete and may not be what we want (I think that keeping the expect_test semantics close to what other OCaml drivers do would be wise, to not be surprised later by differences in behavior).

In my understanding, the difficulties we encounter while trying to understand the real problem are precisely coming form the use of TERM. Here, the Makefiles were setting it while ocamltest does not and that made the situation even more difficult to understand, because somehow it introduced a discrepancy.

Well, the discrepancy comes from the fact that:

• With the new ocamltest workflow, way to run tests manually from a test directory is to invoke ocamltest manually, while make previously worked; so this is what @sliquister did while rebasing his patches

• I failed to insist that ocamltest really did need to set TERM=dumb as the original Makefile.expect carefully does when I reviewed your PR. If ocamltest is to be used as the driver of tests (today, from individual test directories, and at some point in the future for the whole testsuite) then it should really set the environment just as the Makefiles do.

what you are asking would introduce exactly the same kind of discrepancy, this time between calling expect from ocamltest (thus with TERM set to the expected value) and calling expect from the command line

As far as I know, no one used to run expect_test from the command-line. Maybe that is a need that you had while migrating to ocamltest (makes sense) and that you will have again in the future if you to to integrate it closer. It seems reasonable for you to make your decisions in that case.

If you believe that it is important to set Clflags.color := Some Never in the let () part of expect_test.ml, please feel welcome to do so. I think that you should also set TERM=dumb from ocamltest in addition to that. I don't know how to do it in ocamltest, so I won't do it myself.

I am also worried that I am going above the time budget that it is reasonable to spend over how expect_test and colors work together, so I would be happy to close this issue without much further effort.

@shindere I tried this pull request and it fixes the test breakage because of colors.

Now, if making expect-test not rely on the environment is the right solution, then by the same logic, the TERM=dumb around the call ocamltest in testsuite/Makefile should be moved in ocamltest itself.
Of course if we do that, the initial problem is fixed without needing to touch expect-tests.

So maybe we just need both changes, one to make ocamltest work the same with or without a Makefile (my problem), this one to make expect-test work the same regardless of whether it's called directly or by ocamltest.

I'm fine with merging this in addition to #1532, or closing it if you decide to not do both. Your decision!