assert.throws() must not call mismatched class as matcher function #1530
Comments
|
Thanks the details and suggestions! (but is that really the right message?) We have a handful of "negative" assertion tests that this might fit into: For more power, we have CLI tests that execute a test in isolation, and then verify its TAP output: Note that while I was fiddling with your solution, I did notice that an existing unit test began failing: EDITEDI copy/pasted the wrong output that I observed. This uses the proposed fix, and the Still, the message is a little vague, and @Krinkle already has pointed out further complications that the failing unittests expose. I can also report that the "die" message with the current behavior, the individual test bails out (with effectively a hard-error), and other tests do continue after recovery. Only with |
Krinkle
added
Component: Assert
Component: Core
Krinkle
changed the title
assert.throws() Method Can Fail to Check Class Constructors
I am unable to reproduce this. Both in the HTML runner and in the QUnit CLI, I find that the error is caught, reported, and any further tests continue executing. It does not appear to die immediately in a way that is different from normal assertion failures. Of course, the error message is indeed incorrect. It should not be trying to call the class constructor as a validation callback (that is a different signature of assert.throws).
Aye, but I believe this would also break all uses of a matcher callback function, see https://api.qunitjs.com/assert/throws/:
Such as: assert.throws(
() => { throw new Error('foo'); },
(err) => err.message === 'undefined'
);Ambigous signatureI regret not having realised this before, but there is an ambiguity in the Signature 2 and 4 are ambiguous, and I suspect have been for a very long time. Basically when check 2 fails, we (wrongly) run check 4 as well and it's pretty hard to avoid that in a way that won't introduce other failures. The two are inherently ambiguous since a constructor function is really no different from any other function in JavaScript. assert.throws(
() => { throw RangeError(); },
RangeError
);
function CustomError(message) { this.message = message; }
assert.throws(
() => { throw CustomError('foo'); },
CustomError
);
assert.throws(
() => { throw RangeError('foo'); },
function (err) { return err.message === 'foo'; }
);Despite how bad this is, it seems we've never run into it, and I can see why. The issue only happens if the test was already going to fail, and if the constructor can throw an exception. The However, ES6 introduced classes with arrow-like constructor functions that throw by default if called without function Foo() {}
Foo(); // ok
class Bar {}
Bar();
// Uncaught TypeError: class constructors must be invoked with 'new'Bad handling for genuine callback functionAs I mentioned, I'm unable to reproduce the issue, but if the HTML reporter and/or the QUnit CLI are dieing unexpectedly from this exception, then we likely also have an additional issue, since these user-provided functions must be allowed to fail. For example: QUnit.test('example A', (assert) => {
assert.throws(
() => { throw new Error('foo'); },
(err) => { return errTypo.message === 'undefined' }
);
});
QUnit.test('example B', (assert) => {
class Bar {}
assert.throws(
() => { throw new Error('foo'); },
Bar
);
});The first one uses a matcher callback function with a typo causing it to throw a ReferenceError, and it seems to be handled correctly, same as for the reported case with an ES6 class: Like I said, the error message is incorrect for the seconc test, but I did not find the test runner stopping unexpectedly. But, if it does indeed crash unexpectedly, then we likely need to fix that as well, even if we can fix the ambiguity, because it would also be an issue for exceptions from "real" matcher functions. Path forwardLong-term, it seems that the only way to avoid this internal failure during check 4 is by introducing a stricter requirement for what we consider a "real" matcher function, and thus based on some hueristics (is/similar to ES6 class constructor) decide not to call it regardless of whether it would throw or not. I assume this can't be perfect, and thus would constitute a breaking change, and nothappen until QUnit 3. (I expect it to be very unlikely to affect anyone, and either way should be easy to fix when upgrading.) In the short-term, we can:
@smcclure15 Are you able to reproduce the crash? As for detecting it, I haven't looked closely yet, but one way might be to inspect the actual value with toString and look for it startig with |
|
@Krinkle I didn't notice the test runner breaking (though I wasn't really looking for that), just that the |
Krinkle
changed the title
|
@aaron-human Thanks, that makes sense. I was hoping it was that! In general QUnit will nicely provide all failed assertions at once. But, the "exception" is when there is an error throws unexpectedly by your test function or from other source code provided by you and called during a test. In this case, that is technically what is happening, but not supposed to. The failure you are encountering is essentially like the following: QUnit.test('example', (assert) => {
class CustomError {}
assert.throws(
() => { throw new Error('foo'); },
(err) => CustomError() // supposed to return a boolean, but throws TypeError
);
});… except unlike the silly example above, it is not your fault that |
|
There's a final // Expected is a validation function which returns true if validation passed
} else if ( expectedType === "function" && expected.call( {}, actual ) === true ) {
expected = null;
result = true;
}That's the function(err) {
notdefined() // this throws!
}We could more safely invoke the } else if ( expectedType === "function" ) {
try {
result = expected.call( {}, actual ) === true;
expected = null;
} catch ( e ) {
expected = errorString( e );
}
}and that will set the This doesn't consider the special case of ES6 classes, but I think it shows worthwhile forward progress on this cryptic failure. Plus with this sort of change, it looks like all existing unittests pass, which increases my confidence in such iterative improvements. |
|
@smcclure15 Agreed. It would display the same error message, but in relation to the assertion instead of as a "Died on test". In QUnit 3 we could go a step further and tighten it up such that class-like values won't be tried as a validation function. That way, it can fail as |
Krinkle
added
help welcome
and removed
Component: Core
…es not match Previously when the expected value was a class, and it did not satisfy instanceof, the class function was also tried as a matcher function which in the case of ES6 class constructors throws by default, which we did not internally catch, causing a rather abrupt test failure rather than a more informative one. Fixes #1530.
Tell us about your runtime:
localhost.What are you trying to do?
While testing a
assert.throws(), I noticed that if the expected value (a class/constructor) didn't match the exception, QUnit would "die" rather than print that the error was the wrong type.Code that reproduces the problem:
If you have any relevant configuration information, please include that here: None that I know of.
What did you expect to happen?
Should indicate that an exception was thrown, but it didn't match (failed and
instanceofcheck).What actually happened?
The test run failed immediately.
What I see as output:
I believe the issue is this code:
If the code was switched around to do this in the released JS file, it seems to work for the above check.
I was going to setup a PR, but I'm not sure how to write tests that verify a QUnit test page reports a failure...
The text was updated successfully, but these errors were encountered: