Skip to content
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

Remove custom CLI flags in child worker before requiring test file #1393

Closed
sindresorhus opened this issue May 24, 2017 · 8 comments
Closed

Comments

@sindresorhus
Copy link
Member

We shouldn't expose custom CLI flags to the test file. It might change the behavior of user's code.

Currently we expose --color in the test file, even though the user just ran $ ava with no flag.

I had to implement a workaround for now: https://github.com/sindresorhus/xo-init/blob/9755f41134d0c8199674c1888d88cca3375358a3/test.js#L8

@novemberborn Thoughts?

@novemberborn
Copy link
Member

IIRC I wasn't a fan of passing --color, but you said it was convention ;-)

We also pass a JSON string as argv[2] though. I'm not sure process.argv should be relied upon inside tests.

@sindresorhus
Copy link
Member Author

IIRC I wasn't a fan of passing --color, but you said it was convention ;-)

I know. It is convention though, but turns out to be annoying in this context.

I'm not sure process.argv should be relied upon inside tests.

Oh it shouldn't, but I don't think we should clutter it either.


How about we pass whether to use colors or not in the argv[2] JSON string and handle it accordingly before requiring the test file?

@novemberborn
Copy link
Member

If you mean that for use cases under our control we use options.color, yes. It could be argued though that if the user explicitly passes --color and --no-color flags, due to the convention, we should still forward those.

@sindresorhus
Copy link
Member Author

If you mean that for use cases under our control we use options.color, yes.

Yes, I mean our auto-detection.

It could be argued though that if the user explicitly passes --color and --no-color flags, due to the convention, we should still forward those.

Agreed.

@kevva
Copy link
Contributor

kevva commented Jun 1, 2017

So from what I could gather from #1198, we pass on --color/--no-color to the worker to enable/disable for example chalk.red('unicorn') in the test files. If we skip it and passes on whatever flags the user passes on, chalk will be disabled in the test files (because it's not a TTY) unless the user explicitly runs ava with --color.

So, do we want colored text in test files to always display as colored?

@sindresorhus
Copy link
Member Author

@kevva Yeah, basically, we want to pass on the auto-detect color support to the child worker, which always detects it as no color support, without clobbering process.argv for the test file context.

@novemberborn
Copy link
Member

Yeah, basically, we want to pass on the auto-detect color support to the child worker, which always detects it as no color support

To clarify, the worker will never detect color support itself, hence us needing to forward the detected support from the main process.

(It read as ambiguous to me.)

@kevva
Copy link
Contributor

kevva commented Jun 4, 2017

Yeah, basically, we want to pass on the auto-detect color support to the child worker, which always detects it as no color support, without clobbering process.argv for the test file context.

We already do that, I think. The only function --color/--no-color currently fills is passing the auto detection into the actual tests (not the reporter) which I don't think we should do. If the user wants to log stuff in their tests using chalk, just run ava with the --color flag.

kevva added a commit that referenced this issue Jun 4, 2017
@novemberborn novemberborn added this to the 1.0 milestone Oct 25, 2017
novemberborn added a commit that referenced this issue Feb 12, 2018
This removes AVA's internal flags, set by fork.js and consumed by
test-worker.js. Fixes #1393.

AVA now forwards arguments, provided after an `--` argument terminator,
to the worker process. Arguments are available from `process.argv[2]`
onwards.
novemberborn added a commit that referenced this issue Feb 13, 2018
This removes AVA's internal flags, set by fork.js and consumed by
test-worker.js. Fixes #1393.

AVA now forwards arguments, provided after an `--` argument terminator,
to the worker process. Arguments are available from `process.argv[2]`
onwards.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants