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

Test for member name clashes in idlharness #12231

Merged

Conversation

lukebjerring
Copy link
Contributor

#12197 revealed that clashing partial/inherited/included interface member names are totally overridden .

This asserts that each member name is unique when rolling the extra members into the main interface, adding a setup test for each partial/mixin/inheritance.

@lukebjerring
Copy link
Contributor Author

@kenrussell
Copy link
Member

The tryjobs from the Chromium patch are red apparently because of some issues with Document and the createImageBitmap API:

https://chromium-review.googlesource.com/c/chromium/src/+/1154984
https://ci.chromium.org/p/chromium/builders/luci.chromium.try/linux_chromium_rel_ng/152109

https://test-results.appspot.com/data/layout_results/linux_chromium_rel_ng/152109/layout-test-results/results.html

Should those be resolved before proceeding with this patch?

@kenrussell
Copy link
Member

I'll be happy to review this but shouldn't be the assignee.

Are there for example Chromium tryjobs which show that the current patch passes all IDL checks?

There are failing CI checks above; how do reviewers read and understand them?

@lukebjerring
Copy link
Contributor Author

@kenrussell - the wpt-bot picks a random reviewer from the set of reviewers as the assignee; this is intended to ensure that we don't suffer diffusion of responsibility (all reviewers leaving it for someone else). If you're willing to review, then the assignment succeeded!

TEST-FAIL | /infrastructure/testdriver/actions/actionsWithKeyPressed.html | TestDriver actions: actions with key pressed - assert_array_equals: lengths differ, expected 3 got 1
at Test. (http://web-platform.test:8000/infrastructure/testdriver/actions/actionsWithKeyPressed.html:64:34)
at Test.step (http://web-platform.test:8000/resources/testharness.js:1594:25)
at http://web-platform.test:8000/resources/testharness.js:1634:32

The (blocking) failure is not related to this change, so can be ignored with respect to your review.

What you really want to look at is the before/after results on wpt.fyi (there's a link called "View visual comparison" on each of the wpt.fyi checks).

This link shows all the newly failing results on Chrome:
https://wpt.fyi/results/?q=seq%28status%3A%21fail+status%3Afail%29&run_id=247440007&run_id=249160006

Looks like overloaded methods are being incorrectly treated as clashes, so something needs fixing.

resources/idlharness.js Outdated Show resolved Hide resolved
@kenrussell
Copy link
Member

Would it be possible for @foolip to review this? I've never reviewed a patch in this repo and am still lost reading the test results.

@foolip
Copy link
Member

foolip commented Jul 8, 2019

Sure, I can review.

Copy link
Member

@foolip foolip left a comment

Choose a reason for hiding this comment

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

LGTM % nits.

I have to admit I'm not thrilled about growing the number of subtests that are checking that specs are doing the right thing as opposed to implementations, but I think it's a better trade-off than failing the setup which would have been the option. An approach to avoiding this would be to build the tests and have the spec checks be part of the build step, but that'd amount to a full rewrite of the whole idlharness.js machinery.

resources/idlharness.js Outdated Show resolved Hide resolved
resources/idlharness.js Outdated Show resolved Hide resolved
subresource-integrity/idlharness.window.js Show resolved Hide resolved
@@ -7,7 +7,7 @@
'use strict';

idl_test(
['webgl1'],
['webgl1', 'webgl2'],
Copy link
Member

Choose a reason for hiding this comment

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

Yay! :)

@lukebjerring
Copy link
Contributor Author

https://github.com/web-platform-tests/wpt/pull/12231/checks?check_run_id=237131876 produced https://staging.wpt.fyi/results/?diff&filter=ADC&q=seq%28status%3A%21fail%20status%3Afail%29&run_id=253480011&run_id=239540012 which has 6 tests with clashes, which boil down to 3 separate redefinitions.

WebGL with canvas, bufferData, texImage3D (not a typo) all failing.
webxr is same as WebGL
cssom-view MouseEventInit has screenX
dom SVGAElement has href (see w3c/svgwg#312)
svg is same as dom

@lukebjerring
Copy link
Contributor Author

infrastructure/ failures are #19297

@lukebjerring lukebjerring merged commit de4b4df into web-platform-tests:master Sep 26, 2019
Improve idlharness.js automation moved this from In progress to Done Sep 26, 2019
@lukebjerring lukebjerring deleted the idlharness-mixin-clashes branch September 26, 2019 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants