Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix xunit and json reporter crash when used with parallel mode #4623

Merged
merged 3 commits into from
Aug 26, 2021

Conversation

curtisman
Copy link
Contributor

@curtisman curtisman commented Apr 7, 2021

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
Copy link
Contributor Author

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

@curtisman curtisman closed this Apr 7, 2021
@coveralls
Copy link

coveralls commented Apr 7, 2021

Coverage Status

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

@curtisman
Copy link
Contributor Author

curtisman commented Jun 25, 2021

Reactivating PR and rebased after signing CLA

@curtisman
Copy link
Contributor Author

@juergba I believe this should fix #4624

@koerbcm
Copy link

koerbcm commented Jul 21, 2021

+1

@outsideris outsideris added type: bug a defect, confirmed by a maintainer area: parallel mode Regarding parallel mode area: reporters involving a specific reporter labels Jul 28, 2021
@koerbcm
Copy link

koerbcm commented Aug 10, 2021

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

@juergba
Copy link
Contributor

juergba commented Aug 24, 2021

@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.

lib/hook.js Outdated Show resolved Hide resolved
lib/hook.js Outdated Show resolved Hide resolved
lib/hook.js Outdated Show resolved Hide resolved
lib/hook.js Outdated Show resolved Hide resolved
lib/suite.js Outdated Show resolved Hide resolved
lib/suite.js Outdated Show resolved Hide resolved
lib/suite.js Outdated Show resolved Hide resolved
lib/test.js Outdated Show resolved Hide resolved
lib/test.js Outdated Show resolved Hide resolved
lib/test.js Outdated Show resolved Hide resolved
@curtisman
Copy link
Contributor Author

curtisman commented Aug 25, 2021

@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.

@curtisman curtisman changed the title fix xunit reporter crash when used with parallel mode fix xunit and json reporter crash when used with parallel mode Aug 25, 2021
@juergba
Copy link
Contributor

juergba commented Aug 25, 2021

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
Copy link
Contributor Author

curtisman commented Aug 25, 2021

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
Copy link
Contributor

juergba commented Aug 25, 2021

[...] 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 juergba added the run-browser-test run browser tests on forked PR if code is safe label Aug 25, 2021
@github-actions github-actions bot removed the run-browser-test run browser tests on forked PR if code is safe label Aug 25, 2021
Copy link
Contributor

@juergba juergba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@juergba juergba merged commit 9e0369b into mochajs:master Aug 26, 2021
@juergba juergba added this to the next milestone Aug 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: parallel mode Regarding parallel mode area: reporters involving a specific reporter type: bug a defect, confirmed by a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failures in Before and BeforeAll hooks cause xunit & json reports to fail in parallel mode
5 participants