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

Allow unknown CLI options #877

Closed
wants to merge 1 commit into from
Closed

Allow unknown CLI options #877

wants to merge 1 commit into from

Conversation

kozhevnikov
Copy link
Member

Commander by default throws an error when unexpected command-line option is present which is a problem when you try to pass additional arguments to the tests without specifying a 'command' but 'options' only, i.e. when using a profile.

For example the following works fine with test/foo and additional --debug:

node node_modules/cucumber/bin/cucumber.js test/foo --require test/foo --debug

However the following throws an error error: unknown option '--debug' because test/foo directory is in profile definition:

node node_modules/cucumber/bin/cucumber.js --profile foo --debug

The PR is to prevent Commander throwing exceptions when unexpected option is present and pass it verbatim to the tests.

@charlierudolph
Copy link
Member

I consider this a bug in commander that that the first one works. --debug is an unkown option and should always be thrown. It appears in the code that it is only thrown if there are no arguments.

I don't want to allow unknown CLI options as then typos pass through or you may think cucumber-js supports some flag that it doesn't.

What are you trying to pass --debug to?

@charlierudolph
Copy link
Member

Related: tj/commander.js#561

@kozhevnikov
Copy link
Member Author

kozhevnikov commented Jul 12, 2017

To the test framework itself, i.e. to be parsed further by configuration libraries. Typically tools (e.g. Yarn) would have -- escape to ignore the rest of the arguments so the command would become --profile foo -- --debug but it throws ENOENT in this case.

@charlierudolph
Copy link
Member

To the test framework itself, i.e. to be parsed further by configuration libraries

Like what? Cucumber shares very little with yarn and doesn't have a feature for being able to run other scripts and pass arguments for them to parse. It is a standalone test framework and other libraries can integrate with it / wrap it. Also I believe -- means everything following is an argument (and thus option parsing should not be attempted). If you want to pass something to another library we need to build in a dedicated feature for that.

I'm going to close this PR as I don't want to allow unknown options and will see if I can get the bug fixed on commander. Please create a separate issue focusing on your use case.

@kozhevnikov
Copy link
Member Author

kozhevnikov commented Jul 12, 2017

Yarn was only meant as as example of how other tools allow for additional arguments downstream without disabling options error checking, i.e. in this case it would mean passing argv.splice(0, argv.indexOf('--')) to Commander instead of the whole argv if -- is present.

@charlierudolph
Copy link
Member

What tool are you using? I've never heard of running cucumber-js from the CLI and wanting to let some command line arguments be parsed only by another library. I think there is a better way to do this and am happy to explore this with you but I want to focus on the problem and desired behavior and not the solution in determining the best approach.

@kozhevnikov
Copy link
Member Author

Configuration libraries such as nconf/convict/etc allow values to be set/overwritten at runtime through either environment variables or command-line arguments. Having two different ways to manage env vars on Windows and *nix (and Windows doesn't have DEBUG=true node ... prefix requiring N+1 chained commands) is not ideal and an annoyance while spawning separate processes for parallelisation, so having a simple --debug or other flags added to a command seemed like a very elegant solution. Given the first example worked out the box I figured it was a feature, like the aforementioned yarn but without needing the -- separator. I'll give env vars another go and see it I can make them a bit more user friendly, thanks.

BTW, when you said "[...] never heard of running cucumber-js from the CLI [...]", what other ways are there?

@charlierudolph
Copy link
Member

BTW, when you said "[...] never heard of running cucumber-js from the CLI [...]", what other ways are there?

The second half of my sentence ("wanting to let some command line arguments be parsed only by another library") is the part I've never heard of

@charlierudolph
Copy link
Member

We introduced the --world-parameters CLI argument to help with the fact that environment variables are difficult on windows.

@kozhevnikov
Copy link
Member Author

kozhevnikov commented Jul 12, 2017

That doesn't really help with configs outside world, i.e. where test framework is abstracted away from tests themselves (and therefor Cucumber runtime) and use an appropriate config library instead. Cucumber is great at what it does and doesn't try to be the centre of the test framework universe like Protractor or WebDriverIO that follow Zawinski's law.

@charlierudolph
Copy link
Member

I still don't understand the use case. When are these other libraries being loaded and why are you loading these other libraries? Due they have a node API that could be used to configure them?

@lock
Copy link

lock bot commented Oct 24, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Oct 24, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants