Skip to content

Commit

Permalink
Ensure async telemetry hooks resolve before exiting
Browse files Browse the repository at this point in the history
  • Loading branch information
Michael Sitter committed May 10, 2024
1 parent 94790ae commit ed4dba5
Show file tree
Hide file tree
Showing 5 changed files with 208 additions and 108 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"changes": [
{
"packageName": "@microsoft/rush",
"comment": "Ensure async telemetry tasks are flushed by error reporter",
"type": "patch"
}
],
"packageName": "@microsoft/rush"
}
38 changes: 26 additions & 12 deletions libraries/rush-lib/src/cli/RushCommandLineParser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,7 @@ export class RushCommandLineParser extends CommandLineParser {
this._reportErrorAndSetExitCode(error as Error);
}

// This only gets hit if the wrapped execution completes successfully
await this.telemetry?.ensureFlushedAsync();
}

Expand All @@ -243,9 +244,12 @@ export class RushCommandLineParser extends CommandLineParser {
this.telemetry = new Telemetry(this.rushConfiguration, this.rushSession);
}

await super.onExecute();
if (this.telemetry) {
this.flushTelemetry();
try {
await super.onExecute();
} finally {
if (this.telemetry) {
this.flushTelemetry();
}
}
}

Expand Down Expand Up @@ -440,16 +444,26 @@ export class RushCommandLineParser extends CommandLineParser {

this.flushTelemetry();

// Ideally we want to eliminate all calls to process.exit() from our code, and replace them
// with normal control flow that properly cleans up its data structures.
// For this particular call, we have a problem that the RushCommandLineParser constructor
// performs nontrivial work that can throw an exception. Either the Rush class would need
// to handle reporting for those exceptions, or else _populateActions() should be moved
// to a RushCommandLineParser lifecycle stage that can handle it.
if (process.exitCode !== undefined) {
process.exit(process.exitCode);
const handleExit = (): never => {
// Ideally we want to eliminate all calls to process.exit() from our code, and replace them
// with normal control flow that properly cleans up its data structures.
// For this particular call, we have a problem that the RushCommandLineParser constructor
// performs nontrivial work that can throw an exception. Either the Rush class would need
// to handle reporting for those exceptions, or else _populateActions() should be moved
// to a RushCommandLineParser lifecycle stage that can handle it.
if (process.exitCode !== undefined) {
process.exit(process.exitCode);
} else {
process.exit(1);
}
};

if (this.telemetry && this.rushSession.hooks.flushTelemetry.isUsed()) {
this.telemetry.ensureFlushedAsync()
.then(handleExit)
.catch(handleExit);
} else {
process.exit(1);
handleExit();
}
}
}
100 changes: 4 additions & 96 deletions libraries/rush-lib/src/cli/test/RushCommandLineParser.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,108 +17,15 @@ jest.mock(`@rushstack/package-deps-hash`, () => {

import './mockRushCommandLineParser';

import * as path from 'path';
import { FileSystem, JsonFile, Path, PackageJsonLookup } from '@rushstack/node-core-library';
import type { RushCommandLineParser as RushCommandLineParserType } from '../RushCommandLineParser';
import { FileSystem, JsonFile, Path, } from '@rushstack/node-core-library';
import { Autoinstaller } from '../../logic/Autoinstaller';
import type { ITelemetryData } from '../../logic/Telemetry';
import { RushConstants } from '../../logic/RushConstants';
import { FlagFile } from '../../api/FlagFile';

/**
* See `__mocks__/child_process.js`.
*/
interface ISpawnMockConfig {
emitError: boolean;
returnCode: number;
}

interface IChildProcessModuleMock {
/**
* Initialize the `spawn` mock behavior.
*/
__setSpawnMockConfig(config?: ISpawnMockConfig): void;

spawn: jest.Mock;
}

/**
* Interface definition for a test instance for the RushCommandLineParser.
*/
interface IParserTestInstance {
parser: RushCommandLineParserType;
spawnMock: jest.Mock;
}

/**
* Configure the `child_process` `spawn` mock for these tests. This relies on the mock implementation
* in `__mocks__/child_process.js`.
*/
function setSpawnMock(options?: ISpawnMockConfig): jest.Mock {
const cpMocked: IChildProcessModuleMock = require('child_process');
cpMocked.__setSpawnMockConfig(options);

const spawnMock: jest.Mock = cpMocked.spawn;
spawnMock.mockName('spawn');
return spawnMock;
}

function getDirnameInLib(): string {
// Run these tests in the /lib folder because some of them require compiled output
const projectRootFolder: string = PackageJsonLookup.instance.tryGetPackageFolderFor(__dirname)!;
const projectRootRelativeDirnamePath: string = path.relative(projectRootFolder, __dirname);
const projectRootRelativeLibDirnamePath: string = projectRootRelativeDirnamePath.replace(
/^src/,
'lib-commonjs'
);
const dirnameInLIb: string = `${projectRootFolder}/${projectRootRelativeLibDirnamePath}`;
return dirnameInLIb;
}
import type { IParserTestInstance } from './TestUtils';
import { getDirnameInLib, getCommandLineParserInstanceAsync } from './TestUtils';

// eslint-disable-next-line @typescript-eslint/naming-convention
const __dirnameInLib: string = getDirnameInLib();

/**
* Helper to set up a test instance for RushCommandLineParser.
*/
async function getCommandLineParserInstanceAsync(
repoName: string,
taskName: string
): Promise<IParserTestInstance> {
// Run these tests in the /lib folder because some of them require compiled output
// Point to the test repo folder
const startPath: string = `${__dirnameInLib}/${repoName}`;

// The `build` task is hard-coded to be incremental. So delete the package-deps file folder in
// the test repo to guarantee the test actually runs.
FileSystem.deleteFolder(`${startPath}/a/.rush/temp`);
FileSystem.deleteFolder(`${startPath}/b/.rush/temp`);

const { RushCommandLineParser } = await import('../RushCommandLineParser');

// Create a Rush CLI instance. This instance is heavy-weight and relies on setting process.exit
// to exit and clear the Rush file lock. So running multiple `it` or `describe` test blocks over the same test
// repo will fail due to contention over the same lock which is kept until the test runner process
// ends.
const parser: RushCommandLineParserType = new RushCommandLineParser({ cwd: startPath });

// Bulk tasks are hard-coded to expect install to have been completed. So, ensure the last-link.flag
// file exists and is valid
await new FlagFile(
parser.rushConfiguration.defaultSubspace.getSubspaceTempFolder(),
RushConstants.lastLinkFlagFilename
).createAsync();

// Mock the command
process.argv = ['pretend-this-is-node.exe', 'pretend-this-is-rush', taskName];
const spawnMock: jest.Mock = setSpawnMock();

return {
parser,
spawnMock
};
}

function pathEquals(actual: string, expected: string): void {
expect(Path.convertToSlashes(actual)).toEqual(Path.convertToSlashes(expected));
}
Expand Down Expand Up @@ -399,6 +306,7 @@ describe('RushCommandLineParser', () => {
telemetryStore = JsonFile.load(telemetryFilePath);
}).not.toThrowError();
expect(telemetryStore?.[0].name).toEqual('build');
expect(telemetryStore?.[0].result).toEqual('Succeeded');
});
});
});
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
// Copyright (c) Microsoft Corporation. All rights reserved. Licensed under the MIT license.
// See LICENSE in the project root for license information.

