-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[Reporting] Log the chromium child process PID #44947
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,80 +13,79 @@ import { | |
| ConsoleMessage, | ||
| Request as PuppeteerRequest, | ||
| } from 'puppeteer'; | ||
| import { ChildProcess } from 'child_process'; | ||
| import rimraf from 'rimraf'; | ||
| import * as Rx from 'rxjs'; | ||
| import { ignoreElements, mergeMap, tap } from 'rxjs/operators'; | ||
| import { InnerSubscriber } from 'rxjs/internal/InnerSubscriber'; | ||
|
|
||
| import { puppeteerLaunch } from '../puppeteer'; | ||
| import { BrowserConfig } from '../../../../types'; | ||
| import { LevelLogger as Logger } from '../../../lib/level_logger'; | ||
| import { HeadlessChromiumDriver } from '../driver'; | ||
| import { args, IArgOptions } from './args'; | ||
| import { safeChildProcess } from '../../safe_child_process'; | ||
| import { puppeteerLaunch } from '../puppeteer'; | ||
| import { getChromeLogLocation } from '../paths'; | ||
| import { args } from './args'; | ||
|
|
||
| type binaryPath = string; | ||
| type queueTimeout = number; | ||
| interface IBrowserConfig { | ||
| [key: string]: any; | ||
| } | ||
|
|
||
| export class HeadlessChromiumDriverFactory { | ||
| private binaryPath: binaryPath; | ||
| private logger: Logger; | ||
| private browserConfig: IBrowserConfig; | ||
| private browserConfig: BrowserConfig; | ||
| private queueTimeout: queueTimeout; | ||
|
|
||
| constructor( | ||
| binaryPath: binaryPath, | ||
| logger: Logger, | ||
| browserConfig: IBrowserConfig, | ||
| browserConfig: BrowserConfig, | ||
| queueTimeout: queueTimeout | ||
| ) { | ||
| this.binaryPath = binaryPath; | ||
| this.browserConfig = browserConfig; | ||
| this.queueTimeout = queueTimeout; | ||
| this.logger = logger; | ||
| this.logger = logger; // TODO: just pass logger into each method from outside, like test() has it | ||
| } | ||
|
|
||
| type = 'chromium'; | ||
|
|
||
| test( | ||
| { viewport, browserTimezone }: { viewport: IArgOptions['viewport']; browserTimezone: string }, | ||
| logger: Logger | ||
| ) { | ||
| test({ viewport }: { viewport: BrowserConfig['viewport'] }, logger: Logger) { | ||
| const userDataDir = fs.mkdtempSync(path.join(os.tmpdir(), 'chromium-')); | ||
| const chromiumArgs = args({ | ||
| userDataDir, | ||
| viewport, | ||
| verboseLogging: true, | ||
| disableSandbox: this.browserConfig.disableSandbox, | ||
| proxyConfig: this.browserConfig.proxy, | ||
| proxy: this.browserConfig.proxy, | ||
| }); | ||
|
|
||
| return puppeteerLaunch({ | ||
| userDataDir, | ||
| executablePath: this.binaryPath, | ||
| ignoreHTTPSErrors: true, | ||
| args: chromiumArgs, | ||
| env: { | ||
| TZ: browserTimezone, | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| }, | ||
| } as LaunchOptions).catch((error: Error) => { | ||
| logger.error( | ||
| `The Reporting plugin encountered issues launching Chromium in a self-test. You may have trouble generating reports.` | ||
| ); | ||
| logger.error(error); | ||
| logger.warning(`See Chromium's log output at "${getChromeLogLocation(this.binaryPath)}"`); | ||
| return null; | ||
| }); | ||
| } as LaunchOptions) | ||
| .then((browser: Browser) => { | ||
| const childProcess: ChildProcess = browser.process(); | ||
| logger.debug(`Test browser process launched with PID: ${childProcess.pid}`); | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is 1/2 of the actual changes. Everything else is due to increase strictness of Typescript |
||
| return browser; | ||
| }) | ||
| .catch((error: Error) => { | ||
| logger.error( | ||
| `The Reporting plugin encountered issues launching Chromium in a self-test. You may have trouble generating reports.` | ||
| ); | ||
| logger.error(error); | ||
| logger.warning(`See Chromium's log output at "${getChromeLogLocation(this.binaryPath)}"`); | ||
| return null; | ||
| }); | ||
| } | ||
|
|
||
| create({ | ||
| viewport, | ||
| browserTimezone, | ||
| }: { | ||
| viewport: IArgOptions['viewport']; | ||
| viewport: BrowserConfig['viewport']; | ||
| browserTimezone: string; | ||
| }): Rx.Observable<{ | ||
| driver$: Rx.Observable<HeadlessChromiumDriver>; | ||
|
|
@@ -101,7 +100,7 @@ export class HeadlessChromiumDriverFactory { | |
| viewport, | ||
| verboseLogging: this.logger.isVerbose, | ||
| disableSandbox: this.browserConfig.disableSandbox, | ||
| proxyConfig: this.browserConfig.proxy, | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This might have been hiding a bug. If it is meant to match the Reporting Settings, then it should be |
||
| proxy: this.browserConfig.proxy, | ||
| }); | ||
|
|
||
| let browser: Browser; | ||
|
|
@@ -118,6 +117,10 @@ export class HeadlessChromiumDriverFactory { | |
| }, | ||
| } as LaunchOptions); | ||
|
|
||
| // log the child process PID | ||
| const childProcess: ChildProcess = browser.process(); | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is 2/2 of the actual changes. Everything else is due to increase strictness of Typescript |
||
| this.logger.debug(`Job browser process launched with PID: ${childProcess.pid}`); | ||
|
|
||
| page = await browser.newPage(); | ||
|
|
||
| // All navigation/waitFor methods default to 30 seconds, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a bug to wrap this in a conditional. I think we want the logging flags hardcoded to "on":
https://www.chromium.org/for-testers/enable-logging