fix appveyor build #24
Conversation
Current coverage is 100% (diff: 100%)@@ master #24 diff @@
===================================
Files 1 1
Lines 10 13 +3
Methods 0 0
Messages 0 0
Branches 0 0
===================================
+ Hits 10 13 +3
Misses 0 0
Partials 0 0
|
| @@ -1,3 +1,5 @@ | |||
| // does nothing child process | |||
| console.log("Child process.id: "+process.pid); | |||
| console.log(" - - - - - - - - - - - - - - - - - - - - - - - "); | |||
| // keep process alive for one minute | |||
| setTimeout(function () {}, 60000); | |||
There was a problem hiding this comment.
@jedwards1211 please clarify why this timeout is required and why it has to be for 60 sec. thanks!
There was a problem hiding this comment.
Maybe it's not necessary, I got confused because the windows tests are still very timing-dependent, but I was surprised the tests were ever working because the child processes will exit once they finish those two console.logs. Using setTimeout keeps the process alive. The timeout definitely doesn't have to be for one minute though. It could just be a bit longer than the duration of the tests.
There was a problem hiding this comment.
As far as the timing of windows tests, I guess the best thing is to make the first test poll until all the child processes are gone, with a test timeout of 5 seconds or so (sometimes it takes more than 1 second for the child processes to go away).
There was a problem hiding this comment.
do you want to go ahead and merge this, or do you want me to implement polling?
Polling does have one drawback, I realized. It's unlikely but possible that a new process starts with the same pid as one that was killed, causing terminate to poll forever.
I have implemented polling in my crater-util package (ironically I ended up using that instead of this) and it works well enough for my integration tests though.
There was a problem hiding this comment.
Ok. let's merge it and publish a new version. if we get a bug report we can go from there. 👍
fix #9
Note: depends on #20 and #23