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.

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

feat: support of named profiles #1149

Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
57 changes: 56 additions & 1 deletion src/firefox/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -317,13 +317,48 @@ export function configureProfile(
return Promise.resolve(profile);
}

export type getProfileFN = (profileName: string) => Promise<string | void>;
Copy link
Member

Choose a reason for hiding this comment

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

@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?


export type CreateProfileFinderParams = {|
userDirectoryPath?: string,
FxProfile?: typeof FirefoxProfile
|}

export function defaultCreateProfileFinder(
{
userDirectoryPath,
FxProfile = FirefoxProfile,
}: CreateProfileFinderParams = {}
): getProfileFN {
const finder = new FxProfile.Finder(userDirectoryPath);
const readProfiles = promisify(finder.readProfiles, finder);
const getPath = promisify(finder.getPath, finder);
return async (profileName: string): Promise<string | void> => {
try {
await readProfiles();
const hasProfileName = finder.profiles.filter(
(profileDef) => profileDef.Name === profileName).length !== 0;
if (hasProfileName) {
return await getPath(profileName);
}
} 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.

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');

log.warn('No firefox profiles exist');
} else {
throw error;
}
}
};
}

// useProfile types and implementation.

export type UseProfileParams = {
app?: PreferencesAppName,
configureThisProfile?: ConfigureProfileFn,
isFirefoxDefaultProfile?: IsDefaultProfileFn,
customPrefs?: FirefoxPreferences,
createProfileFinder?: typeof defaultCreateProfileFinder,
};

// Use the target path as a Firefox profile without cloning it
Expand All @@ -335,6 +370,7 @@ export async function useProfile(
configureThisProfile = configureProfile,
isFirefoxDefaultProfile = isDefaultProfile,
customPrefs = {},
createProfileFinder = defaultCreateProfileFinder,
}: UseProfileParams = {},
): Promise<FirefoxProfile> {
const isForbiddenProfile = await isFirefoxDefaultProfile(profilePath);
Expand All @@ -346,7 +382,26 @@ export async function useProfile(
'\nSee https://github.com/mozilla/web-ext/issues/1005'
);
}
const profile = new FirefoxProfile({destinationDirectory: profilePath});

let destinationDirectory;
const getProfilePath = createProfileFinder();

const profileIsAPath = await isDirectory(profilePath);
Copy link
Member

Choose a reason for hiding this comment

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

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

if (profileIsAPath) {
log.debug(`Using profile directory "${profilePath}"`);
destinationDirectory = profilePath;
} else {
log.debug(`Assuming ${profilePath} is a named profile`);
destinationDirectory = await getProfilePath(profilePath);
if (!destinationDirectory) {
throw new UsageError(
`The request "${profilePath}" profile name ` +
'cannot be resolved to a profile path'
);
}
}

const profile = new FirefoxProfile({destinationDirectory});
return await configureThisProfile(profile, {app, customPrefs});
}

Expand Down
201 changes: 195 additions & 6 deletions tests/unit/test-firefox/test.firefox.js
Original file line number Diff line number Diff line change
Expand Up @@ -511,20 +511,136 @@ describe('firefox', () => {
} catch (error) {
exception = error;
}

assert.instanceOf(exception, UsageError);
assert.match(
exception && exception.message,
/Cannot use --keep-profile-changes on a default profile/
);
});

it('rejects to a UsageError when profle is not found',
Copy link
Member

Choose a reason for hiding this comment

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

small typo: profle => profile

async () => {
const fakeGetProfilePath = sinon.spy(() => Promise.resolve(false));
const createProfileFinder = () => {
return fakeGetProfilePath;
};
const isFirefoxDefaultProfile = sinon.spy(
() => Promise.resolve(false)
);
let exception;
try {
Copy link
Member

Choose a reason for hiding this comment

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

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

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.

await firefox.useProfile('profileName', {
createProfileFinder,
isFirefoxDefaultProfile,
});
} catch (error) {
exception = error;
}
assert.instanceOf(exception, UsageError);
assert.match(
exception && exception.message,
/The request "profileName" profile name cannot be resolved/
);
}
);

it('resolves to a FirefoxProfile instance', () => withBaseProfile(
(baseProfile) => {
const configureThisProfile = (profile) => Promise.resolve(profile);
return firefox.useProfile(baseProfile.path(), {configureThisProfile})
.then((profile) => {
assert.instanceOf(profile, FirefoxProfile);
async (baseProfile) => {
try {
const app = 'fennec';
const configureThisProfile = (profile) => Promise.resolve(profile);
const createProfileFinder = () => {
return (profilePath) => Promise.resolve(profilePath);
};
const profile = await firefox.useProfile(baseProfile.path(), {
app,
configureThisProfile,
createProfileFinder,
});
assert.instanceOf(profile, FirefoxProfile);
} 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.

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).

throw error;
}
}
));

it('looks for profile path if passed a name', () => withBaseProfile(
async (baseProfile) => {
try {
const app = 'fennec';
const fakeGetProfilePath = sinon.spy(() => baseProfile.path());
const createProfileFinder = () => {
return fakeGetProfilePath;
};
const isFirefoxDefaultProfile = sinon.spy(
() => Promise.resolve(false)
);
await firefox.useProfile('profileName', {
app,
createProfileFinder,
isFirefoxDefaultProfile,
});
sinon.assert.calledOnce(fakeGetProfilePath);
sinon.assert.calledWith(
fakeGetProfilePath,
sinon.match('profileName')
);
} catch (error) {
throw error;
}
}
));

it('checks if named profile is default', () => withBaseProfile(
Copy link
Member

Choose a reason for hiding this comment

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

This test and the one below ('checks if path leads to default profile') seems to be testing the same scenario (especially given that isFirefoxDefaultProfile is called before we actually check if the profilePath parameter is a named profile or a path to a profile directory).

We should be able to remove one of these two tests and the code coverage should stay on the same percentage.

@saintsebastian what do you think?

async (baseProfile) => {
try {
const app = 'fennec';
const createProfileFinder = () => {
return () => Promise.resolve(baseProfile.path());
};
const isFirefoxDefaultProfile = sinon.spy(
() => Promise.resolve(false)
);
await firefox.useProfile('profileName', {
app,
createProfileFinder,
isFirefoxDefaultProfile,
});
sinon.assert.calledOnce(isFirefoxDefaultProfile);
sinon.assert.calledWith(
isFirefoxDefaultProfile,
sinon.match('profileName')
);
} catch (error) {
throw error;
}
}
));

it('checks if path leads to default profile', () => withBaseProfile(
async (baseProfile) => {
try {
const app = 'fennec';
const profilePath = baseProfile.path();
const createProfileFinder = () => {
return () => Promise.resolve(profilePath);
};
const isFirefoxDefaultProfile = sinon.spy(
() => Promise.resolve(false)
);
await firefox.useProfile(profilePath, {
app,
createProfileFinder,
isFirefoxDefaultProfile,
});
sinon.assert.calledOnce(isFirefoxDefaultProfile);
sinon.assert.calledWith(
isFirefoxDefaultProfile,
sinon.match(profilePath)
);
} catch (error) {
throw error;
}
}
));

Expand All @@ -545,6 +661,79 @@ describe('firefox', () => {

});

describe('defaultCreateProfileFinder', () => {

it('creates a finder', async () => {
try {
const FxProfile = {
Finder: sinon.spy(() => () => Promise.resolve()),
};
firefox.defaultCreateProfileFinder({FxProfile});
sinon.assert.called(FxProfile.Finder);
sinon.assert.calledWith(
FxProfile.Finder,
sinon.notOk,
);
} catch (error) {
throw error;
}
});

it('creates finder based on userDirectoryPath if present', async () => {
try {
const FxProfile = {
Finder: sinon.spy(() => () => Promise.resolve()),
};
const userDirectoryPath = '/non/existent/path';

Copy link
Member

Choose a reason for hiding this comment

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

Nit, remove this empty line.

firefox.defaultCreateProfileFinder({userDirectoryPath, FxProfile});

sinon.assert.called(FxProfile.Finder);
sinon.assert.calledWith(
FxProfile.Finder,
sinon.match(userDirectoryPath),
);
} catch (error) {
throw error;
}
});

it('returns a function that looks for a default profile', async () => {
try {
const FxProfile = {
Finder: sinon.spy(() => () => Promise.resolve({})),
};
const fakeReadProfiles = sinon.spy(() => Promise.resolve());
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,
});

await getter('someName');

sinon.assert.called(fakeReadProfiles);
sinon.assert.called(fakeGetPath);
sinon.assert.calledWith(
fakeGetPath,
sinon.match('someName'),
);
} catch (error) {
throw error;
}
});


});

describe('configureProfile', () => {

function withTempProfile(callback) {
Expand Down