Fix issue with using 'chalk' in tests

[jhnns]

This PR fixes a bug where the chalk module reported that the current terminal does not support colors.

'chalk' is using the 'supports-color' module which reads process.stdout.isTTY as soon as it is required. Since our process-adapter is initializing a fake TTY support, we must ensure that 'supports-color' is not executed before our fake TTY support is initialized.

I just re-arranged the require() statements. Also added tests for it.

Fixes #1124

[jhnns]

Seems like supports-color reports false on Travis anyway.... do you have a hint how to solve this? I'm already mocking process.stdout.isTTY

[novemberborn]

Hi @jhnns. The code path where chalk is used only applies when you run a test file with node itself, e.g. node test.js. I think the only change that's needed is to require chalk inside the if (!isForked) { body.

That said we closed #1124 because it seemed fixed. Are you still running into this issue with the latest AVA version?

[jhnns]

I think the only change that's needed is to require chalk inside the if (!isForked) { body.

That is true, but since some of the required dependencies might also require chalk or supports-color (you never know 😉), I thought it would be better to re-arrange all require statements. But I can change that if you're not ok with that.

That said we closed #1124 because it seemed fixed. Are you still running into this issue with the latest AVA version?

My test is failing if my patch is not applied, so I assume that the issue is still there.

[novemberborn]

My test is failing if my patch is not applied, so I assume that the issue is still there.

Could you share your test? Where / how specifically is it failing?

I don't fully understand this color support stuff. I'd wait for @sindresorhus to chime in before updating this PR.

[jhnns]

Could you share your test? Where / how specifically is it failing?

It's included in the PR. If the patch is not applied, the test fails saying that chalk.enabled is false.

In order to finish this PR from my perspective I would need to fix it on Travis. But it's hard to debug it since it works locally ^^

[sindresorhus]

Seems like supports-color reports false on Travis anyway.... do you have a hint how to solve this? I'm already mocking process.stdout.isTTY

It's not enough with just process.stdout.isTTY. You also have to indicate color support, see the relevant detection code: https://github.com/chalk/supports-color/blob/8400d98ade32b2adffd50902c06d9e725a5c6588/index.js You could for example set process.env.TERM = 'xterm256';

[sindresorhus]

AVA 0.18.2:

import chalk from 'chalk';
import test from 'ava';

test(t => {
	t.true(chalk.enabled);
});

  1 passed

Seems to already work fine?

[sindresorhus]

t.true(chalk.enabled);

[jhnns]

Seems to already work fine?

Yes, I can confirm that. Since lib/fork.js creates the child process with --color or --no-color depending on the cli.flags, supports-color will report the desired value in the child process.

I've update my PR and just added tests to test/fork.js to ensure that color support is initialized correctly.

While skimming the source code, I saw that in test/cli.js there's some code that patches chalk. This was necessary because there is a test that checks for colored output while supports-color reports false inside the tap test process because it does not have TTY support. We could set the env flag FORCE_COLOR to enforce colored output, but in that case we would need to update tests in test/api.js in order to check for colored output.

So from my POV, you should decide whether you want to:

• check for colored output. In that case, we should set FORCE_COLOR and update the tests in test/api.js or
• not check for colored output. In that case, we would need to change the test in test/cli.js to set the --no-color flag and check against uncolored strings.

The current PR reflects the second choice, but I'm willing to change that since I've already invested some time in it. Just tell me what you want. If you don't care about this and you want me to leave you alone, that's also fine. Then just close this 😁

[sindresorhus]

Sorry about the super slow reply. I agree with the second option, good choice. Thank you for looking into this and your extensive detective work :)

[jhnns]

I just love ava as a test runner 👍
I'm happy to give something back 😁