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

Check inherited interfaces exist #10285

Merged
merged 7 commits into from Apr 5, 2018

Conversation

Projects
None yet
6 participants
@lukebjerring
Contributor

lukebjerring commented Apr 3, 2018

Reattempt at #10240, accommodating for dictionaries.

Chromium equivalent CL:
https://chromium-review.googlesource.com/#/c/chromium/src/+/992880
Note: Chromium drops the unit tests for idlharness.js, so IMO the CL isn't useful other than to see affected tests.


This change is Reviewable

@lukebjerring lukebjerring requested review from foolip and inexorabletash Apr 3, 2018

@wpt-pr-bot wpt-pr-bot added the infra label Apr 3, 2018

@wpt-pr-bot wpt-pr-bot requested review from ayg, gsnedders, jgraham and tobie Apr 3, 2018

@inexorabletash

This comment has been minimized.

Contributor

inexorabletash commented Apr 3, 2018

What about @foolip's suggestion on the earlier iteration of this to try landing this fix via Chrome's CQ instead, so we can get CI about any failures and update test expectations ahead of time?

@lukebjerring

This comment has been minimized.

Contributor

lukebjerring commented Apr 3, 2018

Doing that - just needed one of the 2 URLs to exist first :)

@w3c-bots

This comment has been minimized.

w3c-bots commented Apr 3, 2018

Build ERRORED

Started: 2018-04-04 23:08:31
Finished: 2018-04-05 00:00:47

Failing Jobs

  • firefox:nightly

View more information about this build on:

@@ -682,6 +682,20 @@ IdlArray.prototype.test = function()
}
this["includes"] = {};
// Assert B defined for A : B
for (var member of Object.values(this.members).filter(m => m.base)) {
let lhs = member.name;

This comment has been minimized.

@inexorabletash

inexorabletash Apr 3, 2018

Contributor

nit: these can be const (not sure what the ambient coding style is, though)

This comment has been minimized.

@lukebjerring

lukebjerring Apr 3, 2018

Contributor

Done.

@@ -682,6 +682,20 @@ IdlArray.prototype.test = function()
}
this["includes"] = {};
// Assert B defined for A : B
for (var member of Object.values(this.members).filter(m => m.base)) {

This comment has been minimized.

@inexorabletash

inexorabletash Apr 3, 2018

Contributor

const instead of var here?

This comment has been minimized.

@lukebjerring

lukebjerring Apr 3, 2018

Contributor

Done.

for (var member of Object.values(this.members).filter(m => m.base)) {
let lhs = member.name;
let rhs = member.base;
if (!(lhs in this.members)) throw new IdlHarnessError(`${lhs} inherits ${rhs}, but ${lhs} is undefined.`);

This comment has been minimized.

@inexorabletash

inexorabletash Apr 3, 2018

Contributor

Does this case (lhs is undefined) ever occur? I'm not familiar enough with the surrounding code to be positive, but it seems weird that it could. If so, add test case for it.

This comment has been minimized.

@lukebjerring

lukebjerring Apr 3, 2018

Contributor

Copy-pasta from code-block above (for which lhs could, in theory, fail to exist). We're iterating values of this.members here though, so lhs in this.members has to always be true. Removed.

@inexorabletash

This comment has been minimized.

Contributor

inexorabletash commented Apr 3, 2018

Cool, patch LGTM, let's see what the bots say.

@foolip

This comment has been minimized.

Contributor

foolip commented Apr 3, 2018

Looks like most of the affected tests are due to base dictionaries not existing. Per #7146 dictionaries aren't really tested in any meaningful way, so an option could be to only do interfaces as a first step. But that'd leave us with a curious inconsistency, so either way is fine with me.

@foolip

foolip approved these changes Apr 3, 2018

Changes LGTM, approving so this can be landed when the affected tests are updated.

@lukebjerring

This comment has been minimized.

Contributor

lukebjerring commented Apr 4, 2018

Note that 'fixing' the affected tests may have the result of changing some failing subtests to passing. I copied all the changed files across to the chromium CL, so we can see the effect in the CI, but we'll submit the fixes here so that expected results are automatically updated by the exporter.

@lukebjerring

This comment has been minimized.

Contributor

lukebjerring commented Apr 5, 2018

@lukebjerring

This comment has been minimized.

Contributor

lukebjerring commented Apr 5, 2018

Build failure is a stability check timeout.

@Hexcles

This comment has been minimized.

Member

Hexcles commented Apr 5, 2018

Since idlharness.js is modified, all IDL tests are affected. Stability checks are likely to timeout in such case (they need to run the tests 10 times).

Interestingly though, Chrome stability check didn't time out and the results were stable. This, along with the Chromium CI results, should give us enough confidence. @foolip may admin-merge this PR once he sees fit.

@foolip foolip merged commit 4d1c192 into web-platform-tests:master Apr 5, 2018

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details

@lukebjerring lukebjerring deleted the lukebjerring:idlharness-base-interfaces branch Apr 5, 2018

chromium-wpt-export-bot added a commit that referenced this pull request Apr 9, 2018

Assert extended interfaces exist
Chromium equivalent of
#10285

Change-Id: I388e2a9605096886291fafdae428b78bfe21dea9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment