fix: --firefox-binary to --firefox #440

shubheksha · 2016-08-17T20:32:54Z

Fixes #116
For some reason, changing firefox to firefoxApp even on just this line makes the tests fail.

kumar303 · 2016-08-17T20:38:05Z

The patch you have here will cause firefoxBinary to be undefined since yargs will no longer pass it in as such. Yargs would be passing in firefox but that is ignored since run() does not reference it in the destructor.

coveralls · 2016-08-17T20:38:57Z

Coverage remained the same at 100.0% when pulling f614198 on shubheksha:fix/116 into 6654239 on mozilla:master.

shubheksha · 2016-08-17T20:39:43Z

What you say makes perfect sense, but how come all the test are passing in this case & it isn't being caught?

kumar303 · 2016-08-17T21:13:11Z

Ha, yeah, that is a very good question. This is actually a problem with the test suite that I plan to fix in #377

kumar303 · 2016-08-18T20:34:22Z

If you update the PR with the changes you mentioned here then I could take a look.

shubheksha · 2016-08-19T17:44:54Z

I tried a lot of different combinations of changes, actually. I tried changing all occurences inside the run function, then I tried to trace the what was getting undefined using the error messages, so I am kinda unsure of what to include in the PR. What do you suggest?

kumar303 · 2016-08-19T23:37:04Z

I think the way to fix it is to only change the two properties in the run() function and nothing else. You would change the firefoxBinary parameter to firefox and the firefox option to firefoxApp. Renaming all the test code that invokes the run() function can be confusing. You can use the Flow type checker (which is part of the test suite invoked by npm start) to guide you.

I tried this quickly in your branch and this got me past most of the errors but didn't completely fix it. Does this help? Here is my diff:

diff --git a/src/cmd/run.js b/src/cmd/run.js
index 0875c56..e8cfe95 100644
--- a/src/cmd/run.js
+++ b/src/cmd/run.js
@@ -146,25 +146,25 @@ export function defaultFirefoxClient(
 export type CmdRunParams = {
   sourceDir: string,
   artifactsDir: string,
-  firefoxBinary: string,
+  firefox: string,
   firefoxProfile: string,
   preInstall: boolean,
   noReload: boolean,
 };

 export type CmdRunOptions = {
-  firefox: typeof defaultFirefox,
+  firefoxApp: typeof defaultFirefox,
   firefoxClient: typeof defaultFirefoxClient,
   reloadStrategy: typeof defaultReloadStrategy,
 };

 export default function run(
   {
-    sourceDir, artifactsDir, firefoxBinary, firefoxProfile,
+    sourceDir, artifactsDir, firefox, firefoxProfile,
     preInstall=false, noReload=false,
   }: CmdRunParams,
   {
-    firefox=defaultFirefox,
+    firefoxApp=defaultFirefox,
     firefoxClient=defaultFirefoxClient,
     reloadStrategy=defaultReloadStrategy,
   }: CmdRunOptions = {}): Promise<Object> {
@@ -190,8 +190,8 @@ export default function run(
     .then((manifestData) => {
       return new ExtensionRunner({
         sourceDir,
-        firefox,
-        firefoxBinary,
+        firefox: firefoxApp,
+        firefoxBinary: firefox,
         manifestData,
         profilePath: firefoxProfile,
       });
@@ -265,7 +265,7 @@ export default function run(
           profile, client, sourceDir, artifactsDir, addonId,
         });
       }
-      return firefox;
+      return firefoxApp;
     });
 }

