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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test: watch full stdout when waiting for match #23767

Merged
merged 1 commit into from
Aug 29, 2022
Merged

test: watch full stdout when waiting for match #23767

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
2 changes: 1 addition & 1 deletion tests/legacy-cli/e2e/tests/build/bundle-budgets.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ export default async function () {
];
});

const errorMessage = await expectToFail(() => ng('build'));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

errorMessage is an Error and this was relying on errorMessage.toString() in the .test below. When making the expectToFail return type stricter this becomes an error.

const { message: errorMessage } = await expectToFail(() => ng('build'));
if (!/Error.+budget/.test(errorMessage)) {
throw new Error('Budget error: all, max error.');
}
Expand Down
107 changes: 52 additions & 55 deletions tests/legacy-cli/e2e/utils/process.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,6 @@ function _exec(options: ExecOptions, cmd: string, args: string[]): Promise<Proce
// Create a separate instance to prevent unintended global changes to the color configuration
const colors = ansiColors.create();

let stdout = '';
let stderr = '';
const cwd = options.cwd ?? process.cwd();
const env = options.env ?? process.env;
console.log(
Expand Down Expand Up @@ -69,48 +67,65 @@ function _exec(options: ExecOptions, cmd: string, args: string[]): Promise<Proce
}

const childProcess = child_process.spawn(cmd, args, spawnOptions);
childProcess.stdout!.on('data', (data: Buffer) => {
stdout += data.toString('utf-8');
if (options.silent) {
return;
}
data
.toString('utf-8')
.split(/[\n\r]+/)
.filter((line) => line !== '')
.forEach((line) => console.log(' ' + line));
});

childProcess.stderr!.on('data', (data: Buffer) => {
stderr += data.toString('utf-8');
if (options.silent) {
return;
}
data
.toString('utf-8')
.split(/[\n\r]+/)
.filter((line) => line !== '')
.forEach((line) => console.error(colors.yellow(' ' + line)));
});

_processes.push(childProcess);

// Create the error here so the stack shows who called this function.
const error = new Error();

// Return log info about the current process status
function envDump() {
return [
`ENV:${JSON.stringify(spawnOptions.env, null, 2)}`,
`STDOUT:\n${stdout}`,
`STDERR:\n${stderr}`,
].join('\n\n');
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Everything above here was just moved into the new Promise so the waitForMatch logic in the callbacks can resolve the promise.


return new Promise<ProcessOutput>((resolve, reject) => {
let stdout = '';
let stderr = '';
let matched = false;

childProcess.on('exit', (code: number) => {
// Return log info about the current process status
function envDump() {
return [
`ENV:${JSON.stringify(spawnOptions.env, null, 2)}`,
`STDOUT:\n${stdout}`,
`STDERR:\n${stderr}`,
].join('\n\n');
}

childProcess.stdout!.on('data', (data: Buffer) => {
stdout += data.toString('utf-8');

if (options.waitForMatch && stdout.match(options.waitForMatch)) {
Copy link
Contributor Author

@jbedard jbedard Aug 19, 2022 •
edited

Choose a reason for hiding this comment

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

Previously this did data.match which in theory could fail if the stdout was spread across multiple 'data' events (this was in the deleted if (options.waitForMatch below). Same with stderr.

resolve({ stdout, stderr });
matched = true;
}

if (options.silent) {
return;
}

data
.toString('utf-8')
.split(/[\n\r]+/)
.filter((line) => line !== '')
.forEach((line) => console.log(' ' + line));
});

childProcess.stderr!.on('data', (data: Buffer) => {
stderr += data.toString('utf-8');

if (options.waitForMatch && stderr.match(options.waitForMatch)) {
resolve({ stdout, stderr });
matched = true;
}

if (options.silent) {
return;
}

data
.toString('utf-8')
.split(/[\n\r]+/)
.filter((line) => line !== '')
.forEach((line) => console.error(colors.yellow(' ' + line)));
});

childProcess.on('close', (code: number) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note this changed from exit to close to ensure stdout/err have completed:

https://nodejs.org/api/child_process.html#event-close

The 'close' event is emitted after a process has ended and the stdio streams of a child process have been closed

https://nodejs.org/api/child_process.html#event-exit

When the 'exit' event is triggered, child process stdio streams might still be open.

_processes = _processes.filter((p) => p !== childProcess);

if (options.waitForMatch && !matched) {
Expand All @@ -134,24 +149,6 @@ function _exec(options: ExecOptions, cmd: string, args: string[]): Promise<Proce
reject(`Process error - "${cmd} ${args.join(' ')}": ${err}...\n\n${envDump()}\n`);
});

if (options.waitForMatch) {
const match = options.waitForMatch;

childProcess.stdout!.on('data', (data: Buffer) => {
if (data.toString().match(match)) {
resolve({ stdout, stderr });
matched = true;
}
});

childProcess.stderr!.on('data', (data: Buffer) => {
if (data.toString().match(match)) {
resolve({ stdout, stderr });
matched = true;
}
});
}

// Provide input to stdin if given.
if (options.stdin) {
childProcess.stdin!.write(options.stdin);
Expand Down Expand Up @@ -192,14 +189,14 @@ export function waitForAnyProcessOutputToMatch(

childProcess.stdout!.on('data', (data: Buffer) => {
stdout += data.toString();
if (data.toString().match(match)) {
if (stdout.match(match)) {
resolve({ stdout, stderr });
}
});

childProcess.stderr!.on('data', (data: Buffer) => {
stderr += data.toString();
if (data.toString().match(match)) {
if (stderr.match(match)) {
resolve({ stdout, stderr });
}
});
Expand Down
4 changes: 2 additions & 2 deletions tests/legacy-cli/e2e/utils/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { mkdtemp, realpath, rm } from 'fs/promises';
import { tmpdir } from 'os';
import path from 'path';

export function expectToFail(fn: () => Promise<any>, errorMessage?: string): Promise<any> {
export function expectToFail(fn: () => Promise<any>, errorMessage?: string): Promise<Error> {
return fn().then(
() => {
const functionSource = fn.name || (<any>fn).source || fn.toString();
Expand All @@ -12,7 +12,7 @@ export function expectToFail(fn: () => Promise<any>, errorMessage?: string): Pro
);
},
(err) => {
return err;
return err instanceof Error ? err : new Error(err);
},
);
}
Expand Down