fix: Add supported choices to --target option #1644
Conversation
tests/unit/test-cmd/test.run.js
Outdated
| sinon.assert.notCalled(androidRunnerStub); | ||
| sinon.assert.notCalled(desktopRunnerStub); | ||
| }); | ||
|
|
There was a problem hiding this comment.
This test doesn't help much, because this tests the internal implementation, and not the CLI frontend. If you add sinon.assert.notCalled(cmd.options.MultiExtensionRunner), then you would see that the test fails, because the runner is run anyway.
There are two relevant conditions to check:
-t firefox-desktopworks.-t firefox-desktop -t not-supported -t firefox-desktopresults in an error message.
To test (1), you can add the parameter after this line:
web-ext/tests/functional/test.cli.run.js
Line 20 in 72bb28c
To test (2), you could copy that test, cut the parts that we don't need, and add it to the same file.
https://github.com/mozilla/web-ext/blob/master/tests/functional/test.cli.run.js#L11
There was a problem hiding this comment.
Would it be fine if I use assert to validate the exitCode and stderr?
There was a problem hiding this comment.
Yes, that works. The reportCommandErrors in the existing test mainly exists to help with debugging if there is an unexpected test failure.
| @@ -442,6 +442,7 @@ Example: $0 --help run. | |||
| default: 'firefox-desktop', | |||
| demandOption: false, | |||
| type: 'array', | |||
| choices: ['firefox-desktop', 'firefox-android'], | |||
There was a problem hiding this comment.
Adding choices here automatically causes them to be added to the help test, when web-ext run --help is used. Now the description (a few lines back) can also be simplified. Could you remove the text between parentheses to shorten the description?
src/program.js
Outdated
| @@ -436,8 +436,8 @@ Example: $0 --help run. | |||
| .command('run', 'Run the extension', commands.run, { | |||
| 'target': { | |||
| alias: 't', | |||
| describe: 'The extensions runners to enable (e.g. firefox-desktop, ' + | |||
| 'firefox-android). Specify this option multiple times to ' + | |||
| describe: 'The extensions runners to enable. ' + | |||
There was a problem hiding this comment.
Could you combine this line with the next two lines so that the string is only cut when the 80-character line limit has been reached?
tests/functional/test.cli.run.js
Outdated
| @@ -47,4 +48,31 @@ describe('web-ext run', () => { | |||
| } | |||
| }); | |||
| })); | |||
|
|
|||
| it('should not accept: --target INVALIDTARGET', | |||
| () => withTempAddonDir( | |||
There was a problem hiding this comment.
In this test, the program is supposed to exit early due to invalid command line parameters. Therefore it is not necessary to create a temporary add-on. You can simplify the test:
it('should not accept: --target INVALIDTARGET', async () => {
const argv = [ ... ] // 'run' and the --target options only
return execWebExt(argv, {}).then(({exitCode, stderr}) => {
...
});
});
Combine lines in "describe" to follow 80 character line limit
The program exit eary due to invalid command line parameters. Therefore it is unnecessary to create a temporary add-on or pass spawnOptions.
Rob--W
changed the title
|
Hey @onlywicked, thanks so much for the patch! 🎉 Your contribution has been added to our recognition wiki. Would you be interested in creating an account on mozillians.org? I'd be happy to vouch for you! |
I have signed up for mozillians.org. |
|
Sorry for the delay, @onlywicked! Your Mozillians profile is now vouched. ✨ Welcome onboard! We look forward to seeing you around the project. |
Fixes #1642