Skip to content

Ensure workers inherit color level from main process #1701

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
novemberborn opened this issue Feb 12, 2018 · 6 comments
Closed

Ensure workers inherit color level from main process #1701

novemberborn opened this issue Feb 12, 2018 · 6 comments
Labels
bug current functionality does not work as desired help wanted scope:internals scope:test-environment

Comments

@novemberborn
Copy link
Member

Workers don't inherit the correct color level from the main process. @nowells raised this in #1455. This got entangled with a solution for #1393, by @kevva in #1401. I'm now fixing that with #1700.

As of #1700 workers remove AVA's internal flags from process.argv. We could apply the solution from #1455 so workers correctly inherit the color level.

I'm concerned that chalk / supports-color will behave inconsistently in the test processes. If the user code shares its chalk / supports-color dependencies with AVA, they'll inherit the color level from the main process. Otherwise the color level is detected afresh. I don't think AVA should foist the --color / --no-color flags onto the worker process. We could solve this inconsistency by bundling chalk. That way we can be sure to only configure our copy and leave user code unaffected. Users can still manipulate color levels in their test processes through environment variables or additional command line arguments.

@nowells, @kevva, @sindresorhus, what do you think?

@jamiebuilds
Copy link
Contributor

Cherry picked changes from #1455 into #1713 on latest master

@novemberborn
Copy link
Member Author

I'm concerned that chalk / supports-color will behave inconsistently in the test processes.

I got thrown off by chalk's odd constructor access pattern. The trick is for AVA to use its own chalk instance and to configure it in the main process based on the package.json options and CLI flags, and in the worker processes based on what it was told by the main process.

I'm refactoring a lot of code at the moment and Chalk's instantiation came up, so I'm tackling that part now.

Once that lands we should support the color level option in the package.json and forward it to the worker processes.

@novemberborn
Copy link
Member Author

See #1722.

@novemberborn
Copy link
Member Author

novemberborn commented Apr 2, 2018

With #1722, AVA now uses its own chalk instance. It's loaded quite early on in lib/worker/subprocess.js. Right now it processes the --color / --no-color argument passed by lib/fork.js.

  • Instead, lib/fork.js should stop passing those arguments. We remove them anyway.
  • lib/worker/subprocess.js should require lib/worker/consume-argv before lib/worker/load-chalk.
  • load-chalk.js should access the color option through requiring lib/worker/options and use it to instantiate chalk.
  • consume-argv.js should only remove process.argv[2], not also process.argv[3].

Additionally (though this could be done as a follow-up), we should expand the --color CLI flag and package.json#ava.color to support the 256 and 16m string values from support-color.

@novemberborn
Copy link
Member Author

This is ready to be worked on again, if anybody wants to pick it up 😀

@novemberborn
Copy link
Member Author

This has been addressed.

Additionally (though this could be done as a follow-up), we should expand the --color CLI flag and package.json#ava.color to support the 256 and 16m string values from support-color.

I'm not sure if this is important, so I'm not going to open this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug current functionality does not work as desired help wanted scope:internals scope:test-environment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants