feat: support of named profiles by saintsebastian · Pull Request #1149 · mozilla/web-ext

saintsebastian · 2017-12-10T16:59:32Z

Fixes #932

I shamelessly reused bits of code from previous attempt to solve this issue.

coveralls · 2017-12-10T17:04:07Z

Coverage decreased (-1.2%) to 98.831% when pulling e1652cc on saintsebastian:932-keep-profile-changes-with-named-profile into 1b79e77 on mozilla:master.

kumar303 · 2017-12-10T22:00:07Z

Thanks! Let me know when you've added tests and I can take a look.

coveralls · 2018-01-13T23:11:33Z

Coverage decreased (-0.7%) to 99.276% when pulling 4bef586 on saintsebastian:932-keep-profile-changes-with-named-profile into 5ed1682 on mozilla:master.

coveralls · 2018-01-13T23:11:33Z

Coverage decreased (-0.6%) to 99.356% when pulling 4bef586 on saintsebastian:932-keep-profile-changes-with-named-profile into 5ed1682 on mozilla:master.

saintsebastian · 2018-01-13T23:19:28Z

@kumar303 Hi, sorry it took me so long to get to this.
I am having troubles figuring out how to test the returned function, maybe you have ideas?

coveralls · 2018-01-14T14:12:22Z

Coverage decreased (-0.6%) to 99.436% when pulling 9258442 on saintsebastian:932-keep-profile-changes-with-named-profile into 5ed1682 on mozilla:master.

coveralls · 2018-01-14T15:15:40Z

Coverage remained the same at 100.0% when pulling 5ffead1 on saintsebastian:932-keep-profile-changes-with-named-profile into 38f6c23 on mozilla:master.

saintsebastian · 2018-02-17T11:19:25Z

@kumar303 @rpl this is pretty much ready for initial review, and maybe your suggestions for further testing. Let me know if you have any thoughts

rpl · 2018-02-19T18:12:40Z

@saintsebastian To test defaultCreateProfileFinder it seems that we can inject a fake FxProfile (and userDirectoryPath for the test case that tests the behavior when a custom user directory path has been specified) when we call the function, then we can call the function returned by defaultCreateProfileFinder and verify the internal behavior of that function by checking that the sinon spies the we include in the fake FxProfile have been called (or not called) with the expected parameters.

Inside the returned function there is also a call to fs.stat, and so we have to ensure the in the test we point it out to a temporary test directory, or we can opt to add fs as an additional "injected dependencies" to the defaultCreateProfileFinder method and in the test case redefine it to suit its expected behavior.

e.g. the test case could be something like the following:

// A fake Finder instance.
const fakeFinder = {
  readProfiles: sinon.spy(() => { ... }),
  getPath: sinon.spy(() => { ... }),
}

// A fake Finder constructor which returns the fake Finder instance.
const FakeFinder = sinon.spy(() => fakeFinder);

// Add a fake localUserDirectory static method to the fake Finder constructor.
FakeFinder.locateUserDirectory = sinon.spy(() => "/tmp/fakeUserDir";

// Define the fake FxProfile to inject as the fake dependency into defaultCreateProfileFinder.
const fakeFxProfile = {Finder: sinon.spy(() => fakeFinder)};

// Create a fakeFS object which provides a fake `stat` method (wrapped by a sinon spy).
const fakeFs = ...

const profileFinder = defaultCreateProfileFinder({FxProfile: fakeFxProfile, fs: fakeFs});

const res = await profileFinder("aFakeProfileName");

sinon.assert.calledOnce(FakeFinder.locateUserDirectory)
...

rpl

Hey @saintsebastian,
Thank you so much for coming back to this pull request!

Besides the review comments below, another remaining issue is to reach the full code coverage of the changes and make coveralls happy again :-)

Based on the last coveralls report, it seems that we are just missing a couple of more branches inside the "finder" function that defaultCreateProfileFinder returns:
https://coveralls.io/builds/18638522/source?filename=src%2Ffirefox%2Findex.js#L345

The other test cases looks to be already in the right direction, and you just need to add a couple more, using the same approach that is currently used in the ones that you already have, e.g. something like the following should be able to cover the "resolves to undefined when no profiles.ini file has been found" scenario:

    it('returns a finder that resolves undefined for no profiles.ini',
       async () => {
         const FxProfile = {
           Finder: sinon.spy(() => () => Promise.resolve({})),
         };

         // We can define here the fake readProfiles method that rejects with a ENOENT error (and then in a separate test we can do the same for an error that hasn't the ENOENT code)
         const fakeReadProfiles = sinon.spy(() => {
           return Promise.reject(
             new ErrorWithCode('ENOENT', 'fake ENOENT error'));
         });
         const fakeGetPath = sinon.spy(() => Promise.resolve());
         FxProfile.Finder.prototype.readProfiles = fakeReadProfiles;
         FxProfile.Finder.prototype.getPath = fakeGetPath;
         FxProfile.Finder.prototype.profiles = [{
           Name: 'someName',
         }];

         const userDirectoryPath = '/non/existent/path';

         const getter = firefox.defaultCreateProfileFinder({
           userDirectoryPath,
           FxProfile,
         });

         const res = await getter('someName');
         assert.equal(
           res,
           undefined,
           'Got an undefined result when the profiles.ini file does not exist');

         sinon.assert.called(fakeReadProfiles);
         sinon.assert.notCalled(fakeGetPath);
       });

We should definitely test also the scenario in which "readProfiles" raises an unexpected error (one that isn't an ENOENT).

rpl · 2018-08-24T09:07:59Z

tests/unit/test-firefox/test.firefox.js

+            createProfileFinder,
          });
+          assert.instanceOf(profile, FirefoxProfile);
+        } catch (error) {


This try...catch is just re-throwing the caught error, it doesn't seem to be needed (same for the other ones in the other new tests below).

rpl · 2018-08-24T09:13:14Z

src/firefox/index.js

+        return await getPath(profileName);
+      }
+    } catch (error) {
+      if (isErrorWithCode('ENOENT', error)) {


Do you mind to reverse this if and remove the else block?

if (!isErrorWithCode('ENOENT', error)) { throw error; } log.warn('...');

Also, I'm thinking that we should tweak the warning message a bit, e.g.

log.warn('Unable to find Firefox profiles.ini');

saintsebastian · 2018-08-25T12:29:20Z

@rpl thanks for the review, i think it really helped! what do you think of those tests?

rpl

@saintsebastian Thanks again for working on this!

I'd like to also test this manually a bit (to double-check that we didn't missed some scenarios with the unit tests), and I'm tempted to say that it would be nice to have some tests that don't mock the Finder class defined in the npm dependency "firefox-profile", especially because we are unit testing it with a number of sinon spies and so we are making a number of assumptions about how the Finder class defined in the npm dependency "firefox-profile" works, and some tests without a mocked Finder class could help us to caught issues with changes in the npm dependency.

If I recall correctly the purpose of the userDirectoryPath (which is never used by the firefox.useProfile method) is exactly to allow us to test some scenarios without a mock of the Finder class (by writing a test that creates a "profiles.ini" file into a temp dir and check that the real Finder class does what it should).

In the meantime, I've collected some additional review comments that I collected by re-looking at the whole change.

rpl · 2018-08-27T19:54:35Z

src/firefox/index.js

  return Promise.resolve(profile);
 }

+export type getProfileFN = (profileName: string) => Promise<string | void>;


@saintsebastian sorry I didn't notice this before, other function types names are suffixed using Fn instead of FN, do you mind to rename this one into getProfileFn?

rpl · 2018-08-27T19:59:31Z

src/firefox/index.js

+  let destinationDirectory;
+  const getProfilePath = createProfileFinder();
+
+  const profileIsAPath = await isDirectory(profilePath);


Nit, do you mind to rename this to profileIsDirPath or isDirPath?

rpl · 2018-08-27T20:03:25Z

tests/unit/test-firefox/test.firefox.js

         );
       });

