Pin mocha to v8 as short-term test failure fix #2017
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@wbt You are a rockstar! Seriously, thank you for getting to the bottom of this. I think we are unblocked for v3.4.0 now. Cheers! |
|
Issue #2018 has been created to track the better solution. |
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Picking up the thread from #1988, we have been seeing test failures recently.
This PR aims to address that the quickest & easiest way. The better way is to update the test to loop over listeners looking for the expected one instead, and update mocha to run with the latest.
Reasoning: Looking at actions, as noted in the other thread the 'v3.3.4' commit appears to be the culprit. What changed there? Not much. However, looking at package lock, I see things that mocha was updated from 8.0.1 to 8.4.0. In the "fix all high-severity vulnerabilities from npm audit" commit before the npm publication but after the GitHub tag, mocha was updated to 9.1.3, a breaking change.
In pinning mocha to different versions and re-running tests, I found that the breaking change was introduced on the feature version of mocha’s update from 8.1.3 to 8.2.0, i.e. definitely somewhere in here. I’m going to make a pretty strong guess it was this commit in this PR, probably in this line (though this new one seemed suspicious, I don't think that's as likely). There are multiple places where new lines remove or verify removal of unhandled rejection listeners, so that’s not exactly by accident.
I'm not about to try & undo that commit. This PR pins mocha to the last version before that breaking change, and I recommend that it soon be replaced by one that actually fixes the test and updates Mocha.