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.

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

Special-case idlharness.js errors as an assert_throw option #10164

Merged

Conversation

Projects
None yet
4 participants
@lukebjerring
Copy link
Contributor

lukebjerring commented Mar 23, 2018

Fixes #10162

This adds support for more succinctly asserting behaviour of idlharness.js itself, using the same mechanism in testharness.js, e.g. when testing expected errors in IdlArray.test().


This change is Reviewable

@wpt-pr-bot wpt-pr-bot added the infra label Mar 23, 2018

@wpt-pr-bot wpt-pr-bot requested review from ayg, gsnedders, jgraham and tobie Mar 23, 2018

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Mar 23, 2018

Build PASSED

Started: 2018-03-28 17:48:04
Finished: 2018-03-28 17:55:38

View more information about this build on:

@lukebjerring lukebjerring force-pushed the lukebjerring:idlharness-errors branch from 90a43e4 to 35ef11d Mar 23, 2018

/// IdlHarnessError ///
// Entry point
self.IdlHarnessError = function(message)
//@{

This comment has been minimized.

Copy link
@foolip

foolip Mar 27, 2018

Contributor

Have you been able to figure out what these annotations are and what bad things happen if we get them wrong? I've copypasted them around before without knowing :)

This comment has been minimized.

Copy link
@lukebjerring

lukebjerring Mar 28, 2018

Author Contributor

Nope. Just copy-pasta-ing

This comment has been minimized.

Copy link
@foolip

foolip Mar 28, 2018

Contributor

Filed #10212 to maybe figure out what they are.

@@ -567,7 +589,7 @@ function exposed_in(globals) {
return globals.indexOf("Worker") >= 0 ||
globals.indexOf("ServiceWorker") >= 0;
}
throw "Unexpected global object";
// throw new IdlHarnessError("Unexpected global object");

This comment has been minimized.

Copy link
@foolip

foolip Mar 27, 2018

Contributor

Is this dead code, or why is it commented out?

This comment has been minimized.

Copy link
@lukebjerring

lukebjerring Mar 28, 2018

Author Contributor

Accidental.

@@ -13,4 +13,4 @@ deps =
selenium
requests

commands = pytest {posargs} -vv tests
commands = pytest {posargs} -vv tests/api-tests-1.html

This comment has been minimized.

Copy link
@foolip

foolip Mar 27, 2018

Contributor

Debugging accidentally left in place?

This comment has been minimized.

Copy link
@lukebjerring

lukebjerring Mar 28, 2018

Author Contributor

Yes, the use of {posargs} preceding means there's no simple way to run only a specific test. Reverted.

@@ -1247,6 +1247,13 @@ policies and contribution forms [3].
}
expose(assert_readonly, "assert_readonly");

/**
* Assert a DOMException with the expected code is thrown.

This comment has been minimized.

Copy link
@foolip

foolip Mar 27, 2018

Contributor

s/DOMException/exception/, as this method can be used for both DOMException and https://tc39.github.io/ecma262/#sec-native-error-types-used-in-this-standard or indeed any object that looks a bit like either of those.

This comment has been minimized.

Copy link
@lukebjerring

lukebjerring Mar 28, 2018

Author Contributor

Done.

/**
* Assert a DOMException with the expected code is thrown.
*
* @param {number|string} code The expected exception code.

This comment has been minimized.

Copy link
@foolip

foolip Mar 27, 2018

Contributor

The argument name is pretty bad, but this is currently {object|string|number}, checked in that order. Renaming it to obj_or_code would be an improvement IMHO.

This comment has been minimized.

Copy link
@lukebjerring

lukebjerring Mar 28, 2018

Author Contributor

Going to leave it alone for this PR.

@@ -1258,6 +1265,19 @@ policies and contribution forms [3].
throw e;
}

if ((typeof IdlHarnessError !== 'undefined')

This comment has been minimized.

Copy link
@foolip

foolip Mar 27, 2018

Contributor

The way that assert_throws is used for the per-spec tests themselves is to just check that the right kind of exception is thrown, as well as it can be checked (see "but we can't" below) and to never assert anything about the exception message itself, since the messages aren't defined in web specs and are UA-defined.

For idlharness.js it is kinda useful to check that the right exception message has been thrown, but I wonder if it isn't better to use a custom assertion for those tests?

An alternative could be to allow messages to be asserted more generically, adding it to the if (typeof code === "object") branch below, that if code.message is a non-empty string, we assert that the throw exception matches. That would remove the need to have a special IdlHarnessError.

WDYT?

This comment has been minimized.

Copy link
@foolip

foolip Mar 27, 2018

Contributor

I see there's also an AssertionError in testharness.js that we could perhaps use. It's immediately rethrown by assert_throws. It really depend on what problem #10162 started out with, if it's just about fixing the inconsistency or if there's also some test that needs to be fixed.

This comment has been minimized.

Copy link
@foolip

foolip Mar 27, 2018

Contributor

To see if that idea is at all viable I sent https://chromium-review.googlesource.com/c/chromium/src/+/980757 to see if there are lots of existing tests that pass a message that shouln't be checked.

This comment has been minimized.

Copy link
@foolip

This comment has been minimized.

Copy link
@lukebjerring

lukebjerring Mar 28, 2018

Author Contributor

I agree that it's bad to reference IdlHarnessError from here.

@@ -217,7 +239,7 @@ IdlArray.prototype.internal_add_idls = function(parsed_idls, options)

if (options && options.only && options.except)
{
throw "The only and except options can't be used together."
throw new IdlHarnessError("The only and except options can't be used together.");

This comment has been minimized.

Copy link
@foolip

foolip Mar 27, 2018

Contributor

See comment elsewhere about doing the message comparison more generically. That'd make it possible to just use new Error everywhere in idlharness.js.

This comment has been minimized.

Copy link
@lukebjerring

lukebjerring Mar 28, 2018

Author Contributor

Keeping explicit error type (I see value in it) but made sure not to ref it from testharness.js

@lukebjerring

This comment has been minimized.

Copy link
Contributor Author

lukebjerring commented Mar 28, 2018

PTAL

@foolip

foolip approved these changes Mar 28, 2018

Copy link
Contributor

foolip left a comment

LGTM % a few nits

/// IdlHarnessError ///
// Entry point
self.IdlHarnessError = function(message)
//@{

This comment has been minimized.

Copy link
@foolip

foolip Mar 28, 2018

Contributor

Filed #10212 to maybe figure out what they are.

//@{
{
try {
idlArrayFunc.call(this, this);

This comment has been minimized.

Copy link
@foolip

foolip Mar 28, 2018

Contributor

In the tests, you've always passed an arrow function that takes no arguments (i => { i.test(); }) so neither of the two thises will ever be used here. The would be required if one passes just i.test instead, but what's the second one for?

This comment has been minimized.

Copy link
@lukebjerring

lukebjerring Mar 28, 2018

Author Contributor

True about the first param, but i => { is function(i) { so it's definitely calling with i = IdlArray

}
return;
}
throw new IdlHarnessError(`${idlArrayFunc} did not throw the expected IdlHarnessError`);

This comment has been minimized.

Copy link
@foolip

foolip Mar 28, 2018

Contributor

I think this is unreachable because of the assert(false, ...) above.

This comment has been minimized.

Copy link
@lukebjerring

lukebjerring Mar 28, 2018

Author Contributor

Corrected.

@lukebjerring lukebjerring merged commit 355c2d7 into web-platform-tests:master Mar 28, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@lukebjerring lukebjerring deleted the lukebjerring:idlharness-errors branch Mar 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.