feat: Added --profile-create-if-missing option #2064

akhilpanchal · 2020-11-03T03:54:49Z

Fixes #2058

coveralls · 2020-11-03T03:59:51Z

Coverage remained the same at 100.0% when pulling 21a2b66 on akhilpanchal:add-profile-create-new-cli-option into 55931e7 on mozilla:master.

coveralls · 2020-11-03T03:59:51Z

Coverage remained the same at 100.0% when pulling 849835d on akhilpanchal:add-profile-create-new-cli-option into 55931e7 on mozilla:master.

tests/unit/test-cmd/test.run.js

…ption

rpl

Thanks @akhilpanchal this PR looks in a very good shape, good job!

I'm requesting just a couple of small changes, one is actually due to my mistake in the guidance comment (I missed to notice that one of the behaviors I suggested is actually kind of conflicting with the use case described in the enhancement request), the rest should be just small and easy nits.

Let me know if any of my comments below are unclear (or also if you disagree with any of my points) and I'll be happy to discuss about those before you plan the changes, or ping me as soon this PR is ready for another (likely its last) review round.

rpl · 2020-11-03T19:05:39Z

src/cmd/run.js

+  if (profileCreateNew && profileDir) {
+    const isDir = fs.existsSync(profileDir);
+    if (isDir) {
+      throw new WebExtError(`Directory ${profileDir} already exists.`);


@akhilpanchal I did re-think a bit more about this and this part of the approach that I suggested in my comment #2058 (comment) is actually not covering the use case describe in the issue description in a nice way, because we will throw an error if the directory exist (while the workaround launch script in the issue description is not throwing in that case).

My apologies for missing to notice this when I wrote the initial guidance comment in the issue.

I think that relaxing this and just log a message (we may opt for a log.info if we want the user to see it even if web-ext run isn't in verbose mode) if the directory already exist may be actually reasonable (and it would better cover the enhancement request)

To better match the expected behavior we may also renamed the option as --profile-create-if-missing.

I completely agree with you about not throwing and just logging an info message that tells the user that the directory already exists. Will update this behavior and also update the option name.

rpl · 2020-11-03T19:13:23Z

src/program.js

+        describe: 'Create directory if the profile directory specified by ' +
+                  'the -p option doesn\'t already exist. If it already ' +
+                  'exists, an error will be thrown.',


nit: Create the profile directory if it does not already exist.

This version does also omit the -p option, mainly because:

that is only the option for the Firefox profile, and this is going to cover the chromium profile cli option as well

I don't think we need to describe that here in too much detail (and option name may actually be clear enough for the user to guess)

we should still add a more detailed description for the option in the mozilla/extension-workshop repository, as part of the web-ext cli options doc page (and we will be happy if you want to create a followup PR in that repo to take care of that after this is landed and we are about to release it in a new web-ext version)

Sure. That sounds good! 👍

rpl · 2020-11-03T19:15:41Z

src/cmd/run.js

+  const profileDir = firefoxProfile || chromiumProfile;
+
+  if (profileCreateNew && profileDir) {
+    const isDir = fs.existsSync(profileDir);


Given that this is already an async function and we are using the fs promise-based API exported by mz, we could easily use the non *Sync version of the fs.exists and fs.mkdir methods and just await on them.

would you mind to look into making this change?

@rpl
I spent some time looking into making these calls async.
The async version of fs.existsSync (fs.exists) is deprecated and to make this call async, we will have to either use fs.stat or fs.access inside of a try catch block (to check for the not found exception and only then create the directory) that might make this code a little more verbose than it needs to be.

We could also use the path-exists library that might alleviate the need to have a try catch block.

Do you have any other thoughts?

@akhilpanchal thanks! that's a very nice research work, I really appreciated that!
That note in the path-exists readme was also a pretty interesting read (the part that warns about the risks of race conditions, in this particular case I don't think we need to worry about that, but it still a good point to keep in mind in general).

Personally I think that we can spare us to add a dependency and just go for fs.existsSync for checking if the directory exists and just prefer the async method for the fs.mkdir part, does that sound reasonable for you too?

Thanks @rpl. Appreciate that ☺️.
Yes, I agree with you that we can spare ourselves from adding a new dependency and just use the async fs.mkdir call and keep the sync fs.existsSync.
Sending an update to the PR in a few moments.

…tead of throwing when dir exists and updated unit tests

rpl

@akhilpanchal this looks almost there, follows just some nitpicks related to the test cases.

Feel free to ping me if there is anything unclear (or if I was misreading something and my comment may be wrong).

Thanks again for working on this enhancement!

rpl · 2020-11-05T13:52:58Z

tests/unit/test-cmd/test.run.js

-     });
-});
+     }
+  );