diff --git a/tests/test-cmd/test.run.js b/tests/test-cmd/test.run.js
index 0dc2295..25e4a75 100644
--- a/tests/test-cmd/test.run.js
+++ b/tests/test-cmd/test.run.js
@@ -37,7 +37,7 @@ describe('run', () => {
       noReload: true,
     };
     let options = {
-      firefox: getFakeFirefox(),
+      firefoxApp: getFakeFirefox(),
       firefoxClient: sinon.spy(() => {
         return Promise.resolve(fake(RemoteFirefox.prototype, {
           installTemporaryAddon: () =>
@@ -77,7 +77,7 @@ describe('run', () => {
     let profile = {};

     const cmd = prepareRun();
-    const {firefox} = cmd.options;
+    const {firefoxApp} = cmd.options;
     const firefoxClient = fake(RemoteFirefox.prototype, {
       installTemporaryAddon: () => Promise.resolve(tempInstallResult),
     });
@@ -91,8 +91,8 @@ describe('run', () => {
       assert.equal(install.called, true);
       assert.equal(install.firstCall.args[0], cmd.argv.sourceDir);

-      assert.equal(firefox.run.called, true);
-      assert.deepEqual(firefox.run.firstCall.args[0], profile);
+      assert.equal(firefoxApp.run.called, true);
+      assert.deepEqual(firefoxApp.run.firstCall.args[0], profile);
     });
   });

@@ -114,26 +114,25 @@ describe('run', () => {
   });

   it('passes a custom Firefox binary when specified', () => {
-    const firefoxBinary = '/pretend/path/to/Firefox/firefox-bin';
+    const firefox = '/pretend/path/to/Firefox/firefox-bin';
     const cmd = prepareRun();
-    const {firefox} = cmd.options;
+    const {firefoxApp} = cmd.options;

-    return cmd.run({firefoxBinary}).then(() => {
-      assert.equal(firefox.run.called, true);
-      assert.equal(firefox.run.firstCall.args[1].firefoxBinary,
-                   firefoxBinary);
+    return cmd.run({firefoxApp}).then(() => {
+      assert.equal(firefoxApp.run.called, true);
+      assert.equal(firefoxApp.run.firstCall.args[1].firefox, firefox);
     });
   });

   it('passes a custom Firefox profile when specified', () => {
     const firefoxProfile = '/pretend/path/to/firefox/profile';
     const cmd = prepareRun();
-    const {firefox} = cmd.options;
+    const {firefoxApp} = cmd.options;

     return cmd.run({firefoxProfile}).then(() => {
-      assert.equal(firefox.createProfile.called, false);
-      assert.equal(firefox.copyProfile.called, true);
-      assert.equal(firefox.copyProfile.firstCall.args[0],
+      assert.equal(firefoxApp.createProfile.called, false);
+      assert.equal(firefoxApp.copyProfile.called, true);
+      assert.equal(firefoxApp.copyProfile.firstCall.args[0],
                    firefoxProfile);
     });
   });

shubheksha · 2016-08-20T15:30:10Z

I didn't change anything in the tests, but that doesn't seem to make a difference, I get the same set of errors. I figured that the issue is mainly with this line - createProfile(). This is what is causing 7/10 tests to fail. Despite changing the name of the object to firefoxApp, it is undefined.

coveralls · 2016-08-20T17:27:44Z

Coverage remained the same at 100.0% when pulling 91b3fd4 on shubheksha:fix/116 into 6654239 on mozilla:master.

shubheksha · 2016-08-20T17:38:12Z

I fixed everything & created a PR, but the flowbin:check tests don't properly run on my system, hence I couldn't catch the errors which caused the builds to fail. I get an error, the details of which can be found in the screenshot.

Why does this happen?
I found this: facebook/flow#2017

coveralls · 2016-08-20T17:45:55Z

Coverage remained the same at 100.0% when pulling c760539 on shubheksha:fix/116 into 6654239 on mozilla:master.

coveralls · 2016-08-20T18:31:51Z

Coverage remained the same at 100.0% when pulling d10c57d on shubheksha:fix/116 into 6654239 on mozilla:master.

rpl · 2016-08-22T13:06:34Z

src/cmd/run.js

@@ -62,7 +62,7 @@ export function defaultWatcherCreator(

 export type ReloadStrategyParams = {
  addonId: string,
-  firefox: FirefoxProcess,
+  firefoxApp: FirefoxProcess,


@shubheksha It is probably better to rename this to firefoxProcess (because it is of type FirefoxProcess, which is a type alias of ChildProcess), because it is not the same type of the other properties that you are rename to firefoxApp in this patch (in this patch most of the times it is associated to the helper module defined as "src/firefox/index.js", which is a collection of helper functions used to create/copy a Firefox profile, install an extension into the profile, run a firefox instance etc.) and it is going to be helpful to avoid confusion in the future.

rpl · 2016-08-22T13:15:43Z

@shubheksha I took a look at the patch and the previous comments attached to this pull request, and I added a couple of comments related to some minor changes.

Thanks for working on this patch, besides the goal of renaming the command line option, this is going to help to make the sources much more clear, there are currently too many things named firefox in the sources :-)

shubheksha · 2016-08-22T13:36:18Z

@rpl, thank you so much for the detailed feedback, I really appreciate it! :) I made all the changes you proposed, however they were causing two tests to fail. Going through the errors, I figured that the same change needs to be made in four more places in addition to those you mentioned. Is this correct & I should push my changes or am I missing something?

shubheksha · 2016-08-22T13:36:21Z

src/cmd/run.js

@@ -75,15 +75,15 @@ export type ReloadStrategyOptions = {

 export function defaultReloadStrategy(
  {
-    addonId, firefox, client, profile, sourceDir, artifactsDir,
+    addonId, firefoxApp, client, profile, sourceDir, artifactsDir,


This had to be changed.

coveralls · 2016-08-22T16:37:11Z

Coverage remained the same at 100.0% when pulling 6b6d65d on shubheksha:fix/116 into bd14cf9 on mozilla:master.

rpl · 2016-08-23T12:44:42Z

tests/test-cmd/test.run.js

@@ -10,7 +10,7 @@ import {onlyInstancesOf, WebExtError, RemoteTempInstallNotSupported}
 import run, {
  defaultFirefoxClient, defaultWatcherCreator, defaultReloadStrategy,
 } from '../../src/cmd/run';
-import * as firefox from '../../src/firefox';
+import * as firefoxApp from '../../src/firefox';


@shubheksha how about renaming the above to defaultFirefox, like it is named in most of the sources?
it would make easier to guess that they are the same thing.

(to be completely honest, now it would be probably better to name it defaultFirefoxApp, here and in "src/cmd/run.js", to make it clear that it is the default implementation of what we usually call firefoxApp)

@rpl, sorry if this is a stupid question, but won't I have to rename all occurrences is test.run.js? Since it is being used as a type in run.js, I changed it in a few places & it works, but the same isn't true for the test file because as far as I understood, it is being used there directly & changing one occurrence breaks a lot of tests.

@shubheksha don't worry, asking question is a good thing.

The very minimal change that I was suggesting require two additional identifier renamed in the test file:

diff --git a/tests/test-cmd/test.run.js b/tests/test-cmd/test.run.js index 49dbd57..7293530 100644 --- a/tests/test-cmd/test.run.js +++ b/tests/test-cmd/test.run.js @@ -10,7 +10,7 @@ import {onlyInstancesOf, WebExtError, RemoteTempInstallNotSupported} import run, { defaultFirefoxClient, defaultWatcherCreator, defaultReloadStrategy, } from '../../src/cmd/run'; -import * as firefoxApp from '../../src/firefox'; +import * as defaultFirefox from '../../src/firefox'; import {RemoteFirefox} from '../../src/firefox/remote'; import {TCPConnectError, fakeFirefoxClient, makeSureItFails, fake, fixturePath} from '../helpers'; @@ -69,7 +69,7 @@ describe('run', () => { run: () => Promise.resolve(), ...implementations, }; - return fake(firefoxApp, allImplementations); + return fake(defaultFirefox, allImplementations); }

Or, to rename defaultFirefox into defaultFirefoxApp (mainly because I think that defaultFirefoxApp immediately suggests that it is the default implementation of what we now call firefoxApp and not firefox) we need to apply some minor changes to both "test.run.js" and "src/cmd/run.js", e.g. this seems to be the needed changes in the "src/cmd/run.js" (besides the slightly changed version of the above patch applied to "test.run.js"):

diff --git a/src/cmd/run.js b/src/cmd/run.js index 15e29d9..8c06f83 100644 --- a/src/cmd/run.js +++ b/src/cmd/run.js @@ -1,5 +1,5 @@ /* @flow */ -import * as defaultFirefox from '../firefox'; +import * as defaultFirefoxApp from '../firefox'; import defaultFirefoxConnector from '../firefox/remote'; import { onlyInstancesOf, onlyErrorsWithCode, RemoteTempInstallNotSupported, @@ -153,7 +153,7 @@ export type CmdRunParams = { }; export type CmdRunOptions = { - firefoxApp: typeof defaultFirefox, + firefoxApp: typeof defaultFirefoxApp, firefoxClient: typeof defaultFirefoxClient, reloadStrategy: typeof defaultReloadStrategy, }; @@ -164,7 +164,7 @@ export default function run( preInstall=false, noReload=false, }: CmdRunParams, { - firefoxApp=defaultFirefox, + firefoxApp=defaultFirefoxApp, firefoxClient=defaultFirefoxClient, reloadStrategy=defaultReloadStrategy, }: CmdRunOptions = {}): Promise<Object> { @@ -276,7 +276,7 @@ export type ExtensionRunnerParams = { sourceDir: string, manifestData: ExtensionManifest, profilePath: string, - firefoxApp: typeof defaultFirefox, + firefoxApp: typeof defaultFirefoxApp, firefox: string, }; @@ -284,7 +284,7 @@ export class ExtensionRunner { sourceDir: string; manifestData: ExtensionManifest; profilePath: string; - firefoxApp: typeof defaultFirefox; + firefoxApp: typeof defaultFirefoxApp; firefox: string; constructor(

rpl · 2016-08-23T12:48:04Z

@shubheksha like pointed out in the above comment, it would be nice to rename defaultFirefox into defaultFirefoxApp, now that we use it as the default value of firefoxApp instead of firefox (just to avoid a potential confusion), but besides that, This looks great now! 👍

coveralls · 2016-08-24T17:30:47Z

Coverage remained the same at 100.0% when pulling f57273f on shubheksha:fix/116 into bd14cf9 on mozilla:master.

shubheksha · 2016-08-24T17:50:59Z

Ah, I completely missed the fact that the return value from that function is what is being used in the rest of the program. For run.js, I got it working the first time itself.
Also - why is the build failing for node v6 due to a timeout? Did I do something wrong? It passed all the test on my system, I am using node v4 & the build also passed with that node version.

rpl · 2016-08-24T18:22:34Z

@shubheksha I was wondering the same thing, and I tried to run the tests locally with nodejs v6 on your branch and they run correctly, as expected.

It looks like an unrelated issue, it is probably related to some issues on the travis ci build machine that was running the job because:

the addon server signing request/response in the tests uses a mock, it doesn't really create an http request and so it should not be a source of intermittent failures
I asked travis to re-run the job (by asking travis to rebuild the job failed on nodejs v6 unmodified, and on this second run, all the tests are completed successfully on nodejs v6 without any issue.

rpl · 2016-08-24T18:23:40Z

@kumar303 This PR is now r+

@shubheksha 👍 thanks for it!

shubheksha · 2016-08-24T18:42:48Z

@rpl, oh, okay!
So glad this is finally done! Thank you for all the help & guidance. :)

kumar303 · 2016-08-29T15:03:19Z

thanks @shubheksha and @rpl ! I just got back, thanks also for your patience.

muffinresearch added the state: pull request ready label Aug 17, 2016

shubheksha added 4 commits August 21, 2016 01:17

fix: --firefox-binary to --firefox

be00018

refactor: changed firefox to firefoxApp and firefoxBinary to firefox

2406fb1

refactor: changed firefox to firefoxApp and firefoxBinary to firefox

7b00324

refactor: changed firefox to firefoxApp and firefoxBinary to firefox

1dd0f06

rpl reviewed Aug 22, 2016
View reviewed changes

shubheksha reviewed Aug 22, 2016
View reviewed changes

refactor: changed firefox to firefoxProcess

6b6d65d

shubheksha force-pushed the fix/116 branch from d10c57d to 6b6d65d Compare August 22, 2016 16:29

rpl reviewed Aug 23, 2016
View reviewed changes

refactor: changed some occurences of firefoxApp to defaultFirefoxApp

f57273f

kumar303 merged commit 96748e3 into mozilla:master Aug 29, 2016

kumar303 removed the state: pull request ready label Aug 29, 2016

kumar303 self-assigned this Sep 1, 2016

kumar303 mentioned this pull request Sep 21, 2016

Support FIREFOX_BINARY again for web-ext run #512

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: --firefox-binary to --firefox #440

fix: --firefox-binary to --firefox #440

shubheksha commented Aug 17, 2016

kumar303 commented Aug 17, 2016

coveralls commented Aug 17, 2016 •

edited

Loading

shubheksha commented Aug 17, 2016

kumar303 commented Aug 17, 2016

kumar303 commented Aug 18, 2016

shubheksha commented Aug 19, 2016

kumar303 commented Aug 19, 2016

shubheksha commented Aug 20, 2016

coveralls commented Aug 20, 2016 •

edited

Loading

shubheksha commented Aug 20, 2016 •

edited

Loading

coveralls commented Aug 20, 2016 •

edited

Loading

coveralls commented Aug 20, 2016 •

edited

Loading

rpl Aug 22, 2016

rpl commented Aug 22, 2016

shubheksha commented Aug 22, 2016 •

edited

Loading

shubheksha Aug 22, 2016

coveralls commented Aug 22, 2016 •

edited

Loading

rpl Aug 23, 2016

shubheksha Aug 23, 2016

rpl Aug 24, 2016

rpl commented Aug 23, 2016

coveralls commented Aug 24, 2016 •

edited

Loading

shubheksha commented Aug 24, 2016

rpl commented Aug 24, 2016

rpl commented Aug 24, 2016

shubheksha commented Aug 24, 2016

kumar303 commented Aug 29, 2016

fix: --firefox-binary to --firefox #440

fix: --firefox-binary to --firefox #440

Conversation

shubheksha commented Aug 17, 2016

kumar303 commented Aug 17, 2016

coveralls commented Aug 17, 2016 • edited Loading

shubheksha commented Aug 17, 2016

kumar303 commented Aug 17, 2016

kumar303 commented Aug 18, 2016

shubheksha commented Aug 19, 2016

kumar303 commented Aug 19, 2016

shubheksha commented Aug 20, 2016

coveralls commented Aug 20, 2016 • edited Loading

shubheksha commented Aug 20, 2016 • edited Loading

coveralls commented Aug 20, 2016 • edited Loading

coveralls commented Aug 20, 2016 • edited Loading

rpl Aug 22, 2016

Choose a reason for hiding this comment

rpl commented Aug 22, 2016

shubheksha commented Aug 22, 2016 • edited Loading

shubheksha Aug 22, 2016

Choose a reason for hiding this comment

coveralls commented Aug 22, 2016 • edited Loading

rpl Aug 23, 2016

Choose a reason for hiding this comment

shubheksha Aug 23, 2016

Choose a reason for hiding this comment

rpl Aug 24, 2016

Choose a reason for hiding this comment

rpl commented Aug 23, 2016

coveralls commented Aug 24, 2016 • edited Loading

shubheksha commented Aug 24, 2016

rpl commented Aug 24, 2016

rpl commented Aug 24, 2016

shubheksha commented Aug 24, 2016

kumar303 commented Aug 29, 2016

coveralls commented Aug 17, 2016 •

edited

Loading

coveralls commented Aug 20, 2016 •

edited

Loading

shubheksha commented Aug 20, 2016 •

edited

Loading

coveralls commented Aug 20, 2016 •

edited

Loading

coveralls commented Aug 20, 2016 •

edited

Loading

shubheksha commented Aug 22, 2016 •

edited

Loading

coveralls commented Aug 22, 2016 •

edited

Loading

coveralls commented Aug 24, 2016 •

edited

Loading