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

Ensuring completion-listener.ts listen to a single close event emitte… #464

Merged
merged 11 commits into from Mar 24, 2024
Merged

Ensuring completion-listener.ts listen to a single close event emitte… #464

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
37156a9
Ensuring completion-listener.ts listen to a single close event emitte…
Feb 1, 2024
2eee2c2
Format.
Feb 1, 2024
5c9b16f
-Apply Latest.
Feb 25, 2024
f5dd736
Remove IPC support
Feb 25, 2024
1c03da7
Apply PR changes.
Feb 26, 2024
9c8edfb
Faked.
Feb 26, 2024
f81fa00
Exited
Feb 26, 2024
bfd2094
.
Feb 26, 2024
6992c26
Merge branch 'open-cli-tools:main' into main
odeadglaz Feb 27, 2024
c86d247
Check command state, instead of close event state
gustavohenke Mar 24, 2024
54818fb
Use emitFakeCloseEvent util more
gustavohenke Mar 24, 2024
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
266 changes: 147 additions & 119 deletions src/completion-listener.spec.ts
View file
Open in desktop
Expand Up @@ -24,186 +24,214 @@ const createController = (successCondition?: SuccessCondition) =>
const emitFakeCloseEvent = (command: FakeCommand, event?: Partial<CloseEvent>) =>
command.close.next(createFakeCloseEvent({ ...event, command, index: command.index }));

