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

Remove wpt/webgl/idlharness.any.html & idlharness.any.worker.html from Interop list #363

Closed
yiyix opened this issue Jun 19, 2023 · 8 comments · Fixed by web-platform-tests/wpt-metadata#4539
Labels
focus area: Offscreen Canvas test-change-proposal Proposal to add or remove tests for an interop area

Comments

@yiyix
Copy link

yiyix commented Jun 19, 2023

Test List

wpt /webgl/idlharness.any.html
wpt /webgl/idlharness.any.worker.html

Rationale

These two tests are exercising the corner cases of the WebGL specification related to the prototype chain of JavaScript wrappers. The issues they revealed have existed for 10 years and have not disturbed any web developers. Note that no current browser passes these tests. These tests are not included in the WebGL conformance suite at https://github.com/KhronosGroup/WebGL as well.

Rather than churn all existing WebGL implementations, the WebGL working group thinks these tests should be removed from the Interop 2023 suite. WebGL working group also suggested that future WebGL tests be proposed for inclusion in the WebGL conformance suite itself, before being added to the Interop 2023 suite.

@yiyix yiyix added the test-change-proposal Proposal to add or remove tests for an interop area label Jun 19, 2023
@yiyix yiyix changed the title remove wpt/webgl/idlharness.any.html & idlharness.any.worker.html from Interop list Remove wpt/webgl/idlharness.any.html & idlharness.any.worker.html from Interop list Jun 19, 2023
@nairnandu
Copy link
Contributor

@jgraham @zcorpan can you review for Gecko?

@gsnedders @nt1m can you review for WebKit?

@gsnedders
Copy link
Member

Rather than churn all existing WebGL implementations, the WebGL working group thinks these tests should be removed from the Interop 2023 suite.

Does this imply that you think no implementation should move to match what is currently spec'd, because that would be additional churn? If so, does the WG believe it should change the spec to match existing implementations?

WebGL working group also suggested that future WebGL tests be proposed for inclusion in the WebGL conformance suite itself, before being added to the Interop 2023 suite.

There's relatively few tests in WPT for WebGL; from memory @Ms2ger had added them there at a point when submitting things to the Khronos test suite was relatively burdensome. If someone wants to move those to the WebGL test suite, I doubt there would be complaints?

The idlharness tests probably make sense to keep in WPT, given we have all the machinery to both auto-update IDL fragments and idlharness.js itself lives in WPT?

@Ms2ger
Copy link

Ms2ger commented Jun 22, 2023

It seems like a rather unfortunate situation for the working group to request by back channels that implementers ignore the specification it publishes. I have no opinion on the inclusion in the interop list, but tests matching the actual specification should definitely remain in wpt.

(At least some of the failures seem to flag that the IDL in the specification is invalid, so that needs fixing anyway.)

@kkinnunen-apple
Copy link

kkinnunen-apple commented Jun 26, 2023

What are the concrete problems?
With a quick glance, I can see:

  1. WebGLObject is not in the global object
    Filed: WebGLObject is specified in IDL but none of the implementations implement it KhronosGroup/WebGL#3542

  2. Overload methods are reported as duplicate members:

Is it a WebGL spec problem or IDL test harness problem?

There are methods with same name, same amount of parameters, but different parameter types. Is this intentionally disallowed by some rule?

Only thing I could find:

Note: The identifier can be the same as that of another operation on the interface, however. This is how operation overloading is specified.

IdlArray.prototype.are_duplicate_members = function(m1, m2) {
    if (m1.name !== m2.name) {
        return false;
    }
    if (m1.type === 'operation' && m2.type === 'operation'
        && m1.arguments.length !== m2.arguments.length) {
        // Method overload. TODO: Deep comparison of arguments.
        return false;
    }
    return true;
}

@zcorpan
Copy link
Member

zcorpan commented Jun 26, 2023

Excluding these from Interop 2023 is OK for Mozilla.

@yiyix
Copy link
Author

yiyix commented Jun 27, 2023

Does this imply that you think no implementation should move to match what is currently spec'd, because that would be additional churn? If so, does the WG believe it should change the spec to match existing implementations?

I think the spec needs to be updated to match the real implementation

There's relatively few tests in WPT for WebGL; from memory @Ms2ger had added them there at a point when submitting things to the Khronos test suite was relatively burdensome. If someone wants to move those to the WebGL test suite, I doubt there would be complaints?

Adding @kenrussell to give more insights on this

The idlharness tests probably make sense to keep in WPT, given we have all the machinery to both auto-update IDL fragments and idlharness.js itself lives in WPT?

I also believe those tests should continue live in WPT, they are catching the errors as they are supposed to. But I don't think they need to be in the interop 2023 test list as they haven't disturbed any developers in last 10 years.

@kenrussell
Copy link
Member

It would be fine to move the few wpt tests for WebGL into the conformance suite at https://github.com/KhronosGroup/WebGL .

The WebGL working group would like to align all implementations with the specification, just without the time pressure of doing this in Interop 2023. Per @kkinnunen-apple 's comment above and looking at https://webidl.spec.whatwg.org/#idl-overloading , it looks like the Web IDL test harness's definition of illegal overloads is not correct, and that it's flagging some legal overloads in the WebGL specification. This needs to be studied in more depth and ironed out.

@gsnedders
Copy link
Member

Does this imply that you think no implementation should move to match what is currently spec'd, because that would be additional churn? If so, does the WG believe it should change the spec to match existing implementations?

I think the spec needs to be updated to match the real implementation

FWIW, given this came up in the meeting: from a WebKit point of view we are fine with dropping these tests from Interop 2023, our primary concern was that the WG is actually tracking what appears to be interoperably failing—rather than just filing an issue here saying we shouldn't score that.

Given @kkinnunen-apple filed KhronosGroup/WebGL#3542, we're fine with removing this; I just wanted to make sure the WG was actually tracking something for the failure on their side.

nairnandu added a commit to web-platform-tests/wpt-metadata that referenced this issue Jul 18, 2023
github-actions bot pushed a commit to web-platform-tests/wpt-metadata that referenced this issue Jul 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
focus area: Offscreen Canvas test-change-proposal Proposal to add or remove tests for an interop area
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants