test(attach): improve disconnect test

[gjf7]

Closes #314.

Problem:

This test may time out, because the proc.on('close') event is never triggered.

FAIL src/attach/attach.test.ts (5.62 s)
  ● Nvim API › emits "disconnect" after quit
    thrown: "Exceeded timeout of 5000 ms for a test while waiting for `done()` to be called.

      |   });
      |
    > |   it('emits "disconnect" after quit', done => {
      |   ^
      |     const disconnectMock = jest.fn();
      |     nvim.on('disconnect', disconnectMock);
      |     nvim.quit();

      at it (src/attach/attach.test.ts:138:3)
      at Object.describe (src/attach/attach.test.ts:6:1)

Solution:

Listen for 'exit' instead of 'close'. This may indicate that Nvim or node-client
is not properly closing all of its streams, but that is unrelated to this
particular test.

[justinmk]

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);

[gjf7]

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

[gjf7]

Do we still need the disconnect call on proc ?

[justinmk]

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

[gjf7]

Oops, failed again, so the mock function indeed not the issue here.

[justinmk]

Based on local runs, changing this to 'exit' seems more reliable. It's strange that 'close' is sometimes not emitted, but that was never the purpose of this test...

Suggested change

	proc.on('close', () => {
	proc.on('exit', () => {

[gjf7]

Make sense, we only want make sure disconnect is called.

[gjf7]

We can run CI a few more times to see if it's working.

[gjf7]

Looks like it happens on windows. see nodejs/node#4500

[justinmk]

CI has been green 10+ times 🎉

Looks like it happens on windows. see nodejs/node#4500

#314 happens on all platforms. Based on some local testing, it looks like stderr is not closed, and that would be why 'close' event does not emit. This may indicate a bug in node-client or Nvim.

[gjf7]

@justinmk Thanks for your guide!