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

feat: allow port option when run with target chromium #3188

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

feat: allow port option when run with target chromium #3188

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
219b32b
feat: Allow port option when launching Chrome
dever23b Jul 8, 2024
01d737c
Fixed code styling
dever23b Jul 8, 2024
da9dd76
Fixed tester
dever23b Jul 8, 2024
8b9a5e4
feat: add new Errors
dever23b Jul 10, 2024
9eed3b6
feat: add port validation
dever23b Jul 10, 2024
5355435
cleanup
dever23b Jul 11, 2024
62628b4
fix: move port validation higher in the flow to prevent unnecessary w…
dever23b Jul 11, 2024
c8cae1e
cleanup
dever23b Jul 12, 2024
7c1eeda
feat: allow test fake process to inherit port
dever23b Jul 12, 2024
d0cdd3e
style: code formatting
dever23b Jul 12, 2024
bd410f4
style: renamed error for simplicity
dever23b Jul 12, 2024
41ad45a
fix: remove extraneous logging, use renamed Error
dever23b Jul 12, 2024
1f9754f
fix: remove extended process validation
dever23b Jul 12, 2024
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
2 changes: 2 additions & 0 deletions src/cmd/run.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ export default async function run(
firefoxApkComponent,
// Chromium CLI options.
chromiumBinary,
chromiumPort,
chromiumProfile,
},
{
Expand Down Expand Up @@ -186,6 +187,7 @@ export default async function run(
const chromiumRunnerParams = {
...commonRunnerParams,
chromiumBinary,
chromiumPort,
chromiumProfile,
};

Expand Down
7 changes: 7 additions & 0 deletions src/extension-runners/chromium.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,12 @@ export class ChromiumExtensionRunner {
chromeFlags.push(...startingUrls);
}

let port;
if (this.params.chromiumPort && !isNaN(this.params.chromiumPort)) {
port = this.params.chromiumPort;
log.debug(`(port: ${port})`);
}

this.chromiumInstance = await this.chromiumLaunch({
enableExtensions: true,
chromePath: chromiumBinary,
Expand All @@ -210,6 +216,7 @@ export class ChromiumExtensionRunner {
userDataDir,
// Ignore default flags to keep the extension enabled.
ignoreDefaultFlags: true,
port,
});

this.chromiumInstance.process.once('close', () => {
Expand Down
17 changes: 17 additions & 0 deletions tests/unit/test-extension-runners/test.chromium.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -615,6 +615,23 @@ describe('util/extension-runners/chromium', async () => {
}),
);

it('does pass port to chrome', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

This unit test only tests the good-weather behavior. You should also test the bad weather behavior, i.e. the error conditions.

Here are the relevant cases:

  • port is available.
  • port is clearly invalid, e.g. -1 and 99999
  • edge case: port 0. Ordinarily this means to select a random port.
  • port is unavailable, which the test can guarantee by reserving a port.

const port = 9222;
Copy link
Member

Choose a reason for hiding this comment

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

This unit test will fail if 9222 is used at any point. You should choose a free port here.

Note that we already have a utility to find a free port, at

web-ext/src/firefox/remote.js

Line 233 in 1c01518

export function findFreeTcpPort() {

const { params } = prepareExtensionRunnerParams({
params: { chromiumPort: port },
});

const runnerInstance = new ChromiumExtensionRunner(params);
await runnerInstance.run();

sinon.assert.calledOnce(params.chromiumLaunch);
sinon.assert.calledWithMatch(params.chromiumLaunch, {
port,
});

await runnerInstance.exit();
});

describe('reloadAllExtensions', () => {
let runnerInstance;
let wsClient;
Expand Down