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

[idlharness] Test class string of prototype objects #27239

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ExE-Boss
Copy link
Contributor

@ExE-Boss ExE-Boss commented Jan 19, 2021

This has been specified in whatwg/webidl#357.


See also: #23206

@foolip
Copy link
Member

foolip commented Jan 20, 2021

Will this introduce new failures to lots of tests that previously had no failing subtests?

Copy link
Contributor

@Ms2ger Ms2ger left a comment

Choose a reason for hiding this comment

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

Looks like this breaks some unit tests:

https://community-tc.services.mozilla.com/tasks/V97XKsF0R5uQgpgUykVQ6A/runs/0/logs/https%3A%2F%2Fcommunity-tc.services.mozilla.com%2Fapi%2Fqueue%2Fv1%2Ftask%2FV97XKsF0R5uQgpgUykVQ6A%2Fruns%2F0%2Fartifacts%2Fpublic%252Flogs%252Flive.log

E     -                        {u'message': u'assert_class_string: class string of Window.prototype expected "[object Window]" but got "[object WindowProperties]"',
E     +                        {u'message': None,
E     u'name': u'Window interface: existence and properties of interface prototype object',
E     u'properties': {},
E     -                         u'status_string': u'FAIL'},
E     ?                                             ^ ^^
E     +                         u'status_string': u'PASS'},
E     ?                                             ^ ^^

@Ms2ger
Copy link
Contributor

Ms2ger commented Jan 21, 2021

Will this introduce new failures to lots of tests that previously had no failing subtests?

Good question. Possibly it will cause all idlharness tests to fail in Firefox; I don't know how many are fully green at this point.

@ExE-Boss
Copy link
Contributor Author

ExE-Boss commented Jan 21, 2021

@Ms2ger
Actually, Firefox has had this fixed as of version 78 in bug 1277799.

Also, that got "[object WindowProperties]" for Window.prototype error appears to be an implementation bug, since the class string is specified for the prototype, regardless of whether it’s a global object.

@ExE-Boss ExE-Boss requested a review from Ms2ger January 22, 2021 09:36
@Ms2ger
Copy link
Contributor

Ms2ger commented Jan 22, 2021

The unit tests need fixing. Looks like CI runs them as

+ ./wpt install firefox browser --destination /home/test
+ ./wpt install firefox webdriver --destination /home/test/firefox
+ export PATH=/home/test/firefox:/home/test/bin:/home/test/bin:/home/test/.local/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
+ PATH=/home/test/firefox:/home/test/bin:/home/test/bin:/home/test/.local/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
+ cd /home/test/web-platform-tests/tools/ci/../../resources/test
+ tox -- --binary=/home/test/browsers/nightly/firefox/firefox

@ExE-Boss
Copy link
Contributor Author

I have filed https://bugzilla.mozilla.org/show_bug.cgi?id=1688198 to get this fixed in Firefox.

@foolip
Copy link
Member

foolip commented Jan 28, 2021

@ExE-Boss do you intend to block this on https://bugzilla.mozilla.org/show_bug.cgi?id=1688198?

Digging into the Chrome regression in https://github.com/web-platform-tests/wpt/pull/27239/checks?check_run_id=1784377402 would be good, but it looks like no changes in Safari test results, which is good.

@ExE-Boss ExE-Boss force-pushed the resources/idlharness/test-prototype-class-string branch from 6426438 to c51fd10 Compare January 30, 2021 14:46
@ExE-Boss
Copy link
Contributor Author

ExE-Boss commented Jan 30, 2021

Looks like Chrome has a bug in their implementation of RTCEncodedVideoFrame and RTCEncodedAudioFrame in that they don’t define a %Symbol.toStringTag% property.


I’d also prefer to get this merged sooner rather than later.

@foolip foolip force-pushed the resources/idlharness/test-prototype-class-string branch from c51fd10 to 49b53d3 Compare May 6, 2021 07:24
@foolip
Copy link
Member

foolip commented May 6, 2021

@ExE-Boss there have been a lot of changes around idlharness.js recently, so I rebased this to see what the current state looks like.

@foolip
Copy link
Member

foolip commented May 6, 2021

Safari results look good, the only difference is in a test that I know is flaky:
https://wpt.fyi/results/media-source/idlharness.window.html?diff&filter=ADC&run_id=5742746000162816&run_id=5660754604720128

resources/idlharness.js Outdated Show resolved Hide resolved
resources/idlharness.js Outdated Show resolved Hide resolved
// assert_class_string(this.get_interface_object().prototype, this.get_qualified_name() + "Prototype",
// "class string of " + this.name + ".prototype");
assert_class_string(this.get_interface_object().prototype, this.get_qualified_name(),
"class string of " + this.name + ".prototype");

// String() should end up calling {}.toString if nothing defines a
// stringifier.
if (!this.has_stringifier()) {
Copy link
Member

Choose a reason for hiding this comment

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

If the above check of a Symbol.toStringTag own property is done, then I think this whole bit can just be removed. The presence of a Symbol.toStringTag doesn't depend on whether there's a stringifier, and can be tested even in the presence of a stringifier. We don't need to also test that String() is implemented correctly, at least not for the prototype object.

Copy link
Contributor

Choose a reason for hiding this comment

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

Testing @@toStringTag is great, but I think we should also make sure to test something closer to how it is used in practice, such as String().

@foolip
Copy link
Member

foolip commented May 6, 2021

Looks like Chrome has a bug in their implementation of RTCEncodedVideoFrame and RTCEncodedAudioFrame in that they don’t define a %Symbol.toStringTag% property.

I tested this a bit. Object.getOwnPropertyDescriptor(RTCEncodedVideoFrame.prototype, Symbol.toStringTag) and Object.getOwnPropertyDescriptor(RTCEncodedAudioFrame.prototype, Symbol.toStringTag) both return the expected descriptors in my testing in Chrome Stable and Canary.

Rather, it's calling String(RTCEncodedVideoFrame.prototype) and String(RTCEncodedAudioFrame.prototype) that throw the TypeError, which is because these interfaces do have stringifier in Chromum's IDL, leading to the equivalent of RTCEncodedAudioFrame.prototype.toString.call(RTCEncodedAudioFrame.prototype), which of course will throw like any attempt to invoke a method on something that isn't an instance of the interface it's on.

This is a bug in Chrome, but the way it should be detected is by checking for the absence of "toString" if there's no stringifier.

But if the final shape of this PR still has a test failing for this reason and it's kind of obscure why, that's OK, I'll file a bug about it then.

I’d also prefer to get this merged sooner rather than later.

Sorry 😳

Co-authored-by: Philip Jägenstedt <philip@foolip.org>
@ExE-Boss ExE-Boss requested a review from foolip June 22, 2021 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants