fix: Support FIREFOX_BINARY again for web-ext run
#523
Conversation
| @@ -218,7 +218,7 @@ Example: $0 --help run. | |||
| }) | |||
| .command('run', 'Run the web extension', commands.run, { | |||
| 'firefox': { | |||
| alias: 'f', | |||
| alias: ['f', 'firefox-binary'], | |||
| @@ -47,3 +47,44 @@ describe('web-ext run', () => { | |||
| }); | |||
| })); | |||
| }); | |||
|
|
|||
| describe('web-ext run', () => { | |||
There was a problem hiding this comment.
Functional tests are very expensive because they take a lot longer than unit tests. Anytime we add a functional test we have to ask, can this be tested just as well in a unit test? For this case I think the answer is yes. You can make a test that passes in a stub version of the run function and asserts --firefox-binary is passed to the function like run({firefox: '/the/path/'}). You can model it after this test: https://github.com/mozilla/web-ext/blob/master/tests/unit/test.program.js#L283-L294
There was a problem hiding this comment.
Doesn't this test do exactly that?
There was a problem hiding this comment.
No, that test does other things:
- It invokes the
cmd.run()function directly so it doesn't cover any command line option parsing - It is making sure that the
firefoxAppobject (which represents the Firefox application process) is getting called correctly which is something separate
| @@ -307,6 +307,19 @@ describe('program.main', () => { | |||
| process.cwd() + path.sep); | |||
| }); | |||
| }); | |||
| it('passes the path of a firefox binary when specified', () => { | |||
There was a problem hiding this comment.
nit: this line should have one line of whitespace above it to show that it() is a new function separate from the function above it
| {commands: fakeCommands}) | ||
| .then(() => { | ||
| assert.equal(fakeCommands.run.called, true); | ||
| assert.equal(fakeCommands.run.firstCall.args[0].firefoxBinary, |
There was a problem hiding this comment.
you should assert that the call to run() passes firefox, not firefoxBinary because firefox is the variable used by the command.
There was a problem hiding this comment.
this test needs to make sure that --firefox-binary gets treated just like --firefox
Fixes #512
firefox-binary