-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 global reporting in parallel mode (fixes #1205) #1861
Conversation
I don't fully understand the "why" as of yet, but it looks like the global-reporter is getting an undefined reporter passed to the constructor. The reporter param is actually supposed to be a string representing either a path to a reporter, or the name of a built-in reporter. As a workaround for the failing tests, you could change line 26 in global-reporter.js to say: This will make the tests pass, but it is a bit of a hack. I don't fully understand how the code paths work yet, so it's hard for me to say why that is happening. |
@lreading thanks for the tip, I'll wait for @beatfactor to chip in on this, he might be able to confirm your hypothesis |
This issue has been automatically marked as stale because it has not had any recent activity. |
Not stale, @beatfactor please take a look whenever you can - not sure whether this is still relevant with the latest version. If it could be then I will rebase |
7cd76fe
to
7d744d2
Compare
@beatfactor I've rebased the branch and included the fix mentioned by @lreading above. It looks like it is working, and I cannot reproduce the bug anymore after testing 1206 here. I tried adding a test case for the bug I'm fixing but didn't manage to, maybe it will be easier for you or you can point me in the right direction |
7d744d2
to
501c918
Compare
lib/runner/test-runners/default.js
Outdated
return this.concurrency | ||
.runMultiple(testEnvArray, modules) | ||
.then(exitCode => { | ||
this.reportResults(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.reportResults()
returns a Promise so we should wait for it.
this.reportResults(); | |
return this.reportResults().then(hasFailures => { | |
return exitCode | |
}) |
501c918
to
78be0e2
Compare
This is an attempt at fixing #1205 - I have fixed the original bug but some tests are failing and I cannot understand why. The test output is not very verbose and it does not point me in any direction.
Also I would have like to add a test case that reproduces the original bug but was not able to.
I'm opening this as WIP because I need some pointers