fix xunit and json reporter crash when used with parallel mode

[curtisman]

Requirements

• Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
• All new code requires tests to ensure against regressions.

Description of the Change

See this comment #4453 (comment)

This change fixes the various issue encountered when using parallel mode with the xunit and json reporter when error happens.

All the changes are in Test, Suite and Hook's serialize function

1. Force serialization of the value for $$isPending to boolean to remove the possibility that it is undefined which is not serialized.
2. Additional field to serialize in Hook.prototype.serialized():
   • state, file, duration, fullTitle(), currentRetry() in the base object
   • parent.fullTitle() in the parent object

Why should this be in core?

Fixes broken functionality in the core

Benefits

xunit and json reporter can be used in parallel mode

Possible Drawbacks

I didn't fully look at all the possible reporters mentioned in #4453 and more fixes may be needed to address the full scope.

Applicable issues

Fix #4624
Partial bug fix to #4453

[curtisman]

Oops. Need to figure out the CLA first. Closing the PR for now.

[coveralls]

Coverage remained the same at 94.403% when pulling 4c7589c on curtisman:parallel-xunit into 014e47a on mochajs:master.

[curtisman]

Reactivating PR and rebased after signing CLA

[curtisman]

@juergba I believe this should fix #4624

[koerbcm]

+1

[koerbcm]

This change would be very helpful for our organization. Any idea when this PR might pick up traction?

[juergba]

@curtisman thank you for this PR. Sorry for the delay, are you still around?

The communication between processes (IPC) with its de-/serialization is a time-costly matter. We have to be very selective with the data we are transmitting, otherwise the parallel mode turns out to be slower than a serial run.

1. serialized parent fully by calling serialize (also call serialize for Hook's ctx.currentTest as well)

This is too generous. I will have a look which properties we will need. At a first look, adding just $$fullTitle: this.parent.fullTitle() seems to be sufficient.

[curtisman]

@juergba I reverted the change to fully serialized nested objects. Although, I feel like this will just turn in the whack-a-mole, as I don't think there is a good documentation on what reporter are expected to have access to in these objects, and other reporters might us a fuller set of data here. I am wondering how much these extra data actually cost.

Additionally, I tested out my original repro in #4453 (comment) with the json reporter as well, and discover a set of property that needed to be added to the Hook as well (currentRetry(), fullTitle(), duration, file) to make output the same with and without --parallel, and with that, it should fully fix #4624.

[juergba]

Although, I feel like this will just turn in the whack-a-mole [...]

Yes, I feel the same way. This entire IPC implementation with its de-/serialization appears so clumsy to me. "speed" is the only argument which could justify the additional maintenance burden we have with parallel mode. I don't want to risk loosing any speed.

So be prepared for more nagging on your JSON fix.

[curtisman]

I don't want to risk loosing any speed.

I totally hear you there. For now, I am fully ok with just getting this PR in following the pattern of what is there already, to fix the bug to unblock people.

I poked around the code a little bit (specifically deserialization), and there are some pretty easy improvements that can be made to speed that part up, but I don't know whether it will have any effect on the overall scenarios that people care about.

What scenarios are we looking at for measuring speed? Any data on which part of it causing speed issues (serialization? deserialization? IPC between worker and main process? contention on the main process when receiving IPC?...)

[juergba]

[...] and there are some pretty easy improvements that can be made [...]

yes please, go ahead, your PRs are very welcome.

What scenarios are we looking at for measuring speed?

Nothing sophisticated, just the total time. The user sitting in front of his/her terminal: serial vs parallel.
The communication main process=>worker is small (just the options?)
You can't really influence or customize the IPC chanel.
So the leverage point is worker=>main, the de-/serialization of the workers' event data.
Back on main, there is little happening. Just emitting the buffered events and creating the report.

I can recommend to read:

• essay: by the author of parallel mode
• mochawesome: implementing parallel, with too little event data

[juergba]

@curtisman thank you!
No tests are ok for now. There will be more parallel fixes to come anyway.