Skip to content

Pass --no-color flag down to the forked process - fixes #843 #1104

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 1 commit into from
Closed

Pass --no-color flag down to the forked process - fixes #843 #1104

wants to merge 1 commit into from

Conversation

thinkimlazy
Copy link

Sorry that I accidentally deleted last one.

Also I want to notice that just chalk.enabled = true not working, because functions from lib/colors.js has own enabled property. So we need that loop

Object.keys(colors).forEach(function (key) {
	colors[key].enabled = true;
});

@@ -6,7 +6,6 @@ var test = require('tap').test;
global.Promise = require('bluebird');
var getStream = require('get-stream');
var figures = require('figures');
var arrify = require('arrify');
Copy link
Author

Choose a reason for hiding this comment

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

We don't need arrify because Array.prototype.concat already can concat single values
Array.prototype.concat docs

Object.keys(colors).forEach(function (key) {
colors[key].enabled = true;
});

Copy link
Author

@thinkimlazy thinkimlazy Nov 5, 2016

Choose a reason for hiding this comment

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

This turns on colors no matter what.

Object.keys(colors).forEach(function (key) {
colors[key].enabled = true;
});

Copy link
Author

@thinkimlazy thinkimlazy Nov 5, 2016

Choose a reason for hiding this comment

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

We need to turn on colors no matter what while testing. Also in case that Istanbul turning them off at this condition

@@ -43,7 +46,7 @@ function execCli(args, opts, cb) {
var stderr;

var processPromise = new Promise(function (resolve) {
child = childProcess.spawn(process.execPath, [path.relative(dirname, cliPath)].concat(arrify(args)), {
child = childProcess.spawn(process.execPath, [path.relative(dirname, cliPath)].concat(args, '--color'), {
Copy link
Author

@thinkimlazy thinkimlazy Nov 5, 2016

Choose a reason for hiding this comment

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

Force to use colors while testing
And get rid of useless arrify

@thinkimlazy thinkimlazy closed this Nov 5, 2016
@thinkimlazy thinkimlazy reopened this Nov 5, 2016
@thinkimlazy thinkimlazy closed this Nov 5, 2016
@thinkimlazy thinkimlazy deleted the no-color branch November 5, 2016 11:50
@thinkimlazy thinkimlazy restored the no-color branch November 5, 2016 11:52
@thinkimlazy thinkimlazy reopened this Nov 5, 2016
@sindresorhus sindresorhus changed the title Pass --no-color flag down to the forked process fixes #843 Pass --no-color flag down to the forked process - fixes #843 Nov 8, 2016
@sindresorhus
Copy link
Member

It also needs to pass the --no-color flag down to the forked process. If I add console.log(require('chalk').red('colorzzzz')); in a test file, it will still log colors even with $ ava --no-colors. I would expect that to prevent all colors.

@thinkimlazy
Copy link
Author

thinkimlazy commented Nov 17, 2016

Colors in test files disabled by default. Without flag

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

test('no colors here', t => {
    t.false(chalk.enabled); // passes
})

@sindresorhus
Copy link
Member

Not for me:

❯ ava

  1 failed

  no colors here

  t.false(chalk.enabled)
                |
                true

    Test.fn (test.js:5:5)
    internal/child_process.js:721:12

@vadimdemedes
Copy link
Contributor

Hey @thinkimlazy, happy holidays! Will you have some free time soon to finish this up?

@thinkimlazy
Copy link
Author

@vadimdemedes Hello, I'm quite busy right now so I'm not sure that I can fix this any time soon.

@sindresorhus
Copy link
Member

Closing for lack of activity and to free it up for someone else interested in working on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants