Skip to content
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.

Sign up for GitHub

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

Assert: Improve rejects/throws validation handling #1635

Merged
merged 10 commits into from
Jul 26, 2021
Merged

Assert: Improve rejects/throws validation handling #1635

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
68af124
add more validation in rejects
smcclure15 Jul 5, 2021
ee3b1a3
add checks in throws to match rejects' logic
smcclure15 Jul 5, 2021
df87aed
consolidate throws/rejects validation
smcclure15 Jul 5, 2021
68aad74
line strip
smcclure15 Jul 6, 2021
939ab66
Add stricter validation checks
smcclure15 Jul 13, 2021
6344ba7
omit legacy alias "raises" in error message
smcclure15 Jul 14, 2021
bbeb736
relax expected type-checking
smcclure15 Jul 15, 2021
ebe0a53
Merge branch 'main' into rejects-fat-arrow
smcclure15 Jul 15, 2021
d9f9a5f
throws: fail if actual value is not a function
smcclure15 Jul 16, 2021
a17ef1a
correct test inputs/diagnostics
smcclure15 Jul 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
104 changes: 50 additions & 54 deletions src/assert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,20 +295,9 @@ class Assert {
let actual,
result = false;

const currentTest = ( this instanceof Assert && this.test ) || config.current;
[ expected, message ] = validateExpectedExceptionArgs( expected, message, "throws/raises" );
smcclure15 marked this conversation as resolved.
Show resolved Hide resolved

// 'expected' is optional unless doing string comparison
if ( objectType( expected ) === "string" ) {
if ( message == null ) {
message = expected;
expected = null;
} else {
throw new Error(
"throws/raises does not accept a string value for the expected argument.\n" +
"Use a non-string object value (e.g. regExp) instead if it's necessary."
);
}
}
const currentTest = ( this instanceof Assert && this.test ) || config.current;

currentTest.ignoreGlobalErrors = true;
try {
Expand All @@ -319,10 +308,7 @@ class Assert {
currentTest.ignoreGlobalErrors = false;

if ( actual ) {
const data = validateException( actual, expected, message, currentTest, "throws" );
result = data.result;
expected = data.expected;
message = data.message;
[ result, expected, message ] = validateException( actual, expected, message );
}

currentTest.assert.pushResult( {
Expand All @@ -337,26 +323,9 @@ class Assert {

rejects( promise, expected, message ) {

const currentTest = ( this instanceof Assert && this.test ) || config.current;

// 'expected' is optional unless doing string comparison
if ( objectType( expected ) === "string" ) {
if ( message === undefined ) {
message = expected;
expected = undefined;
} else {
message = "assert.rejects does not accept a string value for the expected " +
"argument.\nUse a non-string object value (e.g. validator function) instead " +
"if necessary.";

currentTest.assert.pushResult( {
result: false,
message: message
} );
[ expected, message ] = validateExpectedExceptionArgs( expected, message, "rejects" );

return;
}
}
const currentTest = ( this instanceof Assert && this.test ) || config.current;

const then = promise && promise.then;
if ( objectType( then ) !== "function" ) {
Expand Down Expand Up @@ -390,27 +359,65 @@ class Assert {
},

function handleRejection( actual ) {

const data = validateException( actual, expected, message, currentTest, "rejects" );
let result;
[ result, expected, message ] = validateException( actual, expected, message );

currentTest.assert.pushResult( {
result: data.result,
result,

// leave rejection value of undefined as-is
actual: actual && errorString( actual ),
expected: data.expected,
message: data.message
expected,
message
} );
done();
}
);
}
}

function validateException( actual, expected, message, currentTest, assertionMethod ) {
function validateExpectedExceptionArgs( expected, message, assertionMethod ) {
const expectedType = objectType( expected );

// 'expected' is optional unless doing string comparison
if ( expectedType === "string" ) {
if ( message === undefined ) {
message = expected;
expected = undefined;
return [ expected, message ];
} else {
throw new Error(
"assert." + assertionMethod +
" does not accept a string value for the expected argument.\n" +
"Use a non-string object value (e.g. RegExp or validator function) " +
"instead if necessary."
);
}
}

const valid =
expected === undefined ||
expected === null ||
expectedType === "regexp" ||
expectedType === "function" ||
expectedType === "object";

if ( !valid ) {
const message =
"Invalid expected value type (" + expectedType + ") " +
"provided to assert." + assertionMethod + ".";
throw new Error( message );
}

return [ expected, message ];
}

function validateException( actual, expected, message ) {
let result = false;
const expectedType = objectType( expected );

// These branches should be exhaustive, based on validation done in validateExpectedException

// We don't want to validate
if ( expected === undefined || expected === null ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't confirmed it, but it seems from reading the removed code, that previously a null expected value would reach the else branch that reports "invalid expected value provided". If so, did you mean to change that?

I didn't actually realize that null (non-object and unboxable primitive) was valid in a throw statement, but it looks like it actually is possible to throw null (the same way strings and numbers can be thrown).

I'm neutral on whether we need to support that, but I think the current state might be a bit strange, to have null behave the same as no argument, e.g. when would one use assert.throws(fn, null) instead of assert.throws(fn)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old throws logic actually used a !expected check
(assert.js#L325), so undefined, null, false, 0, etc could all make it through here to be "valid" with no matching at all, just that it threw.

The old rejects logic used a more explicit expected === undefined check (assert.js#L435), so the null/false/0 sort of values would be considered invalid and produce failures. That made for some inconsistencies between the two.

To target the "nullish" values, I went with expected === undefined || expected === null. So I've restricted the null/false/0 values for both (which now throw, not fail), but now a null expected value for rejects has changed from an invalid type to a valid/ignored one. I could go one further and restrict it to only undefined, so that a null value would error as well for both.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think any value other than undefined should either be considered invalid, or be used as comparison target in some way. Using null in this way would be unexpected, in my opinion. I did not realize that assert.throws() was implicitly allowing null and other falsely values as if they are undefined. We probably shouldn't change that in a minor release.

Basically, undefined is here to represent the case of no expected parameter being passed and asserting only that something was thrown. A stricter version of this code would have used arguments.length to detect that, instead of comparing to undefined.

Two ideas:

  1. In this commit, change both to use !expected (assuming empty string remains invalid and handled earlier). That should not cause any previously passing code to start failing, and will temporarily lax rejects() to start tolerating any other garbage passed to it. Assumign that people's existing tests are passing, this should not change any behaviour, but will add a temporary blind splot. Then in 3.0 we could change it to === undefined (or arguments.length) and consider unhandled types as invalid.
  2. Alternatively, perhaps vary this section of the code by method such that the behaviour remains the same for both. The throws branch accepts falsey as alias for undefined, the rejects branch considers non-undefined falsey values as invalid. The throws branch can then be deprecated in a follow-up commit.

result = true;
Expand Down Expand Up @@ -450,20 +457,9 @@ function validateException( actual, expected, message, currentTest, assertionMet
// assign the "expected" to a nice error string to communicate the local failure to the user
expected = errorString( e );
}

// Expected is some other invalid type
} else {
result = false;
message = "invalid expected value provided to `assert." + assertionMethod + "` " +
"callback in \"" + currentTest.testName + "\": " +
expectedType + ".";
}

return {
result,
expected,
message
};
return [ result, expected, message ];
}

// Provide an alternative to assert.throws(), for environments that consider throws a reserved word
Expand Down
134 changes: 85 additions & 49 deletions test/main/assert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ QUnit.test( "throws", function( assert ) {
"expected is a string",
"message is non-null"
);
}, /throws\/raises does not accept a string value for the expected argument/ );
}, /^Error: assert\.throws\/raises does not accept a string value for the expected argument/ );

// This test is for IE 7 and prior which does not properly
// implement Error.prototype.toString
Expand Down Expand Up @@ -364,6 +364,42 @@ QUnit.test( "throws", function( assert ) {
/description/,
"throw error from property of 'this' context"
);

// the following are nested assertions, validating that it
// initially throws due to an invalid expected value

assert.throws(
function() {
assert.throws(
undefined, // irrelevant
2
);
},
/^Error: Invalid expected value type \(number\) provided to assert\.throws\/raises\.$/,
"throws errors when provided a number"
);

assert.throws(
function() {
assert.throws(
undefined, // irrelevant
false
);
},
/^Error: Invalid expected value type \(boolean\) provided to assert\.throws\/raises\.$/,
"throws errors when provided a boolean"
);

assert.throws(
function() {
assert.throws(
undefined, // irrelevant
[]
);
},
/^Error: Invalid expected value type \(array\) provided to assert\.throws\/raises\.$/,
"throws errors when provided an array"
);
} );

QUnit.test( "raises", function( assert ) {
Expand Down Expand Up @@ -483,6 +519,54 @@ QUnit.test( "rejects", function( assert ) {
buildMockPromise( undefined ),
"reject with undefined against no matcher"
);

// the following are nested assertions, validating that it
// initially throws due to an invalid expected value

assert.throws(
function() {
assert.rejects(
undefined, // irrelevant
2
);
},
/^Error: Invalid expected value type \(number\) provided to assert\.rejects\.$/,
"rejects errors when provided a number"
);

assert.throws(
function() {
assert.rejects(
undefined, // irrelevant
false
);
},
/^Error: Invalid expected value type \(boolean\) provided to assert\.rejects\.$/,
"rejects errors when provided a boolean"
);

assert.throws(
function() {
assert.rejects(
undefined, // irrelevant
[]
);
},
/^Error: Invalid expected value type \(array\) provided to assert\.rejects\.$/,
"rejects errors when provided an array"
);

assert.throws(
function() {
assert.rejects(
undefined, // irrelevant
"expected is a string",
"message is non-null"
);
},
/^Error: assert\.rejects does not accept a string value for the expected argument/,
"rejects errors when provided a string"
);
} );

QUnit.module( "failing assertions", {
Expand Down Expand Up @@ -614,30 +698,6 @@ QUnit.test( "throws", function( assert ) {
},
"throws fail when expected function returns false"
);

assert.throws(
function() {
throw "foo";
},
2,
"throws fails when provided a number"
);

assert.rejects(
function() {
throw "foo";
},
false,
"throws fails when provided a boolean"
);

assert.rejects(
function() {
throw "foo";
},
[],
"throws fails when provided an array"
);
} );

QUnit.test( "rejects", function( assert ) {
Expand Down Expand Up @@ -673,28 +733,4 @@ QUnit.test( "rejects", function( assert ) {
);

assert.rejects( null );

assert.rejects(
buildMockPromise( "foo" ),
2,
"rejects fails when provided a number"
);

assert.rejects(
buildMockPromise( "foo" ),
"string matcher",
"rejects fails when provided a string"
);

assert.rejects(
buildMockPromise( "foo" ),
false,
"rejects fails when provided a boolean"
);

assert.rejects(
buildMockPromise( "foo" ),
[],
"rejects fails when provided an array"
);
} );