Tests: Patch mocha bugs and fix broken async flow cases #6157
Conversation
It's to ensure it doesn't interfere with async leaks detection
It's to provide an option to run isolated tests in parallel
It allows to pursue faster testing of async leaks
So it doesn't produce false positives in Node.js v6
There was a problem hiding this comment.
Works great and looks good code-wise.
I just have one question regarding native Promises vs. Bluebird. Should we stick to one convention here? While we don't mix those implementations in one file (which we did and had some race condition bugs resulting from that) it might be worthwhile to settle on one implementation to keep the codebase consistent.
| }; | ||
| const initialSetupDeferred = shouldApplyFsCleanupCheck | ||
| ? initialGitStatusDeferred | ||
| : Promise.resolve(); |
There was a problem hiding this comment.
This is a question which pops up all the time. Should we consistently use Bluebird or should we mix and match Bluebird with native Promises?
There was a problem hiding this comment.
I totally agree, and I definitely would not try to mix Bluebird with native promises in framework modules (especially that it may affect long stack traces which become handy)
Still this is an outer stand alone script, that by no means is mixed with framework logic or even invokes framework logic (note tests are run in other processes released with spawn)
It just distributes test runs, and also already uses three dependencies (p-lmit, globby, child-process-ext) which output native promises.
Also at some point (the sooner the better) we should switch to native promises, so configuring it with Bluebird (still partially) will imply an unnecessary need of refactor here.
Therefore I think it's totally fine to stick to native promises in this context.
Addresses #6116 (not closing yet as there are some further steps to make)
Derives from #6147 (which should be merged first)
Test ended with unfinished async jobserror is thrownIs this ready for review?: YES
Is it a breaking change?: NO