Changed --firefox-binary to --firefox

[shubheksha]

Fixes #116

• Changed the option as specified
• Added an alias 'f'

[coveralls]

Coverage decreased (-1.7%) to 98.326% when pulling 9b0b16c on shubheksha:fix/116 into 6654239 on mozilla:master.

[coveralls]

Coverage decreased (-0.9%) to 99.087% when pulling 9b0b16c on shubheksha:fix/116 into 6654239 on mozilla:master.

[kumar303]

I noticed that this includes some of your previous changes to the lint tasks but those have already been merged to master. There are a couple ways to fix this. I think the easiest is to pull from the mozilla repo (so you have the latest changes), merge master into your work branch (fix/116), and push to your fix/116 branch. The pull request should then show only your #116 related changes.

[coveralls]

Coverage decreased (-34.4%) to 65.601% when pulling 67a03fe on shubheksha:fix/116 into 6654239 on mozilla:master.

[kumar303]

As you mentioned, the errors are probably due to a variable name that didn't get changed somewhere. I can't spot it by looking at the diff. To solve a problem like this I'd suggest trying to start over and introduce a smaller change, one that does not touch so many functions. One way to do that is to alias firefox to firefoxBinary at the top of the run() function. This way it will accept the new parameter from the command line but internally it should work like before. Example:

export default function run(
  {
    sourceDir, artifactsDir, firefox, firefoxProfile,
    ...
  }: CmdRunParams,
  {
    firefoxApp=defaultFirefox,
    ...
  }: CmdRunOptions = {}): Promise<Object> {

  // With this declaration the code should function as it did before.
  const firefoxBinary = firefox;
}

You will need to do undo your changes first. You could use git revert or make a new pull request if that's easier.

[kumar303]

I think this patch is still lingering because it was removed at the end of your work done on #434 You can manually fix it just by editing this line back to the way it is on master.

[shubheksha]

Alright, I'll try doing that & make a new PR. Closing this one.

[shubheksha]

This is surprising, but I made the required changes to --firefox-binary & didn't change anything else & tests passed.
However, when I tried to change firefox to firefoxApp in run.js, I ran into the same errors as this PR. I didn't even touch the firefoxBinary variable. What could this mean?

[kumar303]

You would definitely need to change the old firefox to firefoxApp because otherwise it will collide with the new firefox parameter. Otherwise, I'm not sure what's going on. If you make a PR with your new patch I can take a look.

[kumar303]

Also, another way to minimize your changes to make it easier to see what's going on would be to alias both variables. Example:

export default function run(
  {
    sourceDir, artifactsDir, firefox, firefoxProfile,
    ...
  }: CmdRunParams,
  {
    firefoxApp=defaultFirefox,
    ...
  }: CmdRunOptions = {}): Promise<Object> {

  // With this declaration the code should function as it did before.
  const firefoxBinary = firefox;
  firefox = firefoxApp;
  // Any code below this should run the same way it did before...
}

[shubheksha]

I tried to do this, but it throws a Duplicate Declaration error in the build step itself.

[kumar303]

Instead of redeclaring it, you could also keep the changes confined to only the run() function. For example, when you need to pass firefox or firefoxApp to another object, you could do it like:

export default function run(
  {
    sourceDir, artifactsDir, firefox, firefoxProfile,
    ...
  }: CmdRunParams,
  {
    firefoxApp=defaultFirefox,
    ...
  }: CmdRunOptions = {}): Promise<Object> {

  return getValidatedManifest(sourceDir)
    .then((manifestData) => {
      return new ExtensionRunner({
        ...
        // This would make the code work like it did before.
        firefox: firefoxApp,
        firefoxBinary: firefox,
        ...
      });
    }).then(...);
}

[shubheksha]

Even this doesn't work. Even the slightest change to the run function is causing a variable to be undefined which is making the tests fail.
I tried to trace the variable using the property printed in the error message. But fixing one causes another one to fail & get undefined.