Skip to content

Only forward color flag to worker if supplied by user #1401

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion lib/fork.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ const path = require('path');
const fs = require('fs');
const Promise = require('bluebird');
const debug = require('debug')('ava');
const hasFlag = require('has-flag');
const AvaError = require('./ava-error');

if (fs.realpathSync(__filename) !== __filename) {
Expand Down Expand Up @@ -36,7 +37,15 @@ module.exports = (file, opts, execArgv) => {
} : false
}, opts);

const args = [JSON.stringify(opts), opts.color ? '--color' : '--no-color'];
const args = [JSON.stringify(opts)];

if (hasFlag('--color')) {
args.push(`--color=${opts.color}`);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this forward --no-color if specified? And forward --color exactly as it was received?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To forward them exactly like they were received requires some sort of parsing. We can forward --no-color instead of --color=false if it was specified though.

}

if (hasFlag('--no-color')) {
args.push(`--no-color`);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although XO doesn't complain, I'd have expected a regular string here, not a templated one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, escaped my eyes.

}

const ps = childProcess.fork(path.join(__dirname, 'test-worker.js'), args, {
cwd: opts.projectDir,
Expand Down
3 changes: 3 additions & 0 deletions lib/process-adapter.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ exports.ipcChannel = process.channel || process._channel;
const opts = JSON.parse(process.argv[2]);
exports.opts = opts;

// Remove internal `ava` options from arguments
process.argv.splice(2, 1);

// Fake TTY support
if (opts.tty) {
process.stdout.isTTY = true;
Expand Down
5 changes: 4 additions & 1 deletion lib/test-worker.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
'use strict';
const chalk = require('chalk');

// Check if the test is being run without AVA cli
{
const path = require('path');
const chalk = require('chalk');

const isForked = typeof process.send === 'function';
if (!isForked) {
Expand All @@ -27,6 +27,9 @@ globals.options = opts;

const serializeError = require('./serialize-error');

// Initialize color support
chalk.enabled = opts.color;

(opts.require || []).forEach(require);

adapter.installSourceMapSupport();
Expand Down
14 changes: 14 additions & 0 deletions test/cli.js
Original file line number Diff line number Diff line change
Expand Up @@ -802,3 +802,17 @@ test('--color enables formatting colors', t => {
t.end();
});
});

test('--no-color forwards flag to worker', t => {
execCli(['--no-color', 'no-color-flag.js'], {dirname: 'fixture'}, err => {
t.ok(err);
t.end();
});
});

test('--color forwards flag to worker', t => {
execCli(['--color', 'color-flag.js'], {dirname: 'fixture'}, err => {
t.ok(err);
t.end();
});
});
5 changes: 5 additions & 0 deletions test/fixture/arguments.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import test from '../../';

test(t => {
t.is(process.argv[2], undefined);
});
5 changes: 5 additions & 0 deletions test/fixture/color-flag.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import test from '../../';

test(t => {
t.is(process.argv[2], '--color');
});
5 changes: 5 additions & 0 deletions test/fixture/no-color-flag.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import test from '../../';

test(t => {
t.is(process.argv[2], '--no-color');
});
12 changes: 10 additions & 2 deletions test/fork.js
Original file line number Diff line number Diff line change
Expand Up @@ -140,8 +140,7 @@ test('color support is initialized correctly', t => {

return Promise.all([
fork(fixture('chalk-enabled.js'), {color: true}).run({}),
fork(fixture('chalk-disabled.js'), {color: false}).run({}),
fork(fixture('chalk-disabled.js'), {}).run({})
fork(fixture('chalk-disabled.js'), {color: false}).run({})
]).then(info => {
info.forEach(info => {
if (info.stats.failCount > 0) {
Expand All @@ -151,3 +150,12 @@ test('color support is initialized correctly', t => {
t.is(info.length, 3);
});
});

test('doesn\'t pass internal options to worker', t => {
return fork(fixture('arguments.js'))
.run({})
.then(info => {
t.is(info.stats.passCount, 1);
t.end();
});
});