// Mock child_process so we can verify tasks are (or are not) invoked as we expect
jest.mock('child_process');
jest.mock('@rushstack/terminal');
jest.mock(`@rushstack/package-deps-hash`, () => {
return {
getRepoRoot(dir: string): string {
return dir;
},
getRepoStateAsync(): ReadonlyMap<string, string> {
return new Map();
},
getRepoChangesAsync(): ReadonlyMap<string, string> {
return new Map();
}
};
});

import { FileSystem, JsonFile } from '@rushstack/node-core-library';
import { Autoinstaller } from '../../logic/Autoinstaller';
import type { ITelemetryData } from '../../logic/Telemetry';
import type { IParserTestInstance } from './TestUtils';
import { getCommandLineParserInstanceAsync, setSpawnMock } from './TestUtils';

describe('RushCommandLineParserFailureCases', () => {
describe('execute', () => {
afterEach(() => {
jest.clearAllMocks();
});

describe('in repo plugin custom flushTelemetry', () => {
it('custom telemetry reports errors', async () => {
const repoName: string = 'tapFlushTelemetryAndRunBuildActionRepo';

// WARNING: This test case needs the real implementation of _reportErrorAndSetExitCode.
// As a result, process.exit needs to be explicitly mocked to prevent the test runner from exiting.
const procProm = new Promise<void>((resolve, reject) => {
jest.spyOn(process, 'exit').mockImplementation((() => {
resolve();
// eslint-disable-next-line @typescript-eslint/no-explicit-any
}) as any);
});

const instance: IParserTestInstance = await getCommandLineParserInstanceAsync(repoName, 'build');

const telemetryFilePath: string = `${instance.parser.rushConfiguration.commonTempFolder}/test-telemetry.json`;
FileSystem.deleteFile(telemetryFilePath);

jest.spyOn(Autoinstaller.prototype, 'prepareAsync').mockImplementation(async function () {});

setSpawnMock({ emitError: false, returnCode: 1 });
await instance.parser.execute();
await procProm;
expect(process.exit).toHaveBeenCalledWith(1);

expect(FileSystem.exists(telemetryFilePath)).toEqual(true);

const telemetryStore: ITelemetryData[] = JsonFile.load(telemetryFilePath);
expect(telemetryStore?.[0].name).toEqual('build');
expect(telemetryStore?.[0].result).toEqual('Failed');
});
});
});
});
102 changes: 102 additions & 0 deletions libraries/rush-lib/src/cli/test/TestUtils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
// Copyright (c) Microsoft Corporation. All rights reserved. Licensed under the MIT license.
// See LICENSE in the project root for license information.

