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

test(attach): improve disconnect test #414

Merged
merged 4 commits into from
Oct 11, 2024
Merged

test(attach): improve disconnect test #414

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4dcdc39
test(attach): improve disconnect test
gjf7 Oct 11, 2024
c4c1ddb
verify if this is caused by Jest's mock implementation
gjf7 Oct 11, 2024
eb3e086
listen on exit instead close
gjf7 Oct 11, 2024
b895486
Update packages/neovim/src/attach/attach.test.ts
justinmk Oct 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions packages/neovim/src/attach/attach.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,10 +157,12 @@ describe('Nvim API', () => {
it('emits "disconnect" after quit', done => {
const disconnectMock = jest.fn();
nvim.on('disconnect', disconnectMock);

nvim.quit();

proc.on('close', () => {
expect(disconnectMock.mock.calls.length).toBe(1);
// TODO: 'close' event sometimes does not emit. #414
proc.on('exit', () => {
expect(disconnectMock).toHaveBeenCalledTimes(1);
done();
Copy link
Member

@justinmk justinmk Oct 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

since the test is timing out, that means this done is not called, which means the 'close' event isn't emitted (or stuff is getting disposed before handling the emitted 'close' event).

This proc.on was added in c9a3dd0 for some reason. But later the proc.kill call was removed, so whatever reason that proc.on was added no longer exists.

we should just restore the old logic, which simply did nvim.on('disconnect', done);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I understand that, but how race could happen to cause this problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we still need the disconnect call on proc ?

Copy link
Member

Choose a reason for hiding this comment

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

oh, that causes the test harness (jest) to kill Nvim too early, which then raises EPIPE because Nvim still has messages to process.

});

Expand Down