+    it('rejects to a UsageError when profle is not found',


small typo: profle => profile

rpl · 2018-08-27T20:07:29Z

tests/unit/test-firefox/test.firefox.js

+           () => Promise.resolve(false)
+         );
+         let exception;
+         try {


@saintsebastian do you mind to rewrite this assertion to use the assert.isRejected instead of the try...catch? as we do here:

web-ext/tests/unit/test-util/test.adb.js

Lines 106 to 109 in 1cd88a1

const promise = testFn(adbUtils);

await assert.isRejected(promise, UsageError);

await assert.isRejected(promise, /No adb executable has been found/);

It should require just a small tweak on these few lines.

rpl · 2018-08-27T20:14:27Z

tests/unit/test-firefox/test.firefox.js

    it('resolves to a FirefoxProfile instance', () => withBaseProfile(
-      (baseProfile) => {
+      async (baseProfile) => {
+        const app = 'fennec';


why `const app = 'fennec' is being added to these tests?
it shouldn't be needed.

rpl · 2018-08-27T21:20:27Z

tests/unit/test-firefox/test.firefox.js

+
+    it('creates a finder', async () => {
+      const FxProfile = {
+        Finder: sinon.spy(() => () => Promise.resolve()),


Finder is a constructor function and so it seems that its spy should be something like:

const FxProfile = { Finder: sinon.spy(function () { return {}; }), };

rpl · 2018-08-27T21:25:43Z

tests/unit/test-firefox/test.firefox.js

+      sinon.assert.called(FxProfile.Finder);
+      sinon.assert.calledWith(
+        FxProfile.Finder,
+        sinon.notOk,


This sinon.assert.calledwith(FxProfile.Finder, sinon.notOk); assertion doesn't seem right (or it is not very readable), shouldn't it be something like the following?

firefox.defaultCreateProfileFinder({FxProfile}); sinon.assert.calledWith(FxProfile.Finder, sinon.match(undefined));

rpl · 2018-08-27T21:29:00Z

tests/unit/test-firefox/test.firefox.js

+        Finder: sinon.spy(() => () => Promise.resolve()),
+      };
+      const userDirectoryPath = '/non/existent/path';
+


Nit, remove this empty line.

rpl · 2018-08-27T21:31:21Z

tests/unit/test-firefox/test.firefox.js

+      );
+    });
+
+    it('returns a finder that looks for a default profile', async () => {


uhm... I think that the description of this test should probably be 'returns a finder that resolves a profile name' or something like that (instead of 'looks for a default profile').

rpl · 2018-08-27T21:32:56Z

tests/unit/test-firefox/test.firefox.js

+         sinon.assert.notCalled(fakeGetPath);
+       });
+
+    it('returns a finder that throws other errors',


Nit, s/other errors/unexpected errors/

saintsebastian · 2018-08-30T19:30:59Z

@rpl thanks for the thorough as usual review! I think this is pretty close to done

rpl

Thanks @saintsebastian!
This looks great to me, I also tried it a bit locally and it is working pretty well.

Follows a couple of small nits related to the tests, it should be just a matter of applying a couple of small changes, and then we can merge this PR \o/

tests/unit/test-firefox/test.firefox.js

rpl · 2018-09-18T15:39:23Z

@saintsebastian I forgot to mention: don't worry about the failure on appveyor, it is an unrelated (and pretty annoying to be honest) intermittent that is happening (too often :-( ) on the windows CI service.

…).

rpl · 2018-10-29T18:34:38Z

This PR has been merged as c85aa6e (rebased and squashed version of the patches in this PR)
and 7d8b703 (which contains the small tweaks suggested in the last round of review comments).

saintsebastian added 2 commits January 13, 2018 23:05

feat: support of named profiles

6425fae

tests: new tests fro useProfile

3d4bdc2

saintsebastian force-pushed the 932-keep-profile-changes-with-named-profile branch 3 times, most recently from eb8c173 to 4bef586 Compare January 13, 2018 23:06

saintsebastian force-pushed the 932-keep-profile-changes-with-named-profile branch from 4bef586 to 9258442 Compare January 14, 2018 14:07

tests: defaultCreateProfileFinder tests

cada5be

saintsebastian force-pushed the 932-keep-profile-changes-with-named-profile branch from 9258442 to cada5be Compare January 14, 2018 15:09

This was referenced Apr 27, 2018

fix: Can't run with specified profile name and --keep-profile-changes #968

Closed

Using a default profile with --keep-profile-changes will break Firefox #1005

Closed

saintsebastian added 2 commits August 5, 2018 16:11

fix: delete package-lock

e315072

test: returned getter test

b661fc1

rpl requested changes Aug 24, 2018

View reviewed changes

test: result finder tests

f94b9e5

saintsebastian force-pushed the 932-keep-profile-changes-with-named-profile branch from 07e7926 to f94b9e5 Compare August 24, 2018 18:32

rpl requested changes Aug 27, 2018

View reviewed changes

saintsebastian force-pushed the 932-keep-profile-changes-with-named-profile branch from 1ab754d to 3df74fa Compare August 30, 2018 19:18

fix: review comments

5ffead1

saintsebastian force-pushed the 932-keep-profile-changes-with-named-profile branch from 3df74fa to 5ffead1 Compare August 30, 2018 19:20

rpl approved these changes Sep 17, 2018

View reviewed changes

tests/unit/test-firefox/test.firefox.js Show resolved Hide resolved

tests/unit/test-firefox/test.firefox.js Show resolved Hide resolved

rpl mentioned this pull request Sep 17, 2018

Broken loading with existing Firefox profile (race condition) #1363

Open

rpl pushed a commit to rpl/web-ext that referenced this pull request Oct 29, 2018

feat: support of named profiles (mozilla#1149)

c85aa6e

rpl added a commit to rpl/web-ext that referenced this pull request Oct 29, 2018

chore: Applied minor tweaks on test.firefox.js unit tests (mozilla#1149…

7d8b703

…).

rpl closed this Oct 29, 2018

	const promise = testFn(adbUtils);

	await assert.isRejected(promise, UsageError);
	await assert.isRejected(promise, /No adb executable has been found/);

Conversation

saintsebastian commented Dec 10, 2017

Uh oh!

coveralls commented Dec 10, 2017

Uh oh!

kumar303 commented Dec 10, 2017

Uh oh!

coveralls commented Jan 13, 2018

Uh oh!

coveralls commented Jan 13, 2018

Uh oh!

saintsebastian commented Jan 13, 2018

Uh oh!

coveralls commented Jan 14, 2018

Uh oh!

coveralls commented Jan 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saintsebastian commented Feb 17, 2018

Uh oh!

rpl commented Feb 19, 2018

Uh oh!

rpl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saintsebastian commented Aug 25, 2018

Uh oh!

rpl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saintsebastian commented Aug 30, 2018

Uh oh!

rpl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rpl commented Sep 18, 2018

Uh oh!

rpl commented Oct 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented Jan 14, 2018 •

edited

Loading