import * as path from 'path';
import { FileSystem, PackageJsonLookup } from '@rushstack/node-core-library';
import type { RushCommandLineParser as RushCommandLineParserType } from '../RushCommandLineParser';
import { FlagFile } from '../../api/FlagFile';
import { RushConstants } from '../../logic/RushConstants';

/**
* Interface definition for a test instance for the RushCommandLineParser.
*/
export interface IParserTestInstance {
parser: RushCommandLineParserType;
spawnMock: jest.Mock;
}

/**
* See `__mocks__/child_process.js`.
*/
export interface ISpawnMockConfig {
emitError: boolean;
returnCode: number;
}

export interface IChildProcessModuleMock {
/**
* Initialize the `spawn` mock behavior.
*/
__setSpawnMockConfig(config?: ISpawnMockConfig): void;

spawn: jest.Mock;
}

/**
* Configure the `child_process` `spawn` mock for these tests. This relies on the mock implementation
* in `__mocks__/child_process.js`.
*/
export function setSpawnMock(options?: ISpawnMockConfig): jest.Mock {
const cpMocked: IChildProcessModuleMock = require('child_process');
cpMocked.__setSpawnMockConfig(options);

const spawnMock: jest.Mock = cpMocked.spawn;
spawnMock.mockName('spawn');
return spawnMock;
}

export function getDirnameInLib(): string {
// Run these tests in the /lib folder because some of them require compiled output
const projectRootFolder: string = PackageJsonLookup.instance.tryGetPackageFolderFor(__dirname)!;
const projectRootRelativeDirnamePath: string = path.relative(projectRootFolder, __dirname);
const projectRootRelativeLibDirnamePath: string = projectRootRelativeDirnamePath.replace(
/^src/,
'lib-commonjs'
);
const dirnameInLIb: string = `${projectRootFolder}/${projectRootRelativeLibDirnamePath}`;
return dirnameInLIb;
}

// eslint-disable-next-line @typescript-eslint/naming-convention
const __dirnameInLib: string = getDirnameInLib();

/**
* Helper to set up a test instance for RushCommandLineParser.
*/
export async function getCommandLineParserInstanceAsync(
repoName: string,
taskName: string
): Promise<IParserTestInstance> {
// Run these tests in the /lib folder because some of them require compiled output
// Point to the test repo folder
const startPath: string = `${__dirnameInLib}/${repoName}`;

// The `build` task is hard-coded to be incremental. So delete the package-deps file folder in
// the test repo to guarantee the test actually runs.
FileSystem.deleteFolder(`${startPath}/a/.rush/temp`);
FileSystem.deleteFolder(`${startPath}/b/.rush/temp`);

const { RushCommandLineParser } = await import('../RushCommandLineParser');

// Create a Rush CLI instance. This instance is heavy-weight and relies on setting process.exit
// to exit and clear the Rush file lock. So running multiple `it` or `describe` test blocks over the same test
// repo will fail due to contention over the same lock which is kept until the test runner process
// ends.
const parser: RushCommandLineParserType = new RushCommandLineParser({ cwd: startPath });

// Bulk tasks are hard-coded to expect install to have been completed. So, ensure the last-link.flag
// file exists and is valid
await new FlagFile(
parser.rushConfiguration.defaultSubspace.getSubspaceTempFolder(),
RushConstants.lastLinkFlagFilename
).createAsync();

// Mock the command
process.argv = ['pretend-this-is-node.exe', 'pretend-this-is-rush', taskName];
const spawnMock: jest.Mock = setSpawnMock();

return {
parser,
spawnMock
};
}

0 comments on commit ed4dba5

Please sign in to comment.