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.

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

Execute iterator exploration #1

Closed
wants to merge 10 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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 docs/angular/api-node/executors/execute.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,14 @@ Type: `array`

Extra args when starting the app

### emitSubprocessEvents

Default: `false`

Type: `boolean`

Emit events from subprocess

### host

Default: `localhost`
Expand Down
8 changes: 8 additions & 0 deletions docs/node/api-node/executors/execute.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,14 @@ Type: `array`

Extra args when starting the app

### emitSubprocessEvents

Default: `false`

Type: `boolean`

Emit events from subprocess

### host

Default: `localhost`
Expand Down
8 changes: 8 additions & 0 deletions docs/react/api-node/executors/execute.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,14 @@ Type: `array`

Extra args when starting the app

### emitSubprocessEvents

Default: `false`

Type: `boolean`

Emit events from subprocess

### host

Default: `localhost`
Expand Down
249 changes: 244 additions & 5 deletions packages/node/src/executors/execute/execute.impl.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
describe('NodeExecuteBuilder', () => {
let testOptions: NodeExecuteBuilderOptions;
let context: ExecutorContext;
let mockSubProcess: { on: jest.Mock; exitCode: number };

beforeEach(async () => {
buildOptions = {};
Expand All @@ -34,13 +35,16 @@ describe('NodeExecuteBuilder', () => {
);

fork.mockImplementation(() => {
return {
on: (eventName, cb) => {
mockSubProcess = {
on: jest.fn().mockImplementation((eventName, cb) => {
if (eventName === 'exit') {
cb();
mockSubProcess.exitCode = 0;
cb(mockSubProcess.exitCode);
}
},
}),
exitCode: null,
};
return mockSubProcess;
});

treeKill.mockImplementation((pid, signal, callback) => {
Expand Down Expand Up @@ -74,6 +78,7 @@ describe('NodeExecuteBuilder', () => {
waitUntilTargets: [],
host: 'localhost',
watch: true,
emitSubprocessEvents: false,
};
});

Expand All @@ -83,6 +88,7 @@ describe('NodeExecuteBuilder', () => {

it('should build the application and start the built file', async () => {
for await (const event of executeExecutor(testOptions, context)) {
if (!('success' in event)) throw new Error('Got Unexpected event');
Copy link
Owner Author

Choose a reason for hiding this comment

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

Represents a breaking change: the type of event yielded by executeExecutor is now https://github.com/dpchamps/nx/pull/1/files#diff-8b1daad050dbc745646b89f91cd20c497a23f51caf7eeaddb96a517a80b8f01fR28

expect(event.success).toEqual(true);
}
expect(require('@nrwl/devkit').runExecutor).toHaveBeenCalledWith(
Expand All @@ -105,7 +111,6 @@ describe('NodeExecuteBuilder', () => {
expect(treeKill).toHaveBeenCalledTimes(0);
expect(fork).toHaveBeenCalledTimes(1);
});

describe('--inspect', () => {
describe('inspect', () => {
it('should inspect the process', async () => {
Expand Down Expand Up @@ -215,6 +220,10 @@ describe('NodeExecuteBuilder', () => {
});

it('should log errors from killing the process', async () => {
(devkit.runExecutor as any).mockImplementation(function* () {
yield { success: true, outfile: 'outfile.js' };
yield { success: true, outfile: 'outfile.js' };
});
Comment on lines +223 to +226
Copy link
Owner Author

Choose a reason for hiding this comment

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

The old test cases for this spec and the one below were non-deterministic:

The state of subProcess was managed as a file-level global previously. These tests would only pass when at least one previous test had been run to populate the subProcess var.

With that subProcess state encapsulated, there was no way to make this test pass without altering it.

treeKill.mockImplementation((pid, signal, callback) => {
callback(new Error('Error Message'));
});
Expand All @@ -227,6 +236,10 @@ describe('NodeExecuteBuilder', () => {
});

it('should log errors from killing the process on windows', async () => {
(devkit.runExecutor as any).mockImplementation(function* () {
yield { success: true, outfile: 'outfile.js' };
yield { success: true, outfile: 'outfile.js' };
});
treeKill.mockImplementation((pid, signal, callback) => {
callback([new Error('error'), '', 'Error Message']);
});
Expand Down Expand Up @@ -329,4 +342,230 @@ describe('NodeExecuteBuilder', () => {
}
});
});

describe('--watch', () => {
let mockSubProcessExit;
let mockSubProcessMessage;
let mockSubProcessError;

beforeEach(() => {
fork.mockImplementation(() => {
mockSubProcess = {
on: jest.fn().mockImplementation((eventName, cb) => {
if (eventName === 'exit') {
mockSubProcessExit = (exitCode: number) => {
cb(exitCode);
mockSubProcess.exitCode = exitCode;
};
}

if (eventName === 'error') {
mockSubProcessError = cb;
}

if (eventName === 'message') {
mockSubProcessMessage = cb;
}
}),
exitCode: null,
};

return mockSubProcess;
});
});

describe('false', () => {
it('Should not complete until the child process has exited', async () => {
const firstEvent = Symbol('first');
const secondEvent = Symbol('second');
const events = [];
const executorIterator = executeExecutor(
{
...testOptions,
watch: false,
},
context
);

expect(await executorIterator.next()).toEqual({
done: false,
value: {
outfile: 'outfile.js',
success: true,
},
});

const result = await Promise.race([
executorIterator.next(),
new Promise(() => {
mockSubProcessExit(1);
events.push(firstEvent);
}),
]);

events.push(secondEvent);

expect(result).toEqual({
value: undefined,
done: true,
});
expect(events).toEqual([firstEvent, secondEvent]);
});

it('When sub process emits, it should propagate when configured', async () => {
const expectedExitCode = 42;
const executorIterator = executeExecutor(
{
...testOptions,
watch: false,
emitSubprocessEvents: true,
},
context
);

expect(await executorIterator.next()).toEqual({
done: false,
value: {
outfile: 'outfile.js',
success: true,
},
});

mockSubProcessExit(expectedExitCode);

expect(await executorIterator.next()).toEqual({
done: false,
value: {
exitCode: expectedExitCode,
},
});
});

it('Should propagate sub process events in expected order', async () => {
const expectedSubProcessEvents = [
{
message: 'hello',
},
{
message: 'world',
},
{
error: new Error('error'),
},
{
exitCode: 1,
},
];
const expectedTotalEvents = 5;

let subProcessEvents = [];
let totalEvents = 0;

const doSubProcessWork = () => {
mockSubProcessMessage('hello');
mockSubProcessMessage('world');
mockSubProcessError(new Error('error'));
mockSubProcessExit(1);
};

for await (const event of executeExecutor(
{
...testOptions,
watch: false,
emitSubprocessEvents: true,
},
context
)) {
if (totalEvents === 0) {
expect(event).toEqual({
outfile: 'outfile.js',
success: true,
});
doSubProcessWork();
} else {
subProcessEvents.push(event);
}

totalEvents += 1;
}

expect(subProcessEvents).toEqual(expectedSubProcessEvents);
expect(totalEvents).toEqual(expectedTotalEvents);
});
});

describe('true', () => {
beforeEach(() => {
(devkit.runExecutor as any).mockImplementation(async function* () {
yield { success: true, outfile: 'outfile.js' };
await new Promise((resolve) => {
setTimeout(resolve, 100);
});
yield { success: true, outfile: 'outfile.js' };
});
});

it('Should not block build events with process events', async () => {
const executorIterator = executeExecutor(
{
...testOptions,
watch: true,
},
context
);

expect(await executorIterator.next()).toEqual({
done: false,
value: {
outfile: 'outfile.js',
success: true,
},
});

// sub process has not exited yet, but we'll receive a new build event

expect(await executorIterator.next()).toEqual({
done: false,
value: {
outfile: 'outfile.js',
success: true,
},
});

// there's no more build events, but process still needs to exit

const result = await Promise.race([
executorIterator.next(),
new Promise(() => {
mockSubProcessExit();
}),
]);

expect(result).toEqual({
value: undefined,
done: true,
});
});

it('Yield build and process events cooperatively', async () => {
const events = [];
for await (const event of executeExecutor(
{
...testOptions,
watch: true,
emitSubprocessEvents: true,
},
context
)) {
if ('outfile' in event) {
mockSubProcessExit(1);
}

events.push(event);
}

expect(events).toHaveLength(4);
});
});
});
});