Nitpick, would you mind to move the ); here to the line before it as it was? (just to remove the unrelated change form the diff and keep it consistent with the rest of the test files).

rpl · 2020-11-05T13:54:57Z

tests/unit/test-cmd/test.run.js

+      fs.mkdir.restore();
+    });
+
+    it('creates dir when firefox profile doesn\'t exist',


Nitpick (just to void escaping the single quote): creates firefox profile directory if it does not exist

rpl · 2020-11-05T14:02:25Z

tests/unit/test-cmd/test.run.js

+       }
+    );
+
+    it('creates dir when chromium profile doesn\'t exist',


Nitpick similar to the previous one: creates a chromium profile directory if it does not exist

rpl · 2020-11-05T14:14:46Z

tests/unit/test-cmd/test.run.js

+       async () => {
+         sinon.stub(fs, 'existsSync').returns(false);
+         const chromiumProfile = '/pretend/path/to/Chromium/profile';
+         const cmd = prepareRun();
+
+         await cmd.run({
+           chromiumProfile,
+           target: 'chromium',
+           profileCreateIfMissing: true,
+         });
+
+         sinon.assert.calledWith(fs.mkdir, chromiumProfile);
+
+         fs.existsSync.restore();
+       }
+    );


These two tests are so similar to each other that it may be reasonable to refactor it into a single parametrized test, e.g. something like:

async function testCreateProfileIfMissing(runParams, expectedMkdirParam) { sinon.stub(fs, 'existsSync').returns(false); const cmd = prepareRun(); await cmd.run({ ...runParams, promiseCreateIfMissing: true }); sinon.assert.calledWith(fs.mkdir, fs.mkdir, expectedMkdirParam); } const fakeProfilePath = '/pretend/profile/path' it('creates a Firefox profile directory if it does not exist', testCreateProfileIfMissing.bind({ firefoxProfile: fakeProfielPath }, fakeProfilePath)); it('creates a Chromium profile directory if it does not exist', testCreateProfileIfMissing.bind( { chromiumProfile: fakeProfielPath, target: 'chromium' }, fakeProfilePath));

does that sounds reasonable to you too?

I definitely agree there is quite a bit of duplication in the tests. I actually feel even the last 2 tests are very similar.
How do you feel about going a step further:

async function testCreateProfileIfMissing(existsSyncReturnValue, runParams, expectedMkdirParam) { sinon.stub(fs, 'existsSync').returns(existsSyncReturnValue); const cmd = prepareRun(); await cmd.run({ ...runParams, promiseCreateIfMissing: true }); sinon.assert.calledWith(fs.mkdir, fs.mkdir, expectedMkdirParam); } const fakeProfilePath = '/pretend/profile/path' it('creates a Firefox profile directory if it does not exist', testCreateProfileIfMissing.bind(false, { firefoxProfile: fakeProfielPath }, fakeProfilePath)); it('creates a Chromium profile directory if it does not exist', testCreateProfileIfMissing.bind( false, { chromiumProfile: fakeProfielPath, target: 'chromium' }, fakeProfilePath)); it('Uses profile directory when firefox profile directory already exists', testCreateProfileIfMissing.bind(true, { firefoxProfile: fakeProfielPath }, fakeProfilePath)); it('Uses profile directory when chromium profile directory already exists', testCreateProfileIfMissing.bind( true, { chromiumProfile: fakeProfielPath, target: 'chromium' }, fakeProfilePath));

@akhilpanchal sure thing, if all four tests can be expressed with a single parametrized function and still be readable enough (so that we will not have to spent to much time to decode a failure, if any of these test would fail at some point in the future).

I think that for the last 2 test cases the assertion sinon.assert.calledWith(fs.mkdir, ...) should actually be replaced by one that assert that mkdir has not been called at all.

(I guess that if we may also assert in all 4 tests that we did run the extension runner with the expected profile dir as the last 2 tests seems to be doing in this version).

@rpl
Thanks for all your feedback 👍.
I addressed all your comments in the latest commit.

I did abstract the common test code into a single function.
Does the implementation look reasonable to you?
If not, I can either split them out to 2 different parameterized tests like you suggested earlier, OR any other way you feel that makes the tests more readable and easier to maintain.
Let me know.

rpl · 2020-11-05T14:17:15Z

tests/unit/test-cmd/test.run.js

+
+         sinon.assert.calledWith(fs.mkdir, chromiumProfile);
+
+         fs.existsSync.restore();


I think that we should restore fs.existsSync in the afterEach (and stub it in beforeEach, eventually changing what it does return in the particular test if necessary) as for the other stub, in case one test case fails and fs.existsSync.restore() isn't reached.

rpl · 2020-11-05T14:24:04Z

tests/unit/test-cmd/test.run.js

+       }
+    );
+
+    it('Uses profile directory when firefox profile directory already exists',


Nitpicks:

Uses should be in lowercase, we may also tweak it into uses the give firefox profile if it does exist.

we may want to assert that fs.mkdir was not called in this case

stub and restore fs.existsSync in beforeEach and afterEach for the same reason described in my previous comment

Another thing that I've been thinking of is:
could these other two tests be refactored in a single parametrized test function as for the other two?

rpl · 2020-11-05T14:24:23Z

tests/unit/test-cmd/test.run.js

+       }
+    );
+  });
+});


Nitpick, add the newline here

akhilpanchal · 2020-11-06T15:17:39Z

@rpl
Some questions that I had related to the project setup:
I am using VSCode with ESLint and Prettier Plugins and looks like the auto formatting isn't working for me for this project. So I had to manually fix things like indentation errors.

Which Editor do you use?
Do you rely on any plugins to do auto-fix formatting and ESLint errors OR do you use the ESLint CLI?

rpl

@akhilpanchal this looks pretty great, thank you so much for refactoring the 4 test cases into a single parametrized function, I really appreciate that.

The following comments are related to some additional nits (still only on the test case) that it would be nice to apply before landing this (but I'm also approving the PR because they are just small nitpicks nothing really that much critical, and one is a note about a corner case that we may want to cover explicitly but it can be evaluated and then covered as a separate followup).

rpl · 2020-11-06T16:21:31Z

tests/unit/test-cmd/test.run.js

+        sinon.assert.calledWith(fs.mkdir, expectedProfile);
+      }
+
+      if (expectedProfile === fakeFirefoxProfile) {


another option to decide which extension runner we should run the assertion on would be to look to runParams.target and based on that (if it is == to chromium) run the assertion on the chromiumRunnerStub.

and by doing that I think that we would not need to use a separate fake profile dir for the two of them and we could just use one for both.

how that sounds?

Sounds perfect. 👍

rpl · 2020-11-06T16:25:24Z

tests/unit/test-cmd/test.run.js

-         fs.existsSync.restore();
-       }
+         }, fakeFirefoxProfile
+       )


Nitpick: would you mind to move this ) to the previous line and keep the parameters on separate lines?
I mean something like:

testCreateProfileIfMissing( false, { ... }, fakeFirefoxProfile);

as it is more or less the convention we do use at the moment, same for other calls that follow.

rpl · 2020-11-06T16:27:45Z

tests/unit/test-cmd/test.run.js

+    const fakeChromiumProfile = '/pretend/path/to/Chromium/profile';
+
+    async function testCreateProfileIfMissing(
+      existsSyncReturnValue,


Nitpick: we may call this parameter as expectProfileExists and then use it for fs.existsSync and the if..else at line 355-359 (just because I prefer the name to rapresent the conditions of the tests instead of the name of the stubbed function that will return that value).

rpl · 2020-11-06T16:45:00Z

src/cmd/run.js

@@ -117,6 +121,18 @@ export default async function run(
  const customPrefs = pref;
  const manifestData = await getValidatedManifest(sourceDir);

+  const profileDir = firefoxProfile || chromiumProfile;
+
+  if (profileCreateIfMissing && profileDir) {


while I was looking this part one more time I did notice another corner case that may be worth to consider (and eventually handle in a follow up):

if the user does call web-ext run with --profile-create-if-missing but without any profile parameter we currently just ignore the --profile-create-if-missing, it isn't strictly wrong and so it is not really a bit deal, but another option would be to throw an explicit UsageError to make the user aware that --profile-create-if-missing should be paired with --firefox-profile or --chromium-profile).

what do you think?

as I did mention it is not a really big deal and so it seems ok to defer it to a separate follow up.

Sure. That does make sense. I can do that in a follow up PR. Should we be creating a separate issue for that? Or should I just send another PR after this one is merged?

Sure. That does make sense. I can do that in a follow up PR. Should we be creating a separate issue for that? Or should I just send another PR after this one is merged?

@akhilpanchal thanks! just a separate PR for that part would be fine

in this one let's just cover the nits on the test, and after that I'll just merge it and you can open the follow up.

@rpl
I have addressed all the nits. Please let me know if I missed something.
Once this is merged, I will create a follow up for this enhancement.
Also, you mentioned making changes to the mozilla/extension-workshop repo. Should that be also linked to an issue in that repo?

rpl · 2020-11-06T18:39:14Z

@rpl
Some questions that I had related to the project setup:
I am using VSCode with ESLint and Prettier Plugins and looks like the auto formatting isn't working for me for this project. So I had to manually fix things like indentation errors.
* Which Editor do you use?

* Do you rely on any plugins to do auto-fix formatting and ESLint errors OR do you use the ESLint CLI?

I totally missed this comment, my apologies.

Personally I use neovim (VSCode is a pretty nice editor, and way more friendly then vim, but I'm used to have a pretty "terminal + zsh + tmux"-based workflow and I like to use an editor that is running in the tmux session related to that project or task), this project doesn't use prettier at the moment and so some conventions are actually enforced by eslint and others are just preferred by us for consistency (but eslint is not going to complain nor format automatically the one that are not enforced by eslint rules).

rpl · 2020-11-06T19:58:04Z

@rpl
I have addressed all the nits. Please let me know if I missed something.
Once this is merged, I will create a follow up for this enhancement.

@akhilpanchal
looks great! I just merged it on master. 🎉

Thanks again for contributing this enhancement!!!

Also, you mentioned making changes to the mozilla/extension-workshop repo. Should that be also linked to an issue in that repo?

Sure, linking the web-ext issue in the extension-workshop PR description comment would be nice 👍
Add me as a reviewer (or mention me in a comment) once you open it.

feat: Added --profile-create-new option

849835d

akhilpanchal commented Nov 3, 2020

View reviewed changes

tests/unit/test-cmd/test.run.js Outdated Show resolved Hide resolved

rpl self-requested a review November 3, 2020 11:47

test: Added unit tests for profile-create-new with chromium-profile o…

6a3357a

…ption

rpl requested changes Nov 3, 2020

View reviewed changes

akhilpanchal added 2 commits November 3, 2020 16:32

chore: Renamed cli option to profile-create-if-missing, show info ins…

9ad2bd4

…tead of throwing when dir exists and updated unit tests

chore: Use async fs.mkdir instead of sync, update unit tests

dd16b6a

rpl requested changes Nov 5, 2020

View reviewed changes

chore: Refactor unit tests for profile-create-if-missing cli option

8f8d0a7

rpl approved these changes Nov 6, 2020

View reviewed changes

chore: improve tests for profile-create-if-missing and fix formatting

21a2b66

rpl changed the title ~~feat: Added --profile-create-new option~~ feat: Added --profile-create-if-missing option Nov 6, 2020

rpl merged commit 279a279 into mozilla:master Nov 6, 2020

This was referenced Nov 15, 2020

feat: threw error if profile-create-new-if-missing used without profile #2073

Merged

doc: Added entry for run command option profile-create-new-if-missing mozilla/extension-workshop#832

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Added --profile-create-if-missing option #2064

feat: Added --profile-create-if-missing option #2064

akhilpanchal commented Nov 3, 2020

coveralls commented Nov 3, 2020 •

edited

Loading

coveralls commented Nov 3, 2020

rpl left a comment

rpl Nov 3, 2020

akhilpanchal Nov 3, 2020

rpl Nov 3, 2020

akhilpanchal Nov 3, 2020

rpl Nov 3, 2020

akhilpanchal Nov 3, 2020

rpl Nov 4, 2020

akhilpanchal Nov 4, 2020 •

edited

Loading

rpl left a comment

rpl Nov 5, 2020

rpl Nov 5, 2020

rpl Nov 5, 2020

rpl Nov 5, 2020

akhilpanchal Nov 5, 2020

rpl Nov 5, 2020

akhilpanchal Nov 6, 2020

rpl Nov 5, 2020

rpl Nov 5, 2020

rpl Nov 5, 2020

akhilpanchal commented Nov 6, 2020

rpl left a comment

rpl Nov 6, 2020

akhilpanchal Nov 6, 2020

rpl Nov 6, 2020

rpl Nov 6, 2020

rpl Nov 6, 2020

akhilpanchal Nov 6, 2020

rpl Nov 6, 2020

akhilpanchal Nov 6, 2020

rpl commented Nov 6, 2020

rpl commented Nov 6, 2020


		sinon.assert.calledWith(fs.mkdir, chromiumProfile);

		fs.existsSync.restore();

feat: Added --profile-create-if-missing option #2064

feat: Added --profile-create-if-missing option #2064

Conversation

akhilpanchal commented Nov 3, 2020

coveralls commented Nov 3, 2020 • edited Loading

coveralls commented Nov 3, 2020

rpl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akhilpanchal Nov 4, 2020 • edited Loading

Choose a reason for hiding this comment

rpl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akhilpanchal commented Nov 6, 2020

rpl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpl commented Nov 6, 2020

rpl commented Nov 6, 2020

coveralls commented Nov 3, 2020 •

edited

Loading

akhilpanchal Nov 4, 2020 •

edited

Loading