Assert: Improve rejects/throws validation handling

[smcclure15]

Resolves #1630 and more...

I noticed that rejects was also subject to #1567, which was identified and fixed only for throws; the solution was the same. Comparing the logic side-by-side, I also noticed that we could improve the "invalid expected type" case, where we used to have:

QUnit.test( "demo, function( assert ) {
    assert.throws(
        () => { throw new Error( "foo" ) },
        2,
        "demo"
    );
} );

TAP version 13
not ok 1 demo
  ---
  message: "demo"
  severity: failed
  actual  : Error: foo
  expected: 2
  stack: |
        at Object.<anonymous> (/SNIP/demo.js:7:9)
  ...
1..1
# pass 0
# skip 0
# todo 0
# fail 1

but now that reads

  message: "invalid expected value provided to `assert.throws` callback in \"demo\": number."
  severity: failed
  actual  : Error: foo
  expected: 2
  stack: |
        at Object.<anonymous> (/SNIP/demo.js:7:9)

just as rejects handled it.

Once all the logic/behavior was more aligned, it was fairly trivial to consolidate the logic into one reusable validation-helper. This should protect us from finding/fixing one and not the other.

[Krinkle]

I think we may want to throw rather than push a failure for this one.

[smcclure15]

Sounds great, thanks! I'll touch this up, though it might take a week with my schedule.

[Krinkle]

Left a question regarding null. I might be missing something, let me know what you meant there!

[Krinkle]

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)?

[smcclure15]

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?

[Krinkle]

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.

[Krinkle]

I'll try to summarise what we did and to confirm intentions:

• Validation and matching are now the same between assert.throws and assert.rejects.
  • This fixed the inability to use arrow functions with rejects(), as previously fixed for throws(). – This fixes assert.rejects can error if a fat-arrow function is used #1630.
  • This (temporarily) loosens assert.rejects to tolerate and silently ignore all falsey expected value. We'll deprecate it for both in a follow-up patch.
• Non-function values passed as block to assert.throws() now result in a simulated assertion failure with descriptive error message, instead of previously where we failed via block.call() throwing a TypeError which the user presumably would not have been (wanting to be) matching. – This fixes assert.throws can succeed if actual value is not a function and no matchers are used #1637.

Is that right?

[smcclure15]

I'll try to summarise what we did and to confirm intentions:
[...]
Is that right?

Yes; that's a good summary of these sprawling changes, thanks!