Wpt: assert_throws is too loose, misses browser bugs

[bzbarsky]

I discovered today that some browsers throw a DOMException with name == "TypeError" in some cases when specs say to throw a TypeError.

Unfortunately, wpt's assert_throws(new TypeError(), stuff) doesn't catch this, because it just compares .name values.

I was going to suggest comparing .constructor.name values as well, but then I discovered https://github.com/web-platform-tests/wpt/pull/11643 and the issues mentioned in the review comments there.

It seems like we really want the following primitives for testing throwing behavior:

1. Test that a DOMException is thrown with the given name/code and corresponding code/name. Right now you can sort of get that by passing a string name or integer code as the first arg, instead of an object, though even then assert_throws doesn't check that the result is a DOMException in any meaningful way (e.g. by checking constructor.name or trying to use a DOMException.prototype getter on it or something).

2. Test that an exception is thrown with the given .constructor.name, or whatever other way we can think of to detect a "real TypeError".

3. Test whatever https://github.com/web-platform-tests/wpt/pull/11643 is trying to test.

The current assert_throws does a mix of these, without getting any of them right.

Are there other things one might want to test? Should we even be trying to shoehorn all of these into a single assert_throws?

//cc @zcorpan @saschanaz @gsnedders @jgraham @annevk @domenic

[saschanaz]

assert_throws currently looks too long, how about assert_throws_domexception for DOMException cases?

[bzbarsky]

Sounds pretty reasonable. We'd need to mass-convert existing consumers in some way. Looks like there are at least 1150 callers that pass a literal string as the first arg to assert_throws and some more that pass a string that's not a literal (e.g. dom/classList.html, custom-elements/custom-element-registry/define.html, etc)...

[domenic]

Test that an exception is thrown with the given .constructor.name, or whatever other way we can think of to detect a "real TypeError".

It may be a good idea to test if .constructor is ===, to catch wrong-realm issues. That may be harder to mass-convert though

Here is my ideal world:

• assert_throws(TypeError, stuff): checks ex.constructor === TypeError
• assert_throws_domexception("StringName", stuff): checks ex.constructor.name === "DOMException", and ex.name === "StringName"
  
  
  
  • Get rid of the integer variant; it just leads to confusion as to which one to use
  
  
  • Maybe extend this with an overload that also checks the realm? I dunno.
  
  
• assert_throws_exactly(value, stuff): checks Object.is(ex, value).

Whether anyone wants to invest all the time necessary to fix the thousands of callers... seems unlikely.

Pragmatically speaking, it may be better to do a targeted one-off hack for the new TypeError() vs. new DOMException("TypeError") case.

[bzbarsky]

It may be a good idea to test if .constructor is ===, to catch wrong-realm issues.

I did consider that. If we're willing to make people deal with this in writing tests, seems like a good idea to me.

checks ex.constructor.name === "DOMException", and ex.name === "StringName"

Should check ex.code too.

One plausible path to making this happen is to rename assert_throws to assert_throws_legacy in a single behavior-preserving mass-rename. Then introduce the new APIs. Then we can parallelize the work of eliminating assert_throws_legacy, starting with disallowing it in new tests.

[gsnedders]

If someone wants to write a PR that does that, that LGTM. We should just leave the mass-rename to assert_throws_legacy until the very last moment before landing it.

[bzbarsky]

How do people feel about assert_domexception instead of assert_throws_domexception, to reduce typing and line length?

Or even assert_domexc to preserve the current length, but that's a bit cryptic. ;)

[Ms2ger]

@domenic's three cases sound good to me.

I'll try to resist the urge to bikeshed, but I do have one naming-related point. Should we reintroduce a function with the name assert_throws at all? Now that we have two-way sync into a number of repositories, and hundreds of outstanding PRs, I feel it would be safer if all three new methods also get their own new name.

Okay, resisting didn't work. Maybe: assert_throws_js, assert_throws_dom and assert_throws_{is, value, exactly}?

[bzbarsky]

OK, I'd like to go ahead with this, using @ms2ger's names: assert_throws_js, assert_throws_dom, and assert_throws_exactly. @foolip @jgraham @domenic any objections? I've put off some actual bugfixes for too long due to this being unfixed and it therefore being too hard to test them....