Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 17 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
e7712a0
Modified useProfile()
harikishen Jun 8, 2017
0af91f6
Modified useProfile()
harikishen Jun 8, 2017
9b7ce5a
Fix #932 - Modified useProfile() to check for profile paths
harikishen Jun 8, 2017
939a2a3
Promisified finder.getPath()
harikishen Jun 14, 2017
2be9919
Promisified finder.getPath()
harikishen Jun 14, 2017
806f9e0
Moved code from util/finder.js to firefox/index.js
harikishen Jun 14, 2017
854b09c
Added tests for useProfile()
harikishen Jun 15, 2017
5876d14
Added test for named profile
harikishen Jun 16, 2017
9fc5e9d
Removed readProfiles() call
harikishen Jun 16, 2017
8c333de
Changes to finder
harikishen Jun 16, 2017
c09b22e
Changes to finder
harikishen Jun 16, 2017
1709154
Adding further tests.
harikishen Jun 19, 2017
ac9a6b2
Fixed tests
harikishen Jun 20, 2017
232e1e5
Refactored tests for useProfile()
harikishen Jun 20, 2017
fdff679
Adding tests for defaultCreateProfileFinder()
harikishen Jun 20, 2017
bfb1703
Added tests for defaultCreateProfileFinder()
harikishen Jun 22, 2017
4c4d407
Modified to combine hasProfileName and getPath
harikishen Jun 24, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 67 additions & 1 deletion src/firefox/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,11 @@ export interface FirefoxProcess extends events$EventEmitter {
kill: Function;
}

export interface IProfileFinder {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This flow type is currently unused and it should be used for the defaultCreateProfileFinder return value type annotation (we should also move it near that function, that is where it is going to be used).

Also, the type signatures are wrong: both hasProfileName and getPath take a string as parameter
(and so they should be hasProfileName(string): Promise<boolean>, and getPath needs the same kind of fix).

hasProfileName(): Promise<boolean>;
getPath(): Promise<string>;
}

export type FirefoxRunnerResults = {|
process: FirefoxProcess,
binary: string,
Expand Down Expand Up @@ -239,8 +244,34 @@ export type UseProfileParams = {
app?: PreferencesAppName,
configureThisProfile?: ConfigureProfileFn,
customPrefs?: FirefoxPreferences,
createProfileFinder?: typeof defaultCreateProfileFinder,
};

export function defaultCreateProfileFinder() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the new tests are using a fake createProfileFinder, and so (like the code coverage reports confirm) it is time to create some tests specifically for defaultCreateProfileFinder).

To make this function more easily testable we should add an optional userDirectoryPath (a string) parameter to it, so that we can customize the directory where the Finder class will search the profiles.ini file and the profile directories.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what tests do you propose ?

const finder = new FirefoxProfile.Finder();
const readProfiles = promisify(finder.readProfiles, finder);
return {
getPath: promisify(finder.getPath, finder),
hasProfileName: async (profileName: string) => {
try {
await fs.stat(path.join(FirefoxProfile.Finder.locateUserDirectory(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@harikishen the tests are failing because on an issue in the hasProfileName implementation:

  • while we are configuring the Finder to use the specified parameter as the profiles search path (as expected)
    at line 251, here we are always doing an fs.stat on the default search path the FirefoxProfile.Finder.locateUserDirectory() provides (but it should use the same path that the Finder is going to use

And so it should be something like:

        const profilesIniPath = path.join(
          userDirectoryPath || FirefoxProfile.Finder.locateUserDirectory(),
          'profiles.ini'
        );
        const res = await fs.stat(profilesIniPath);

'profile.ini'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"profiles.ini"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I was trying the case if the file did not exist... forgot to fix it

} catch (error) {
if (isErrorWithCode('ENOENT', error)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This try catch should only surround the await fs.stat call and it should re-throw any error that does not verify isErrorWithCode('ENOENT', error) (or, on the contrary, this function is going to return undefined on all the other kind of exception that can be caught from this try...catch).

log.info('No firefox profiles exist');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, we have already checked that "profiles.ini" doesn't exist (which should probably be a warning instead of an info message), but we are going to execute await readProfiles() at line 266, which will raises an exception because of that.

And so I think that it would be reasonable to return false after logging a warning (no profile name can be found if no "profiles.ini" has been found).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the call for warning...???

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unsurprisingly log.warn :-) (

web-ext/tests/unit/helpers.js

Line 174 in 97404a9

log.warn(`Running stubbed function ${key} (default implementation)`);
)

} else {
throw error;
}
}
await readProfiles();
return finder.profiles.filter(
(profileDef) => profileDef.Name === profileName
).length !== 0;
},
};
}


// Use the target path as a Firefox profile without cloning it

export async function useProfile(
Expand All @@ -249,9 +280,44 @@ export async function useProfile(
app,
configureThisProfile = configureProfile,
customPrefs = {},
createProfileFinder = defaultCreateProfileFinder,
}: UseProfileParams = {},
): Promise<FirefoxProfile> {
const profile = new FirefoxProfile({destinationDirectory: profilePath});
let profile;
const finder = createProfileFinder();
try {
const dirExists = await isDirectory(profilePath);
const defaultProfilePath = (
finder.hasProfileName('default') && await finder.getPath('default')
);
const defaultDevProfilePath = (
finder.hasProfileName('dev-edition-default') &&
await finder.getPath('dev-edition-default')
);
if (dirExists) {
log.debug(`Using profile directory from "${profilePath}"`);
if (profilePath === defaultProfilePath ||
profilePath === defaultDevProfilePath) {
throw new WebExtError(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By looking at the coverage reports, this line doesn't seem to be currently executed by any of the current tests.

`Cannot use profile at "${profilePath}"`
);
}
profile = new FirefoxProfile({destinationDirectory: profilePath});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can make this function to call const profile = new FirefoxProfile(...) in a single place by limiting this block and the one that contains line 317 to only "resolve the final destinationDirectory value", and then creating the FirefoxProfile instance at the end of this function.

} else {
log.debug(`Assuming ${profilePath} is a named profile`);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we should make this to always fail if the requested profile name is default or dev-edition-default.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rpl what do you propose ?

Copy link
Member

@rpl rpl Jun 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something like:

if (profilePath === 'default' || profilePath === 'dev-edition-default') {
  throw new WebExtError(`Cannot use the blacklisted named profile "${profilePath}" `);
}

if (profilePath === 'default' ||
profilePath === 'dev-edition-default') {
throw new WebExtError(
`Cannot use the blacklisted named profile "${profilePath}"`
);
}
const profileDirectory = await finder.getPath(profilePath);
profile = new FirefoxProfile({destinationDirectory: profileDirectory});
}
} catch (error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that this giant try...catch is actually useful, at least by looking currently we can just leave any other exception to be propagated to the callers.

throw new WebExtError(
`Could not use Firefox profile from ${profilePath}: ${error}`);
}
return await configureThisProfile(profile, {app, customPrefs});
}

Expand Down
143 changes: 143 additions & 0 deletions tests/unit/test-firefox/test.firefox.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,149 @@ describe('firefox', () => {
}
));

it('configures a named profile', () => withTempDir(
(tmpDir) => {
const configureProfile =
sinon.spy((profile) => Promise.resolve(profile));
const app = 'fennec';
const profilePath = tmpDir.path();
const profileFinder = () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@harikishen profileFinder is a function that creates the fake Finder objects, and not a Finder object itself, that is the reason why the assertion commented on line 351 is not working for you.

You should do something like:

const profileFinder = {
  getPath: sinon.spy(...),
  ...
};

const createProfileFinder = () => profileFinder;

const profile = await firefox.useProfile(profilePath, {
   ...
   configureThisProfile: configureProfile,
   createProfileFinder: createProfileFinder,
});

assert.equal(configureProfile.called, true);
...
assert.equal(profileFinder.getPath.callCount, 2)

return {
getPath: sinon.spy((pathToProfile) =>
Promise.resolve(pathToProfile)),
hasProfileName: () => Promise.resolve(true),
};
};
const spy = sinon.spy(profileFinder, "getPath");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@harikishen this is probably a leftover, but it is making the CI to fail on the eslint checks and it never reaches the unit tests:

return firefox.useProfile(profilePath,
{
app: 'fennec',
configureThisProfile: configureProfile,
createProfileFinder: profileFinder,
})
.then((profile) => {
assert.equal(configureProfile.called, true);
assert.equal(configureProfile.firstCall.args[0], profile);
assert.equal(configureProfile.firstCall.args[1].app, app);
// assert.equal(profileFinder.getPath.callCount, 2)
});
}
));

it('does not configure named profile default', () => {
const configureProfile =
sinon.spy((profile) => Promise.resolve(profile));
const profileFinder = () => {
return {
getPath: () => Promise.resolve(),
hasProfileName: () => Promise.resolve(true),
};
};
return firefox.useProfile('default',
{
app: 'fennec',
configureThisProfile: configureProfile,
createProfileFinder: profileFinder,
})
.then((profile) => {
assert.equal(configureProfile.called, true);
assert.equal(configureProfile.firstCall.args[0], profile);
assert.equal(configureProfile.firstCall.args[1].app, 'fennec');
})
.catch((error) => {
assert.instanceOf(error, WebExtError);
assert.match(error.message,
/Cannot use the blacklisted named profile "default"+/);
});
});

it('does not configure named profile dev-edition-default', () => {
const configureProfile =
sinon.spy((profile) => Promise.resolve(profile));
const profileFinder = () => {
return {
getPath: () => Promise.resolve(),
hasProfileName: () => Promise.resolve(true),
};
};
return firefox.useProfile('dev-edition-default',
{
app: 'fennec',
configureThisProfile: configureProfile,
createProfileFinder: profileFinder,
})
.then((profile) => {
assert.equal(configureProfile.called, true);
assert.equal(configureProfile.firstCall.args[0], profile);
assert.equal(configureProfile.firstCall.args[1].app, 'fennec');
})
.catch((error) => {
assert.instanceOf(error, WebExtError);
assert.match(error.message,
/Cannot use the blacklisted named profile "dev-edition-default"+/);
});
});

it('does not configure profile at default', () => withTempDir(
(tmpDir) => {
const configureProfile =
sinon.spy((profile) => Promise.resolve(profile));
const defaultPath = path.join(tmpDir.path(), 'fake-profile.default');
const profileFinder = () => {
return {
getPath: (profilePath) => Promise.resolve(profilePath),
hasProfileName: () => Promise.resolve(true),
};
};
return firefox.useProfile(defaultPath,
{
app: 'fennec',
configureThisProfile: configureProfile,
createProfileFinder: profileFinder,
})
.then((profile) => {
assert.equal(configureProfile.called, true);
assert.equal(configureProfile.firstCall.args[0], profile);
assert.equal(configureProfile.firstCall.args[1].app, 'fennec');
})
.catch((error) => {
assert.instanceOf(error, WebExtError);
assert.match(error.message,
/Cannot use profile at+/);
});
}
));

it('does not configure profile at dev-edition-default', () => withTempDir(
(tmpDir) => {
const configureProfile =
sinon.spy((profile) => Promise.resolve(profile));
const defaultDevPath = path.join(tmpDir.path(),
'fake-profile.dev-edition-default');
const profileFinder = () => {
return {
getPath: (profilePath) => Promise.resolve(profilePath),
hasProfileName: () => Promise.resolve(true),
};
};
return firefox.useProfile(defaultDevPath,
{
app: 'fennec',
configureThisProfile: configureProfile,
createProfileFinder: profileFinder,
})
.then((profile) => {
assert.equal(configureProfile.called, true);
assert.equal(configureProfile.firstCall.args[0], profile);
assert.equal(configureProfile.firstCall.args[1].app, 'fennec');
})
.catch((error) => {
assert.instanceOf(error, WebExtError);
assert.match(error.message,
/Cannot use profile at+/);
});
}
));
});

describe('configureProfile', () => {
Expand Down