describe('with default success condition set', () => {
it('succeeds if all processes exited with code 0', () => {
const result = createController().listen(commands);
describe('listen', () => {
it('check for success once all commands have emitted at least a single close event', async () => {
const finallyCallback = jest.fn();
const result = createController().listen(commands).finally(finallyCallback);

// Emitting multiple close events to mimic calling command `kill/start` APIs.
commands[0].close.next(createFakeCloseEvent({ exitCode: 0 }));
commands[0].close.next(createFakeCloseEvent({ exitCode: 0 }));
commands[0].close.next(createFakeCloseEvent({ exitCode: 0 }));
commands[1].close.next(createFakeCloseEvent({ exitCode: 0 }));
commands[2].close.next(createFakeCloseEvent({ exitCode: 0 }));

scheduler.flush();

return expect(result).resolves.toEqual(expect.anything());
});

it('fails if one of the processes exited with non-0 code', () => {
const result = createController().listen(commands);
await Promise.resolve();
expect(finallyCallback).not.toHaveBeenCalled();

commands[0].close.next(createFakeCloseEvent({ exitCode: 0 }));
commands[1].close.next(createFakeCloseEvent({ exitCode: 1 }));
commands[1].close.next(createFakeCloseEvent({ exitCode: 0 }));
commands[2].close.next(createFakeCloseEvent({ exitCode: 0 }));

scheduler.flush();

return expect(result).rejects.toEqual(expect.anything());
await expect(result).resolves.toEqual(expect.anything());

expect(finallyCallback).toHaveBeenCalled();
});
});

describe('with success condition set to first', () => {
it('succeeds if first process to exit has code 0', () => {
const result = createController('first').listen(commands);
describe('Detect commands exit conditions', () => {
describe('with default success condition set', () => {
it('succeeds if all processes exited with code 0', () => {
const result = createController().listen(commands);

commands[1].close.next(createFakeCloseEvent({ exitCode: 0 }));
commands[0].close.next(createFakeCloseEvent({ exitCode: 1 }));
commands[2].close.next(createFakeCloseEvent({ exitCode: 1 }));
commands[0].close.next(createFakeCloseEvent({ exitCode: 0 }));
commands[1].close.next(createFakeCloseEvent({ exitCode: 0 }));
commands[2].close.next(createFakeCloseEvent({ exitCode: 0 }));

scheduler.flush();
scheduler.flush();

return expect(result).resolves.toEqual(expect.anything());
});
return expect(result).resolves.toEqual(expect.anything());
});

it('fails if first process to exit has non-0 code', () => {
const result = createController('first').listen(commands);
it('fails if one of the processes exited with non-0 code', () => {
const result = createController().listen(commands);

commands[1].close.next(createFakeCloseEvent({ exitCode: 1 }));
commands[0].close.next(createFakeCloseEvent({ exitCode: 0 }));
commands[2].close.next(createFakeCloseEvent({ exitCode: 0 }));
commands[0].close.next(createFakeCloseEvent({ exitCode: 0 }));
commands[1].close.next(createFakeCloseEvent({ exitCode: 1 }));
commands[2].close.next(createFakeCloseEvent({ exitCode: 0 }));

scheduler.flush();
scheduler.flush();

return expect(result).rejects.toEqual(expect.anything());
return expect(result).rejects.toEqual(expect.anything());
});
});
});

describe('with success condition set to last', () => {
it('succeeds if last process to exit has code 0', () => {
const result = createController('last').listen(commands);
describe('with success condition set to first', () => {
it('succeeds if first process to exit has code 0', () => {
const result = createController('first').listen(commands);

commands[1].close.next(createFakeCloseEvent({ exitCode: 1 }));
commands[0].close.next(createFakeCloseEvent({ exitCode: 0 }));
commands[2].close.next(createFakeCloseEvent({ exitCode: 0 }));
commands[1].close.next(createFakeCloseEvent({ exitCode: 0 }));
commands[0].close.next(createFakeCloseEvent({ exitCode: 1 }));
commands[2].close.next(createFakeCloseEvent({ exitCode: 1 }));

scheduler.flush();
scheduler.flush();

return expect(result).resolves.toEqual(expect.anything());
});
return expect(result).resolves.toEqual(expect.anything());
});

it('fails if last process to exit has non-0 code', () => {
const result = createController('last').listen(commands);
it('fails if first process to exit has non-0 code', () => {
const result = createController('first').listen(commands);

commands[1].close.next(createFakeCloseEvent({ exitCode: 0 }));
commands[0].close.next(createFakeCloseEvent({ exitCode: 1 }));
commands[2].close.next(createFakeCloseEvent({ exitCode: 1 }));
commands[1].close.next(createFakeCloseEvent({ exitCode: 1 }));
commands[0].close.next(createFakeCloseEvent({ exitCode: 0 }));
commands[2].close.next(createFakeCloseEvent({ exitCode: 0 }));

scheduler.flush();
scheduler.flush();

return expect(result).rejects.toEqual(expect.anything());
return expect(result).rejects.toEqual(expect.anything());
});
});
});

describe.each([
// Use the middle command for both cases to make it more difficult to make a mess up
// in the implementation cause false passes.
['command-bar' as const, 'bar'],
['command-1' as const, 1],
])('with success condition set to %s', (condition, nameOrIndex) => {
it(`succeeds if command ${nameOrIndex} exits with code 0`, () => {
const result = createController(condition).listen(commands);
describe('with success condition set to last', () => {
it('succeeds if last process to exit has code 0', () => {
const result = createController('last').listen(commands);

emitFakeCloseEvent(commands[0], { exitCode: 1 });
emitFakeCloseEvent(commands[1], { exitCode: 0 });
emitFakeCloseEvent(commands[2], { exitCode: 1 });
commands[1].close.next(createFakeCloseEvent({ exitCode: 1 }));
commands[0].close.next(createFakeCloseEvent({ exitCode: 0 }));
commands[2].close.next(createFakeCloseEvent({ exitCode: 0 }));

scheduler.flush();
scheduler.flush();

return expect(result).resolves.toEqual(expect.anything());
});
return expect(result).resolves.toEqual(expect.anything());
});

it(`succeeds if all commands ${nameOrIndex} exit with code 0`, () => {
commands = [commands[0], commands[1], commands[1]];
const result = createController(condition).listen(commands);
it('fails if last process to exit has non-0 code', () => {
const result = createController('last').listen(commands);

emitFakeCloseEvent(commands[0], { exitCode: 1 });
emitFakeCloseEvent(commands[1], { exitCode: 0 });
emitFakeCloseEvent(commands[2], { exitCode: 0 });
commands[1].close.next(createFakeCloseEvent({ exitCode: 0 }));
commands[0].close.next(createFakeCloseEvent({ exitCode: 1 }));
commands[2].close.next(createFakeCloseEvent({ exitCode: 1 }));

scheduler.flush();
scheduler.flush();

return expect(result).resolves.toEqual(expect.anything());
return expect(result).rejects.toEqual(expect.anything());
});
});

it(`fails if command ${nameOrIndex} exits with non-0 code`, () => {
const result = createController(condition).listen(commands);
describe.each([
// Use the middle command for both cases to make it more difficult to make a mess up
// in the implementation cause false passes.
['command-bar' as const, 'bar'],
['command-1' as const, 1],
])('with success condition set to %s', (condition, nameOrIndex) => {
it(`succeeds if command ${nameOrIndex} exits with code 0`, () => {
const result = createController(condition).listen(commands);

emitFakeCloseEvent(commands[0], { exitCode: 0 });
emitFakeCloseEvent(commands[1], { exitCode: 1 });
emitFakeCloseEvent(commands[2], { exitCode: 0 });
emitFakeCloseEvent(commands[0], { exitCode: 1 });
emitFakeCloseEvent(commands[1], { exitCode: 0 });
emitFakeCloseEvent(commands[2], { exitCode: 1 });

scheduler.flush();
scheduler.flush();

return expect(result).rejects.toEqual(expect.anything());
});
return expect(result).resolves.toEqual(expect.anything());
});

it(`fails if some commands ${nameOrIndex} exit with non-0 code`, () => {
commands = [commands[0], commands[1], commands[1]];
const result = createController(condition).listen(commands);
it(`succeeds if all commands ${nameOrIndex} exit with code 0`, () => {
commands = [commands[0], commands[1], commands[1]];
const result = createController(condition).listen(commands);

emitFakeCloseEvent(commands[0], { exitCode: 1 });
emitFakeCloseEvent(commands[1], { exitCode: 0 });
emitFakeCloseEvent(commands[2], { exitCode: 1 });
emitFakeCloseEvent(commands[0], { exitCode: 1 });
emitFakeCloseEvent(commands[1], { exitCode: 0 });
emitFakeCloseEvent(commands[2], { exitCode: 0 });

scheduler.flush();
scheduler.flush();

return expect(result).resolves.toEqual(expect.anything());
});
return expect(result).resolves.toEqual(expect.anything());
});

it(`fails if command ${nameOrIndex} doesn't exist`, () => {
const result = createController(condition).listen([commands[0]]);
it(`fails if command ${nameOrIndex} exits with non-0 code`, () => {
const result = createController(condition).listen(commands);

emitFakeCloseEvent(commands[0], { exitCode: 0 });
scheduler.flush();
emitFakeCloseEvent(commands[0], { exitCode: 0 });
emitFakeCloseEvent(commands[1], { exitCode: 1 });
emitFakeCloseEvent(commands[2], { exitCode: 0 });

return expect(result).rejects.toEqual(expect.anything());
});
});
scheduler.flush();

describe.each([
// Use the middle command for both cases to make it more difficult to make a mess up
// in the implementation cause false passes.
['!command-bar' as const, 'bar'],
['!command-1' as const, 1],
])('with success condition set to %s', (condition, nameOrIndex) => {
it(`succeeds if all commands but ${nameOrIndex} exit with code 0`, () => {
const result = createController(condition).listen(commands);
return expect(result).rejects.toEqual(expect.anything());
});

emitFakeCloseEvent(commands[0], { exitCode: 0 });
emitFakeCloseEvent(commands[1], { exitCode: 1 });
emitFakeCloseEvent(commands[2], { exitCode: 0 });
it(`fails if some commands ${nameOrIndex} exit with non-0 code`, () => {
commands = [commands[0], commands[1], commands[1]];
const result = createController(condition).listen(commands);

scheduler.flush();
emitFakeCloseEvent(commands[0], { exitCode: 1 });
emitFakeCloseEvent(commands[1], { exitCode: 0 });
emitFakeCloseEvent(commands[2], { exitCode: 1 });

return expect(result).resolves.toEqual(expect.anything());
});
scheduler.flush();

it(`fails if any commands but ${nameOrIndex} exit with non-0 code`, () => {
const result = createController(condition).listen(commands);
return expect(result).resolves.toEqual(expect.anything());
});

emitFakeCloseEvent(commands[0], { exitCode: 1 });
emitFakeCloseEvent(commands[1], { exitCode: 1 });
emitFakeCloseEvent(commands[2], { exitCode: 0 });
it(`fails if command ${nameOrIndex} doesn't exist`, () => {
const result = createController(condition).listen([commands[0]]);

scheduler.flush();
emitFakeCloseEvent(commands[0], { exitCode: 0 });
scheduler.flush();

return expect(result).rejects.toEqual(expect.anything());
return expect(result).rejects.toEqual(expect.anything());
});
});

it(`succeeds if command ${nameOrIndex} doesn't exist`, () => {
const result = createController(condition).listen([commands[0]]);
describe.each([
// Use the middle command for both cases to make it more difficult to make a mess up
// in the implementation cause false passes.
['!command-bar' as const, 'bar'],
['!command-1' as const, 1],
])('with success condition set to %s', (condition, nameOrIndex) => {
it(`succeeds if all commands but ${nameOrIndex} exit with code 0`, () => {
const result = createController(condition).listen(commands);

emitFakeCloseEvent(commands[0], { exitCode: 0 });
scheduler.flush();
emitFakeCloseEvent(commands[0], { exitCode: 0 });
emitFakeCloseEvent(commands[1], { exitCode: 1 });
emitFakeCloseEvent(commands[2], { exitCode: 0 });

scheduler.flush();

return expect(result).resolves.toEqual(expect.anything());
});

it(`fails if any commands but ${nameOrIndex} exit with non-0 code`, () => {
const result = createController(condition).listen(commands);

emitFakeCloseEvent(commands[0], { exitCode: 1 });
emitFakeCloseEvent(commands[1], { exitCode: 1 });
emitFakeCloseEvent(commands[2], { exitCode: 0 });

scheduler.flush();

return expect(result).rejects.toEqual(expect.anything());
});

it(`succeeds if command ${nameOrIndex} doesn't exist`, () => {
const result = createController(condition).listen([commands[0]]);

emitFakeCloseEvent(commands[0], { exitCode: 0 });
scheduler.flush();

return expect(result).resolves.toEqual(expect.anything());
return expect(result).resolves.toEqual(expect.anything());
});
});
});
7 changes: 4 additions & 3 deletions src/completion-listener.ts
View file
Open in desktop
Expand Up @@ -68,12 +68,12 @@ export class CompletionListener {
({ command, index }) => command.name === nameOrIndex || index === Number(nameOrIndex),
);
if (this.successCondition.startsWith('!')) {
// All commands except the specified ones must exit succesfully
// All commands except the specified ones must exit successfully
return events.every(
(event) => targetCommandsEvents.includes(event) || event.exitCode === 0,
);
}
// Only the specified commands must exit succesfully
// Only the specified commands must exit successfully
return (
targetCommandsEvents.length > 0 &&
targetCommandsEvents.every((event) => event.exitCode === 0)
Expand All @@ -86,7 +86,8 @@ export class CompletionListener {
* @returns A Promise that resolves if the success condition is met, or rejects otherwise.
*/
listen(commands: Command[]): Promise<CloseEvent[]> {
const closeStreams = commands.map((command) => command.close);
const closeStreams = commands.map((command) => command.close.pipe(take(1)));

return Rx.lastValueFrom(
Rx.merge(...closeStreams).pipe(
bufferCount(closeStreams.length),
Copy link
Member

Choose a reason for hiding this comment

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

So how would u recommend proceeding?

I think that replacing these lines with Rx.combineLatest() + removing the take(1) above can achieve retaining the last emitted close event.

We can then also fix your edge case from #464 (comment) with a filter() operator that checks if every command is indeed closed.
This means waiting until every command emits another close event.

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea that sounds legit, at-least the first part, seems like combineLatest can do the job 👍 Ill try to change the implementation accordingly

Copy link
Contributor Author

@odeadglaz odeadglaz Feb 25, 2024 •
edited

Choose a reason for hiding this comment

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

@gustavohenke I've pushed the changes, the only drawback using the combineLatest is that it actually keep the close events according to the registered stream and not by the actual order they were emitted, So beside accepting your offer I also added a map to sort it via the startingDate of the event, would love to hear if you got any better suggestion here 👍

Expand Down