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

smoke - fix log and retry exitApplication #160244

Merged
merged 1 commit into from Sep 7, 2022
Merged

smoke - fix log and retry exitApplication #160244

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/vs/workbench/electron-sandbox/window.ts
View file
Open in desktop
Expand Up @@ -682,10 +682,18 @@ export class NativeWindow extends Disposable {

private setupDriver(): void {
const that = this;
let pendingQuit = false;

registerWindowDriver({
async exitApplication(): Promise<void> {
if (pendingQuit) {
that.logService.info('[driver] not handling exitApplication() due to pending quit() call');
return;
}

that.logService.info('[driver] handling exitApplication()');

pendingQuit = true;
return that.nativeHostService.quit();
}
});
Expand Down
10 changes: 5 additions & 5 deletions test/automation/src/application.ts
View file
Open in desktop
Expand Up @@ -69,7 +69,7 @@ export class Application {
}

async restart(options?: { workspaceOrFolder?: string; extraArgs?: string[] }): Promise<void> {
await measureAndLog((async () => {
await measureAndLog(() => (async () => {
await this.stop();
await this._start(options?.workspaceOrFolder, options?.extraArgs);
})(), 'Application#restart()', this.logger);
Expand All @@ -82,7 +82,7 @@ export class Application {
const code = await this.startApplication(extraArgs);

// ...and make sure the window is ready to interact
await measureAndLog(this.checkWindowReady(code), 'Application#checkWindowReady()', this.logger);
await measureAndLog(() => this.checkWindowReady(code), 'Application#checkWindowReady()', this.logger);
}

async stop(): Promise<void> {
Expand Down Expand Up @@ -117,12 +117,12 @@ export class Application {
private async checkWindowReady(code: Code): Promise<void> {

// We need a rendered workbench
await measureAndLog(code.didFinishLoad(), 'Application#checkWindowReady: wait for navigation to be committed', this.logger);
await measureAndLog(code.waitForElement('.monaco-workbench'), 'Application#checkWindowReady: wait for .monaco-workbench element', this.logger);
await measureAndLog(() => code.didFinishLoad(), 'Application#checkWindowReady: wait for navigation to be committed', this.logger);
await measureAndLog(() => code.waitForElement('.monaco-workbench'), 'Application#checkWindowReady: wait for .monaco-workbench element', this.logger);

// Remote but not web: wait for a remote connection state change
if (this.remote) {
await measureAndLog(code.waitForTextContent('.monaco-workbench .statusbar-item[id="status.host"]', undefined, statusHostLabel => {
await measureAndLog(() => code.waitForTextContent('.monaco-workbench .statusbar-item[id="status.host"]', undefined, statusHostLabel => {
this.logger.log(`checkWindowReady: remote indicator text is ${statusHostLabel}`);

// The absence of "Opening Remote" is not a strict
Expand Down
59 changes: 37 additions & 22 deletions test/automation/src/code.ts
View file
Open in desktop
Expand Up @@ -76,19 +76,19 @@ export async function launch(options: LaunchOptions): Promise<Code> {
throw new Error('Smoke test process has terminated, refusing to spawn Code');
}

await measureAndLog(copyExtension(rootPath, options.extensionsPath, 'vscode-notebook-tests'), 'copyExtension(vscode-notebook-tests)', options.logger);
await measureAndLog(() => copyExtension(rootPath, options.extensionsPath, 'vscode-notebook-tests'), 'copyExtension(vscode-notebook-tests)', options.logger);

// Browser smoke tests
if (options.web) {
const { serverProcess, driver } = await measureAndLog(launchPlaywrightBrowser(options), 'launch playwright (browser)', options.logger);
const { serverProcess, driver } = await measureAndLog(() => launchPlaywrightBrowser(options), 'launch playwright (browser)', options.logger);
registerInstance(serverProcess, options.logger, 'server');

return new Code(driver, options.logger, serverProcess);
}

// Electron smoke tests (playwright)
else {
const { electronProcess, driver } = await measureAndLog(launchPlaywrightElectron(options), 'launch playwright (electron)', options.logger);
const { electronProcess, driver } = await measureAndLog(() => launchPlaywrightElectron(options), 'launch playwright (electron)', options.logger);
registerInstance(electronProcess, options.logger, 'electron');

return new Code(driver, options.logger, electronProcess);
Expand Down Expand Up @@ -140,7 +140,7 @@ export class Code {
}

async exit(): Promise<void> {
return measureAndLog(new Promise<void>(resolve => {
return measureAndLog(() => new Promise<void>(resolve => {
const pid = this.mainProcess.pid!;

let done = false;
Expand All @@ -154,18 +154,39 @@ export class Code {
while (!done) {
retries++;

if (retries === 40) {
this.logger.log('Smoke test exit call did not terminate process after 20s, forcefully exiting the application...');

// no need to await since we're polling for the process to die anyways
treekill(pid, err => {
try {
process.kill(pid, 0); // throws an exception if the process doesn't exist anymore
this.logger.log('Failed to kill Electron process tree:', err?.message);
} catch (error) {
// Expected when process is gone
}
});
switch (retries) {

// after 5 / 10 seconds: try to exit gracefully again
case 10:
case 20: {
this.logger.log('Smoke test exit call did not terminate process after 5-10s, gracefully trying to exit the application again...');
this.driver.exitApplication();
break;
}

// after 20 seconds: forcefully kill
case 40: {
this.logger.log('Smoke test exit call did not terminate process after 20s, forcefully exiting the application...');

// no need to await since we're polling for the process to die anyways
treekill(pid, err => {
try {
process.kill(pid, 0); // throws an exception if the process doesn't exist anymore
this.logger.log('Failed to kill Electron process tree:', err?.message);
} catch (error) {
// Expected when process is gone
}
});

break;
}

// after 30 seconds: give up
case 60: {
done = true;
this.logger.log('Smoke test exit call did not terminate process after 30s, giving up');
resolve();
}
}

try {
Expand All @@ -175,12 +196,6 @@ export class Code {
done = true;
resolve();
}

if (retries === 60) {
done = true;
this.logger.log('Smoke test exit call did not terminate process after 30s, giving up');
resolve();
}
}
})();
}), 'Code#exit()', this.logger);
Expand Down
4 changes: 2 additions & 2 deletions test/automation/src/electron.ts
View file
Open in desktop
Expand Up @@ -55,7 +55,7 @@ export async function resolveElectronConfiguration(options: LaunchOptions): Prom

if (codePath) {
// running against a build: copy the test resolver extension
await measureAndLog(copyExtension(root, extensionsPath, 'vscode-test-resolver'), 'copyExtension(vscode-test-resolver)', logger);
await measureAndLog(() => copyExtension(root, extensionsPath, 'vscode-test-resolver'), 'copyExtension(vscode-test-resolver)', logger);
}
args.push('--enable-proposed-api=vscode.vscode-test-resolver');
const remoteDataDir = `${userDataDir}-server`;
Expand All @@ -65,7 +65,7 @@ export async function resolveElectronConfiguration(options: LaunchOptions): Prom
// running against a build: copy the test resolver extension into remote extensions dir
const remoteExtensionsDir = join(remoteDataDir, 'extensions');
mkdirp.sync(remoteExtensionsDir);
await measureAndLog(copyExtension(root, remoteExtensionsDir, 'vscode-notebook-tests'), 'copyExtension(vscode-notebook-tests)', logger);
await measureAndLog(() => copyExtension(root, remoteExtensionsDir, 'vscode-notebook-tests'), 'copyExtension(vscode-notebook-tests)', logger);
}

env['TESTRESOLVER_DATA_FOLDER'] = remoteDataDir;
Expand Down
6 changes: 3 additions & 3 deletions test/automation/src/logger.ts
View file
Open in desktop
Expand Up @@ -41,15 +41,15 @@ export class MultiLogger implements Logger {
}
}

export async function measureAndLog<T>(promise: Promise<T>, name: string, logger: Logger): Promise<T> {
export async function measureAndLog<T>(promiseFactory: () => Promise<T>, name: string, logger: Logger): Promise<T> {
const now = Date.now();

logger.log(`Starting operation '${name}...`);
logger.log(`Starting operation '${name}'...`);

let res: T | undefined = undefined;
let e: unknown;
try {
res = await promise;
res = await promiseFactory();
} catch (error) {
e = error;
}
Expand Down
16 changes: 8 additions & 8 deletions test/automation/src/playwrightBrowser.ts
View file
Open in desktop
Expand Up @@ -34,7 +34,7 @@ async function launchServer(options: LaunchOptions) {
const { userDataDir, codePath, extensionsPath, logger, logsPath } = options;
const codeServerPath = codePath ?? process.env.VSCODE_REMOTE_SERVER_PATH;
const agentFolder = userDataDir;
await measureAndLog(mkdirp(agentFolder), `mkdirp(${agentFolder})`, logger);
await measureAndLog(() => mkdirp(agentFolder), `mkdirp(${agentFolder})`, logger);

const env = {
VSCODE_REMOTE_SERVER_PATH: codeServerPath,
Expand Down Expand Up @@ -81,28 +81,28 @@ async function launchServer(options: LaunchOptions) {

return {
serverProcess,
endpoint: await measureAndLog(waitForEndpoint(serverProcess, logger), 'waitForEndpoint(serverProcess)', logger)
endpoint: await measureAndLog(() => waitForEndpoint(serverProcess, logger), 'waitForEndpoint(serverProcess)', logger)
};
}

async function launchBrowser(options: LaunchOptions, endpoint: string) {
const { logger, workspacePath, tracing, headless } = options;

const browser = await measureAndLog(playwright[options.browser ?? 'chromium'].launch({ headless: headless ?? false }), 'playwright#launch', logger);
const browser = await measureAndLog(() => playwright[options.browser ?? 'chromium'].launch({ headless: headless ?? false }), 'playwright#launch', logger);
browser.on('disconnected', () => logger.log(`Playwright: browser disconnected`));

const context = await measureAndLog(browser.newContext(), 'browser.newContext', logger);
const context = await measureAndLog(() => browser.newContext(), 'browser.newContext', logger);

if (tracing) {
try {
await measureAndLog(context.tracing.start({ screenshots: true, /* remaining options are off for perf reasons */ }), 'context.tracing.start()', logger);
await measureAndLog(() => context.tracing.start({ screenshots: true, /* remaining options are off for perf reasons */ }), 'context.tracing.start()', logger);
} catch (error) {
logger.log(`Playwright (Browser): Failed to start playwright tracing (${error})`); // do not fail the build when this fails
}
}

const page = await measureAndLog(context.newPage(), 'context.newPage()', logger);
await measureAndLog(page.setViewportSize({ width: 1200, height: 800 }), 'page.setViewportSize', logger);
const page = await measureAndLog(() => context.newPage(), 'context.newPage()', logger);
await measureAndLog(() => page.setViewportSize({ width: 1200, height: 800 }), 'page.setViewportSize', logger);

if (options.verbose) {
context.on('page', () => logger.log(`Playwright (Browser): context.on('page')`));
Expand Down Expand Up @@ -133,7 +133,7 @@ async function launchBrowser(options: LaunchOptions, endpoint: string) {
`["logLevel","${options.verbose ? 'trace' : 'info'}"]`
].join(',')}]`;

await measureAndLog(page.goto(`${endpoint}&${workspacePath.endsWith('.code-workspace') ? 'workspace' : 'folder'}=${URI.file(workspacePath!).path}&payload=${payloadParam}`), 'page.goto()', logger);
await measureAndLog(() => page.goto(`${endpoint}&${workspacePath.endsWith('.code-workspace') ? 'workspace' : 'folder'}=${URI.file(workspacePath!).path}&payload=${payloadParam}`), 'page.goto()', logger);

return { browser, context, page };
}
Expand Down
14 changes: 7 additions & 7 deletions test/automation/src/playwrightDriver.ts
View file
Open in desktop
Expand Up @@ -46,7 +46,7 @@ export class PlaywrightDriver {
}

try {
await measureAndLog(this.context.tracing.startChunk({ title: name }), `startTracing for ${name}`, this.options.logger);
await measureAndLog(() => this.context.tracing.startChunk({ title: name }), `startTracing for ${name}`, this.options.logger);
} catch (error) {
// Ignore
}
Expand All @@ -63,7 +63,7 @@ export class PlaywrightDriver {
persistPath = join(this.options.logsPath, `playwright-trace-${PlaywrightDriver.traceCounter++}-${name.replace(/\s+/g, '-')}.zip`);
}

await measureAndLog(this.context.tracing.stopChunk({ path: persistPath }), `stopTracing for ${name}`, this.options.logger);
await measureAndLog(() => this.context.tracing.stopChunk({ path: persistPath }), `stopTracing for ${name}`, this.options.logger);

// To ensure we have a screenshot at the end where
// it failed, also trigger one explicitly. Tracing
Expand Down Expand Up @@ -95,7 +95,7 @@ export class PlaywrightDriver {
try {
const persistPath = join(this.options.logsPath, `playwright-screenshot-${PlaywrightDriver.screenShotCounter++}-${name.replace(/\s+/g, '-')}.png`);

await measureAndLog(this.page.screenshot({ path: persistPath, type: 'png' }), 'takeScreenshot', this.options.logger);
await measureAndLog(() => this.page.screenshot({ path: persistPath, type: 'png' }), 'takeScreenshot', this.options.logger);
} catch (error) {
// Ignore
}
Expand All @@ -110,7 +110,7 @@ export class PlaywrightDriver {
// Stop tracing
try {
if (this.options.tracing) {
await measureAndLog(this.context.tracing.stop(), 'stop tracing', this.options.logger);
await measureAndLog(() => this.context.tracing.stop(), 'stop tracing', this.options.logger);
}
} catch (error) {
// Ignore
Expand All @@ -119,7 +119,7 @@ export class PlaywrightDriver {
// Web: exit via `close` method
if (this.options.web) {
try {
await measureAndLog(this.application.close(), 'playwright.close()', this.options.logger);
await measureAndLog(() => this.application.close(), 'playwright.close()', this.options.logger);
} catch (error) {
this.options.logger.log(`Error closing appliction (${error})`);
}
Expand All @@ -128,15 +128,15 @@ export class PlaywrightDriver {
// Desktop: exit via `driver.exitApplication`
else {
try {
await measureAndLog(this.evaluateWithDriver(([driver]) => driver.exitApplication()), 'driver.exitApplication()', this.options.logger);
await measureAndLog(() => this.evaluateWithDriver(([driver]) => driver.exitApplication()), 'driver.exitApplication()', this.options.logger);
} catch (error) {
this.options.logger.log(`Error exiting appliction (${error})`);
}
}

// Server: via `teardown`
if (this.serverProcess) {
await measureAndLog(teardown(this.serverProcess, this.options.logger), 'teardown server process', this.options.logger);
await measureAndLog(() => teardown(this.serverProcess!, this.options.logger), 'teardown server process', this.options.logger);
}
}

Expand Down
6 changes: 3 additions & 3 deletions test/automation/src/playwrightElectron.ts
View file
Open in desktop
Expand Up @@ -29,19 +29,19 @@ export async function launch(options: LaunchOptions): Promise<{ electronProcess:
async function launchElectron(configuration: IElectronConfiguration, options: LaunchOptions) {
const { logger, tracing } = options;

const electron = await measureAndLog(playwright._electron.launch({
const electron = await measureAndLog(() => playwright._electron.launch({
executablePath: configuration.electronPath,
args: configuration.args,
env: configuration.env as { [key: string]: string }
}), 'playwright-electron#launch', logger);

const window = await measureAndLog(electron.firstWindow(), 'playwright-electron#firstWindow', logger);
const window = await measureAndLog(() => electron.firstWindow(), 'playwright-electron#firstWindow', logger);

const context = window.context();

if (tracing) {
try {
await measureAndLog(context.tracing.start({ screenshots: true, /* remaining options are off for perf reasons */ }), 'context.tracing.start()', logger);
await measureAndLog(() => context.tracing.start({ screenshots: true, /* remaining options are off for perf reasons */ }), 'context.tracing.start()', logger);
} catch (error) {
logger.log(`Playwright (Electron): Failed to start playwright tracing (${error})`); // do not fail the build when this fails
}
Expand Down
12 changes: 6 additions & 6 deletions test/smoke/src/main.ts
View file
Open in desktop
Expand Up @@ -265,13 +265,13 @@ async function ensureStableCode(): Promise<void> {
if (!stableCodePath) {
const { major, minor } = parseVersion(version!);
const majorMinorVersion = `${major}.${minor - 1}`;
const versionsReq = await retry(() => measureAndLog(fetch('https://update.code.visualstudio.com/api/releases/stable', { headers: { 'x-api-version': '2' } }), 'versionReq', logger), 1000, 20);
const versionsReq = await retry(() => measureAndLog(() => fetch('https://update.code.visualstudio.com/api/releases/stable', { headers: { 'x-api-version': '2' } }), 'versionReq', logger), 1000, 20);

if (!versionsReq.ok) {
throw new Error('Could not fetch releases from update server');
}

const versions: { version: string }[] = await measureAndLog(versionsReq.json(), 'versionReq.json()', logger);
const versions: { version: string }[] = await measureAndLog(() => versionsReq.json(), 'versionReq.json()', logger);
const prefix = `${majorMinorVersion}.`;
const previousVersion = versions.find(v => v.version.startsWith(prefix));

Expand All @@ -284,7 +284,7 @@ async function ensureStableCode(): Promise<void> {
let lastProgressMessage: string | undefined = undefined;
let lastProgressReportedAt = 0;
const stableCodeDestination = path.join(testDataPath, 's');
const stableCodeExecutable = await retry(() => measureAndLog(vscodetest.download({
const stableCodeExecutable = await retry(() => measureAndLog(() => vscodetest.download({
cachePath: stableCodeDestination,
version: previousVersion.version,
extractSync: true,
Expand Down Expand Up @@ -339,9 +339,9 @@ async function setup(): Promise<void> {

if (!opts.web && !opts.remote && opts.build) {
// only enabled when running with --build and not in web or remote
await measureAndLog(ensureStableCode(), 'ensureStableCode', logger);
await measureAndLog(() => ensureStableCode(), 'ensureStableCode', logger);
}
await measureAndLog(setupRepository(), 'setupRepository', logger);
await measureAndLog(() => setupRepository(), 'setupRepository', logger);

logger.log('Smoketest setup done!\n');
}
Expand Down Expand Up @@ -375,7 +375,7 @@ before(async function () {
after(async function () {
try {
let deleted = false;
await measureAndLog(Promise.race([
await measureAndLog(() => Promise.race([
new Promise<void>((resolve, reject) => rimraf(testDataPath, { maxBusyTries: 10 }, error => {
if (error) {
reject(error);
Expand Down