Conversation
|
The current set of changes fail identically (due to timeout) in all versions of Mocha. |
| helpers.clearRejections = function () { | ||
| var listeners = process.listeners('unhandledRejection'); | ||
| process.removeAllListeners('unhandledRejections'); | ||
| process.removeAllListeners('unhandledRejection'); |
There was a problem hiding this comment.
This was the real problem with the tests.
test/helpers/handlers.js
Outdated
| it('.handle()', function (done) { | ||
| var msg = new Date().toString(); | ||
| var writeable = helpers.writeable(function (info) { | ||
| console.log('in writeable', info); |
There was a problem hiding this comment.
Called as expected for the exception handler but never for the rejection handler.
There was a problem hiding this comment.
Makes sense... excellent eye @fearphage 💯 Does this allow tests to pass in mocha@9 (or whatever the latest version is)?
There was a problem hiding this comment.
No, these changes cause the rejection handler test to fail consistently (due to timeout) accross all versions of Mocha (8 and 9).
So this appears to be a code issue and the previously bad test hid this for a while. I don't know if this is a test problem or a code problem.
The event is attached to the handler (as confirmed by the tests), but firing the appropriate event doesn't trigger the stream to be written. I don't get it. I could use another set of eyes on it for sure.
There was a problem hiding this comment.
@wbt @DABH Anything jump out as broken or misspelled here? I'm stuck.
Either the test is bad OR the code is bad... or possibly both. I'm going to take a break and circle back to it in a day or two to see if that gives me some perspective.
Edit: This test passes for the exception handler, but only fails for the rejection handler.
There was a problem hiding this comment.
Sorry, I have not had time to look into this, and I'm not super optimistic about being able to give much time to this over the next few days at least 😞
DABH
left a comment
DABH
left a comment
There was a problem hiding this comment.
I think this looks good and is a nice quality improvement to these tests. And looks like all the tests pass. Thank you @fearphage for your patience and persistence with this PR, and excited to merge it at last!
3 participants
Fixes #2018