Watcher integration test

[craigtaub]

Description of the Change

• Attempt at adding basic watcher integration test.
• Asserts output of a "watched" process, which is terminated (checks correct listener is run).
• uses exit.fixture to keep process hanging inside spawned child.
• Code block it tests is located https://github.com/mochajs/mocha/blob/master/bin/_mocha#L581.
• Am hoping to look at adding an Int test to check for reload when file has changed, in the future

Benefits

• Increases general coverage
• Gives coverage to a useful feature

Possible Drawbacks

• slow down tests as take another .5 seconds to run.

Applicable issues

• Issue: "watch" functionality needs tests #3270
• Does not impact production code.
• Patch release.

[craigtaub]

CURRENTLY: Investigating failures (in process of debugging).

[craigtaub]

Issue seems to be AppVeyor's Windows not receiving the SIGINT signal, despite using the readline approach.
Will try with Windows VM.

[craigtaub]

Replicated on local Windows VM, does appear to be SIGINT not being received on spawned process.
Have been testing this in isolation (here https://github.com/craigtaub/child_process_experiments, usually with only parent or parent+child).

The problem appears to be due to Windows signals support. The signal simulation appears to ONLY work on manual terminal termination and does not proxy (i.e. only on real user input. Currently no supported way to pass CTRL+C programmatically). Its a problem as the tests run Mocha in a child process.

TESTED IN ISOLATION

• code https://github.com/craigtaub/child_process_experiments/tree/parentChildProxy
• scenario:
  • parent blocking (on child stdout i.e. child console.logs), child blocking, parent timer sends kill to child
• Results:
  • Unix - SIGINT proxies to child (after parent timer fires SIGINT, falls into childs process.SIGINT)
  • Windows - SIGINT doesn’t proxy to child. Just exits. Even with RL lib (readline).

[craigtaub]

MY SOLUTION - i have had to ignore the test on Windows. I am happy to try other solutions if people have ideas but i did not find a way to do this without re-writing a load (e.g. switch to using messages).
This part of watch feature has coverage on unix now at least.

[Bamieh]

@craigtaub Thanks for your much needed contribution!

Can you check this solution for windows to check for sigint and manually close the process?
https://stackoverflow.com/a/14861513/5384679

[craigtaub]

hi @Bamieh
Unfortunately that was the first thing I tried (see commit 5ae7932) and it did not help.
Tried it again on my experiment repo https://github.com/craigtaub/child_process_experiments/tree/parentChildProxy2 and still doesnt work (only on Windows, fine on Mac).
Seemed to have same issue (i.e. parents simulated termination never reaching childs rl.on('SIGINT')).
Was gutted I could not get it to work.

[Bamieh]

@craigtaub okay, I'll try to fiddle around with your code to give it another try. Although this is definitely better than having no tests IMO, specific implementation tests might introduce unexpected side effects, i believe @mochajs/core might have some input on this as well.

[craigtaub]

Ok @Bamieh please let me know if you make any progress. I have tried again to no avail.
I agree not ideal, but feels like half better than nothing.

[Bamieh]

@craigtaub I will let you know by tomorrow

[craigtaub]

Ok thanks @Bamieh .

[craigtaub]

Hey @Bamieh, is there any update on this?

[Bamieh]

@craigtaub Sorry for the delay, it is hard to get my hands on a windows computer. I was unable to find a solution with our current implementation, although I want to mention:

Windows does not support sending signals, but Node.js offers some emulation with process.kill(), and subprocess.kill(). Sending signal 0 can be used to test for the existence of a process. Sending SIGINT, SIGTERM, and SIGKILL cause the unconditional termination of the target process.

I am not confident about having falsely passing tests on certain platforms, I'm sure @boneskull have some thoughts on this.

[craigtaub]

@Bamieh completely sensible point.
Do you think you would be more confident if the entire test was not run on Windows? That way its not falsely passing, its just not covered for Windows, same situation we have now.

Also, have been trying with process.kill.

[Bamieh]

@craigtaub yes that would be much better i believe.

[craigtaub]

@Bamieh cool, have updated the PR to reflect this. Could you have a look please.

[Bamieh]

@craigtaub looks good, thank you

[craigtaub]

@Bamieh great. If there is nothing outstanding any chance we can merge it please? Happy to look at other watcher scenarios once it is in.