feat(child process): Make all child processes silent

[nicojs]

Make all test runner and transpiler child processes silent. The standard out and standard error (stdout and stderr) are now only visible when loglevel: 'trace'. If a child process crashes, the last 10 messages received are logged as warning.

This is also a refactoring of the way we spawn child processes. Instead of having 2 similar implementations (one for transpiler and one for test runners), they are both consolidated in one coherent ChildProcessProxy abstraction.

Also clean up the test runner decorator pattern. Timeouts and retries are now implemented only once. Recognizing that the child process crashed is done by validating that the error is an instance of ChildProcessCrashedError. No process specifics other than the name of the error is known from the outside.

The Task class is also refactored. Instead of relying on a custom implementation, it uses the Promise.race method for timeout functionality.

Fixes #1038 #976

[nicojs]

This is ready to be merged IMHO

[nicojs]

@simondel @JoshuaKGoldberg With this PR we right now log this if a process runs out-of-memory:

20:54:02 (2296) WARN ChildProcessProxy Child process [pid 14360] exited unexpectedly with exit code 134 (without signal). Last part of stdout and stderr was: 
	FATAL ERROR: Ineffective mark-compacts near heap limit Allocation failed - JavaScript heap out of memory
	 1: 00007FF713BD82F5 
	 2: 00007FF713BB4156 
	 3: 00007FF713BB5890 
	 4: 00007FF7140AAD5E 
	 5: 00007FF7140AAC93 
	 6: 00007FF713F 89CB4 
	 7: 00007FF713F80797 
	 8: 00007FF713F7ED1C 
	 9: 00007FF713F87975 
	10: 00007FF713E8BB21 
	11: 00007FF713E8C5E2 
	12: 00007FF713E6CC0B 
	13: 00007FF713E4277A 
	14: 00007FF713E42F70 
	15: 00007FF713E48BC0 
	16: 00007FF7141B817D 
	17: 00007FF713E4A288 
	18: 00007FF713EEE39D 
	19: 00007FF713EEE6CE 
	20: 000003AACAF841C1 
 Error: Child process exited unexpectedly (code 134)
    at ChildProcessProxy.handleUnexpectedExit (C:\z\github\stryker-mutator\stryker\packages\stryker\src\child-proxy\ChildProcessProxy.js:147:29)
    at ChildProcess.emit (events.js:182:13)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:237:12)

I'm thinking of changing that. If a process crashes and "JavaScript heap out of memory" is in the output we can log a different kind of warning. Simply something like "Test runner process [PID] ran out of memory. You probably have a memory leak in your tests. Don't worry, Stryker will restart the process, but you might want to investigate."

[simondel]

Nice! And what if Stryker itself runs out of memory?

[nicojs]

If Stryker itself runs out of memory we cannot do anything to intercept right? As we're out of memory (non-interceptable)

[simondel]

Good point!

[nicojs]

I've implemented the improved error message. I also added an integration test for it (it turns out that creating a memory leak is easy as pie :)). Ready for review

[simondel]

Will this value ever change during execution? If not, I would suggest just reading the value on the line if (traceEnabled)

[nicojs]

Good point. Will change that

[simondel]

In what case would you cant to use T[K] extends MethodPromised ? T[K]? Can't you just always call back on T[K] extends Function ? MethodPromised?

[nicojs]

Not sure what you are getting at. There are 3 scenarios supported here. In case the method already returns a promise, it is left alone. In case the method returns something else, it is changed to returning a promise. In case it's not a method, it is mapped to a method returning a promise of that type. This is not perfect (still missing the type arguments in some cases), but is the best I could came up with.

[nicojs]

Ah I see what you mean. You mean change it to this:

export type Promisified<T> = {
  [K in keyof T]: T[K] extends Function ? MethodPromised : () => Promise<T[K]>;
};

The problem is that you don't really want to use MethodPromised if you don't have to. It doesn't help you with method types, converting it to (...args: any[]): Promise<any>. It would be awesome if we could keep the method signature, but I don't know how. I've created a stack overflow question for this:

https://stackoverflow.com/questions/51587576/using-generics-with-mapped-conditional-types-in-typescript

I would say: let's keep it like this for now.

[nicojs]

Wow! We have an answer on stack overflow! Fast. It seems that it is possible using infer. The problem now is that we do not have support for rest type parameters yet. So needs to be implemented using the death by a thousand overloads syntax. Let's keep it like it is for now and improve once TS 3 is out.

// TODO: Implement using unknown and rest parameter type
// https://blogs.msdn.microsoft.com/typescript/2018/07/12/announcing-typescript-3-0-rc/#tuples-and-parameters
interface Func1<R> {
  (): R;
}
interface Func2<T1, R> {
  (arg: T1): R;
}
interface Func3<T1, T2, R> {
  (arg: T1, arg2: T2): R;
}
interface Func<R> {
  (...args: any[]): R;
}
interface PromisedFunc1<R> {
  (): Promise<R>;
}
interface PromisedFunc2<T1, R> {
  (arg: T1): Promise<R>;
}
interface PromisedFunc3<T1, T2, R> {
  (arg: T1, arg2: T2): Promise<R>;
}
interface PromisedFunc<R> {
  (...args: any[]): Promise<R>;
}

export type Promisified<T> = {
  [K in keyof T]: T[K] extends { (...args: any[]): Promise<any> } ? T[K] :
  T[K] extends Func3<infer T1, infer T2, infer R> ? PromisedFunc3<T1, T2, R> :
  T[K] extends Func2<infer T1, infer R> ? PromisedFunc2<T1, R> :
  T[K] extends Func1<infer R> ? PromisedFunc1<R> :
  T[K] extends Func<infer R> ? PromisedFunc<R> :
  () => Promise<T[K]>;
};

[nicojs]

As TS 3 is out since a couple of hours, I've implemented this in the latest commit. Awesome!

[simondel]

Can we save the exit and error worker messages on the worker type itself so we don't have to use magic strings? Or are they a part of NodeJS?

[nicojs]

What do you mean? The worker here is an instance of ChildProcess right?

[simondel]

Do you mean NodeJS's ChildPRocess? Do we choose to name the event 'exit' or does NodeJS determine this name? If NodeJS determines it, this code is fine. If we personally have code that raises an event with name 'exit' then we should use a constant.

[nicojs]

Yes, it's a node thing. It works based on events. Not too great if you ask me, but that's how it works.

[simondel]

How does this work?

[nicojs]

It's the same as used by istanbul. It's a way to always grab the global object. I could change it to global.__coverage__. that should be fine. Agreed?

[simondel]

Sounds good!

[nicojs]

@simondel Thanks for the review. I've refactored some code and commented on the rest. Could you take another look?