Removed MOCHA_COLORS environment variable

[waseemahmad31]

Description of the Change

Remove MOCHA_COLORS environment variable

Why should this be in core?

The supports-color module's environment variable FORCE_COLOR provides the same functionality.

Benefits

Redundant. One less thing to maintain.

Applicable issues

Fixes #3641
patch release

[jsf-clabot]

All committers have signed the CLA.

[coveralls]

Coverage decreased (-0.09%) to 90.682% when pulling 33f2dae on waseemahmad31:issue/3641 into 9d62030 on mochajs:master.

[boneskull]

LGTM, thanks.

[boneskull]

Wait. I don't think we should merge this.

This is why.

Can we instead detect the use of MOCHA_COLORS and print a deprecation notice and migration info? This will hopefully avoid a torrent of issues.

[boneskull]

please see my comment about deprecation

[boneskull]

AFAICT, "migration" looks like replacing use of MOCHA_COLORS with FORCE_COLOR.

Here's an example of FORCE_COLOR=1 set at the global level.

For whatever reason we never bothered to do this in our own CI, but there you go.

[boneskull]

(The reason why we never bothered to do this is that it interferes w/ our integration tests, apparently.)

[boneskull]

We could even go ahead and print the deprecation notice, then remove MOCHA_COLORS from the environment and set FORCE_COLOR=1... too invasive?

[waseemahmad31]

I think it will be good if we show the deprecation message and then remove MOCHA_COLORS from the environment and set FORCE_COLOR=1.

[plroebuck]

We could even go ahead and print the deprecation notice, then remove MOCHA_COLORS from the environment and set FORCE_COLOR=1... too invasive?

Far less invasive would be to check for process.env.MOCHA_COLORS at startup (i.e., Mocha-5.2 "/bin/_mocha"). If it is defined, display deprecation notice and append --colors to user-provided Mocha cmdline arguments. FORCE_COLOR is used by many packages; we treat color support as a boolean state, but others may process differently depending on the number of colors available.

[plroebuck]

As we've had an issue with environment come up twice within a year, also think we should document our officially supported environment. Also provide notice that any envvar not officially documented should be considered an implementation detail and subject to change without warning.

[plroebuck]

Uh, you already removed it...

  utils.deprecate(
    'Support for deprecated "MOCHA_COLORS" environment variable was removed from Mocha. Use "FORCE_COLOR" instead.');

[plroebuck]

Environment variables should always be strings...

[plroebuck]

But this is a rather heavy-handed approach...

[plroebuck]

Think all deprecation-related code belongs here instead, prior to yargs processing, so --colors can be handled upfront.

[waseemahmad31]

Thanks, @plroebuck for reviewing the changes. Also If I conclude from your review comments,

I shoud write the below code in cli.js just before the yargs processing, to show the deprecation message and handle --colors when the MOCHA_COLORS environment variable is being used instead of handling it in the base.js.

  if (process.env.MOCHA_COLORS) {
    utils.deprecate(
      'Support for deprecated "MOCHA_COLORS" environment variable was removed from Mocha. Use "FORCE_COLOR" instead.');
    if (argv.indexOf('--colors') < 0 && argv.indexOf('-c') < 0) {
      argv.push('--colors');
    }
    delete process.env.MOCHA_COLORS;
  }

Let me know, if anything needs to change

Approved version prior to boneskull's request for deprecation notice

[waseemahmad31]

Added code to show deprication message of MOCHA_COLORS and handled --colors argv in the cli.js.

[boneskull]

@plroebuck

Far less invasive would be to check for process.env.MOCHA_COLORS at startup (i.e., Mocha-5.2 "/bin/_mocha"). If it is defined, display deprecation notice and append --colors to user-provided Mocha cmdline arguments. FORCE_COLOR is used by many packages; we treat color support as a boolean state, but others may process differently depending on the number of colors available.

I don't think --colors is equivalent to FORCE_COLOR (it still defers the final decision to supports-color, which considers FORCE_COLOR), but I'll need to confirm that. It would be great if @waseemahmad31 could confirm, actually.

UPDATE: I can confirm that --colors works even if TERM=dumb.

[waseemahmad31]

yes, @boneskull. --colors do the same work as FORCE_COLOR = true, FORCE_COLOR = 1 or FORCE_COLOR = []

[boneskull]

OK, I don't think we should do this at all actually.

I've spent another 30m or so looking at this, and it's a rabbit hole.

1. We shouldn't append to process.argv to do this, as this would potentially conflict with --no-colors, --no-color or -C. We could special-case it, but that's just replacing code rot with tech debt.
2. We need to decide the priority environment flags take. I believe environment flags should be secondary only to command-line arguments, meaning they should override configuration files ({"color": false}) and mocha.opts.
3. We should support some form of environment flags; Lerna monorepos in particular seem to want to use this behavior to avoid maintaining the same settings across packages.
4. We can support environment flags "easily" (I said that before, of course) because yargs supports it; we haven't enabled it. I do want to enable it, but maybe in a near-future minor release. This takes all our options and allows them to be defined via environment flags having a prefix, e.g., MOCHA_BAIL=1 would be equivalent to --bail.
5. If we enable environment flag support via yargs, MOCHA_COLORS=1 is no longer actually deprecated; it's a valid configuration option, equivalent to --colors.

[boneskull]

@waseemahmad31 I apologize for the confusion here.

[plroebuck]

1. We shouldn't append to `process.argv` to do this, as this would potentially conflict with `--no-colors`, `--no-color` or `-C`.  We could special-case it, but that's just replacing code rot with tech debt.

2. We need to decide the priority environment flags take.  I believe environment flags should be secondary only to command-line arguments, meaning they should override configuration files (`{"color": false}`) and `mocha.opts`.

Mea culpa. I wrote "append", but meant "prepend" --no-colors, which requires no checking of existing argv and covers the argument processing order issue.

3. We _should_ support some form of environment flags; Lerna monorepos in particular seem to want to use this behavior to avoid maintaining the same settings across packages.
4. We _can_ support environment flags "easily" (I said that before, of course) because [yargs supports it](http://yargs.js.org/docs/#api-envprefix); we haven't enabled it.  I _do_ want to enable it, but maybe in a near-future minor release.  This takes all our options and allows them to be defined via environment flags having a prefix, e.g., `MOCHA_BAIL=1` would be equivalent to `--bail`.
5. If we enable environment flag support via yargs, **`MOCHA_COLORS=1` is no longer actually deprecated**; it's a valid configuration option, equivalent to `--colors`.

Potential future maintenance nightmare, given the effort to remove this single redundant envvar.

[boneskull]

It's not usually trivial to remove things from Mocha, no.

@waseemahmad31 Thanks for your effort on this

[waseemahmad31]

@boneskull, @plroebuck thanks for the feedback. Initially, it seems like the easy one to start the first contribution. Anyway thanks for the detailed analysis of the impact of this change.