Check inherited interfaces exist #10285
Conversation
|
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? |
|
Doing that - just needed one of the 2 URLs to exist first :) |
Build ERROREDStarted: 2018-04-04 23:08:31 Failing Jobs
View more information about this build on: |
resources/idlharness.js
Outdated
| @@ -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; | |||
There was a problem hiding this comment.
nit: these can be const (not sure what the ambient coding style is, though)
resources/idlharness.js
Outdated
| @@ -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)) { | |||
There was a problem hiding this comment.
const instead of var here?
resources/idlharness.js
Outdated
| 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.`); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Cool, patch LGTM, let's see what the bots say. |
|
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. |
|
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
force-pushed
the
idlharness-base-interfaces
branch
from
33b7160
to
ada9ac6
Compare
|
We're now down to 17 'regressions' which change or add passing subtests. |
|
Build failure is a stability check timeout. |
|
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. |
Chromium equivalent of #10285 Change-Id: I388e2a9605096886291fafdae428b78bfe21dea9
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