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

Jump to bottom

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

Merged
merged 3 commits into from Mar 28, 2018
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
35ef11d
Special-case idlharness.js errors as an assert_throw option
Mar 23, 2018
5276c19
Changes as per review
Mar 28, 2018
408c5e4
Changes as per review
Mar 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
52 changes: 37 additions & 15 deletions resources/idlharness.js
View file
Open in desktop
Expand Up @@ -123,6 +123,28 @@ var fround =
})();
//@}

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope. Just copy-pasta-ing

Copy link
Member

Choose a reason for hiding this comment

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

Filed #10212 to maybe figure out what they are.

{
/**
* Message to be printed as the error's toString invocation.
*/
this.message = message;
};

IdlHarnessError.prototype = Object.create(Error.prototype);

//@}
IdlHarnessError.prototype.toString = function()
//@{
{
return this.message;
};

//@}

/// IdlArray ///
// Entry point
self.IdlArray = function()
Expand Down Expand Up @@ -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.");
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}

function should_skip(name)
Expand Down Expand Up @@ -280,7 +302,7 @@ IdlArray.prototype.internal_add_idls = function(parsed_idls, options)
}
if (parsed_idl.name in this.members)
{
throw "Duplicate identifier " + parsed_idl.name;
throw new IdlHarnessError("Duplicate identifier " + parsed_idl.name);
}
switch(parsed_idl.type)
{
Expand Down Expand Up @@ -374,7 +396,7 @@ IdlArray.prototype.recursively_get_implements = function(interface_name)
ret = ret.concat(this.recursively_get_implements(ret[i]));
if (ret.indexOf(ret[i]) != ret.lastIndexOf(ret[i]))
{
throw "Circular implements statements involving " + ret[i];
throw new IdlHarnessError("Circular implements statements involving " + ret[i]);
}
}
return ret;
Expand Down Expand Up @@ -404,7 +426,7 @@ IdlArray.prototype.recursively_get_includes = function(interface_name)
ret = ret.concat(this.recursively_get_includes(ret[i]));
if (ret.indexOf(ret[i]) != ret.lastIndexOf(ret[i]))
{
throw "Circular includes statements involving " + ret[i];
throw new IdlHarnessError("Circular includes statements involving " + ret[i]);
}
}
return ret;
Expand Down Expand Up @@ -533,7 +555,7 @@ IdlArray.prototype.is_json_type = function(type)
function exposure_set(object, default_set) {
var exposed = object.extAttrs.filter(function(a) { return a.name == "Exposed" });
if (exposed.length > 1 || exposed.length < 0) {
throw "Unexpected Exposed extended attributes on " + memberName + ": " + exposed;
throw new IdlHarnessError("Unexpected Exposed extended attributes on " + memberName + ": " + exposed);
}

if (exposed.length === 0) {
Expand Down Expand Up @@ -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");
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Accidental.

}

//@}
Expand All @@ -583,7 +605,7 @@ IdlArray.prototype.test = function()
if (!(parsed_idl.name in this.members)
|| !(this.members[parsed_idl.name] instanceof IdlInterface))
{
throw "Partial interface " + parsed_idl.name + " with no original interface";
throw new IdlHarnessError("Partial interface " + parsed_idl.name + " with no original interface");
}
if (parsed_idl.extAttrs)
{
Expand Down Expand Up @@ -848,7 +870,7 @@ IdlArray.prototype.assert_type_is = function(value, type)

if (!(type in this.members))
{
throw "Unrecognized type " + type;
throw new IdlHarnessError("Unrecognized type " + type);
}

if (this.members[type] instanceof IdlInterface)
Expand Down Expand Up @@ -876,7 +898,7 @@ IdlArray.prototype.assert_type_is = function(value, type)
}
else
{
throw "Type " + type + " isn't an interface or dictionary";
throw new IdlHarnessError("Type " + type + " isn't an interface or dictionary");
}
};
//@}
Expand Down Expand Up @@ -1345,13 +1367,13 @@ IdlInterface.prototype.test_self = function()
{
var aliasAttrs = this.extAttrs.filter(function(o) { return o.name === "LegacyWindowAlias"; });
if (aliasAttrs.length > 1) {
throw "Invalid IDL: multiple LegacyWindowAlias extended attributes on " + this.name;
throw new IdlHarnessError("Invalid IDL: multiple LegacyWindowAlias extended attributes on " + this.name);
}
if (this.is_callback()) {
throw "Invalid IDL: LegacyWindowAlias extended attribute on non-interface " + this.name;
throw new IdlHarnessError("Invalid IDL: LegacyWindowAlias extended attribute on non-interface " + this.name);
}
if (this.exposureSet.indexOf("Window") === -1) {
throw "Invalid IDL: LegacyWindowAlias extended attribute on " + this.name + " which is not exposed in Window";
throw new IdlHarnessError("Invalid IDL: LegacyWindowAlias extended attribute on " + this.name + " which is not exposed in Window");
}
// TODO: when testing of [NoInterfaceObject] interfaces is supported,
// check that it's not specified together with LegacyWindowAlias.
Expand All @@ -1360,7 +1382,7 @@ IdlInterface.prototype.test_self = function()

var rhs = aliasAttrs[0].rhs;
if (!rhs) {
throw "Invalid IDL: LegacyWindowAlias extended attribute on " + this.name + " without identifier";
throw new IdlHarnessError("Invalid IDL: LegacyWindowAlias extended attribute on " + this.name + " without identifier");
}
var aliases;
if (rhs.type === "identifier-list") {
Expand Down Expand Up @@ -1733,7 +1755,7 @@ IdlInterface.prototype.test_member_const = function(member)
//@{
{
if (!this.has_constants()) {
throw "Internal error: test_member_const called without any constants";
throw new IdlHarnessError("Internal error: test_member_const called without any constants");
}

test(function()
Expand Down Expand Up @@ -2289,7 +2311,7 @@ IdlInterface.prototype.test_object = function(desc)
{
if (!(current_interface.name in this.array.members))
{
throw "Interface " + current_interface.name + " not found (inherited by " + this.name + ")";
throw new IdlHarnessError("Interface " + current_interface.name + " not found (inherited by " + this.name + ")");
}
if (current_interface.prevent_multiple_testing && current_interface.already_tested)
{
Expand Down
25 changes: 25 additions & 0 deletions resources/test/tests/idlharness/basic.html
View file
Open in desktop
Expand Up @@ -29,6 +29,31 @@
test(function() {
assert_equals(typeof WebIDL2.parse("interface Foo {};"), "object");
}, 'WebIDL2 parse method should produce an AST for correct WebIDL');
test(function() {
assert_throws(new IdlHarnessError("Partial interface A with no original interface"), () => {
let i = new IdlArray();
i.add_untested_idls('partial interface A {};');
i.test();
});
}, 'assert_throws should handle IdlHarnessError');
test(function() {
assert_throws("Partial interface A with no original interface", () => {
let i = new IdlArray();
i.add_untested_idls('partial interface A {};');
i.test();
});
}, 'assert_throws should handle IdlHarnessError from message');
test(function () {
try {
assert_throws("Partial interface A with no original interface", () => {
let i = new IdlArray();
i.add_untested_idls('interface A {};');
i.test();
});
} catch (e) {
assert_true(e instanceof AssertionError);
}
}, 'assert_throws should throw if no IdlHarnessError thrown');
</script>
</body>
</html>
Expand Down
2 changes: 1 addition & 1 deletion resources/test/tox.ini
View file
Open in desktop
Expand Up @@ -13,4 +13,4 @@ deps =
selenium
requests

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

Choose a reason for hiding this comment

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

Debugging accidentally left in place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

20 changes: 20 additions & 0 deletions resources/testharness.js
View file
Open in desktop
Expand Up @@ -1247,6 +1247,13 @@ policies and contribution forms [3].
}
expose(assert_readonly, "assert_readonly");

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

*
* @param {number|string} code The expected exception code.
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Going to leave it alone for this PR.

* @param {Function} func Function which should throw.
* @param {string} description Error description for the case that the error is not thrown.
*/
function assert_throws(code, func, description)
{
try {
Expand All @@ -1258,6 +1265,19 @@ policies and contribution forms [3].
throw e;
}

if ((typeof IdlHarnessError !== 'undefined')
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, there were a handful, like this one:
https://chromium.googlesource.com/chromium/src/+/15e299fd19da9cf6f74dbcb78b6bc4865f7a5a35/third_party/WebKit/LayoutTests/external/wpt/mediasession/setactionhandler.html

Seems manageable if we want to do it that way, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

&& (e instanceof IdlHarnessError)) {
// Assertions for behaviour of the idlharness.js engine.
if (code instanceof IdlHarnessError) {
code = code.message;
}
assert(e.message == code,
'assert_throws', description,
'${func} threw ${e} with type ${type}, not IdlHarnessError',
{func:func, e:e, type:typeof e});
return;
}

assert(typeof e === "object",
"assert_throws", description,
"${func} threw ${e} with type ${type}, not an object",
Expand Down