-
-
Notifications
You must be signed in to change notification settings - Fork 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
fixing process.nexTick
(should be process.nextTick
), which invalidated tests
#2983
Conversation
Thanks for splitting this out! If we can figure out what's causing test failures and address it, then I'm definitely comfortable merging this fix to the tests. It would be nice to ensure they wouldn't have passed with the incorrect spelling in the first place, but the fact is that before this fix they're not even testing what they're supposed to and with this fix they are, so it's an unambiguous step in the right direction. Especially if there's a regression that we missed detecting before due to the incorrect spelling... |
process.nexTick
(should be process.nextTick
), which invalidated tests
Perhaps this needs |
I don't quite understand why we need |
It was a stab in the dark. And it didn't work. |
4da0335
to
4241900
Compare
ok, node-inspector was a total dead-end. But I managed to get We treat I had to move the I have no idea why one pair of seemingly unrelated tests is failing on one specific subplatform of one of two test systems. |
This is definitely not right -- we don't expect users to monkey-patch |
Ok. I'm not going to spend any more time on this. There's no obvious way to make uncaught cooperate. I tried setting allowUncaught on various things and stepping through, it didn't seem to help. And uncaught doesn't have any hooks. The to be called thing didn't match things, but it also didn't die which confused me... Anyway, good luck. I'm off to other projects. |
Ok, that's fine -- thanks for bringing the faulty tests to our attention nonetheless! We'll see if we can track down what's wrong in the source (if fiddling with |
fwiw, I think I've figured out why this happens. Look at this thing: it('should not pass if throwing async and test is async', function (done) {
var test = new Test('im async and throw null async', function (done2) {
process.nextTick(function () {
throw null;
});
});
// blah blah, run the test, and finish up
}); What happens when an asynchronous exception is thrown in Node.js? Well, the When do we register an In other words, the Potential solutions:
I'm going to see if 2. works. |
closing in lieu of #3130 with fix applied. |
Requirements
Description of the Change
This is intended to fix a spelling error -- Split from #2981.
Alternate Designs
Instead of fixing the spelling error, the misspelled token can be replaced by a clear call to a nonexistent (but properly spelled) method.
Why should this be in core?
This is existing test code (that happens to be incorrectly documenting what it claims to be testing).
Benefits
Once someone fixes this to work correctly it will be easier to understand what the test is testing.
Possible Drawbacks
process.nexTick
could be intentionally misspelled, but that's unlikely, if that's the goal, there are better ways to cause a similar error.Applicable issues
This isn't a proper bug fix at this time. Someone will have to decide how to properly fix it.