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.js] Expand test coverage #6262

Merged
merged 2 commits into from Jun 30, 2017

Conversation

jugglinmike
Copy link
Contributor

@jugglinmike jugglinmike commented Jun 16, 2017

A previous patch introduced new tests intended to verify that global
platform objects were also immutable prototype objects. In actuality,
the new tests verified an unrelated condition: that interface prototype
objects were also immutable prototype objects.

Update idlharness to assert that both types of objects are immutable
prototype objects, and modify test expectation strings to correctly
describe each type. Introduce "cleanup" logic to avoid sub-test
interaction.


This is a correction/extension of #5788


This change is Reviewable

A previous patch introduced new tests intended to verify that global
platform objects were also immutable prototype objects. In actuality,
the new tests verified an unrelated condition: that interface prototype
objects were also immutable prototype objects.

Update idlharness to assert that both types of objects are immutable
prototype objects, and modify test expectation strings to correctly
describe each type. Introduce "cleanup" logic to avoid sub-test
interaction.
@w3c-bots
Copy link

w3c-bots commented Jun 16, 2017

View the complete job log.

Firefox (nightly)

Testing web-platform-tests at revision ec2b544
Ignoring 2 changed files:
No files changed

@w3c-bots
Copy link

w3c-bots commented Jun 16, 2017

View the complete job log.

Sauce (safari)

Testing web-platform-tests at revision ec2b544
Ignoring 2 changed files:
No files changed

@w3c-bots
Copy link

w3c-bots commented Jun 16, 2017

View the complete job log.

Chrome (unstable)

Testing web-platform-tests at revision ec2b544
Ignoring 2 changed files:
No files changed

@w3c-bots
Copy link

w3c-bots commented Jun 16, 2017

View the complete job log.

Sauce (MicrosoftEdge)

Testing web-platform-tests at revision ec2b544
Ignoring 2 changed files:
No files changed

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

So this appears to now test

  • window's [[Prototype]] cannot change (will always be Window.prototype)
  • Window.prototype's [[Prototype]] cannot change (will always be window's named properties object)

or in the service worker case:

  • self's [[Prototype]] cannot change (will always be ServiceWorkerGlobalScope.prototype)
  • ServiceWorkerGlobalScope.prototype's [[Prototype]] cannot change (will always be WorkerGlobalScope.prototype)

But it is not testing the rest of the objects in the prototype chain, which are tested by https://codereview.chromium.org/2933913002 for the service worker case:

  • Window case
    • Named properties object's [[Prototype]] cannot change (will always be EventTarget.prototype)
    • EventTarget.prototype's [[Prototype]] cannot change (will always be Object.prototype)
    • Object.prototype's [[Prototype]] cannot change (will always be null)
  • Service worker case
    • WorkerGlobalScope.prototype's [[Prototype]] cannot change (will always be EventTarget.prototype)
    • EventTarget.prototype's [[Prototype]] cannot change (will always be Object.prototype)
    • Object.prototype's [[Prototype]] cannot change (will always be null)

I think this would be better if it crawled the prototype chain, like https://codereview.chromium.org/2933913002 does.

But overall the refactoring LGTM, just not sure it accomplishes everything you're hoping for.

IdlInterface.prototype.test_immutable_prototype = function(type, obj)
//@{
{
if (typeof Object.setPrototypeOf !== "function") {
Copy link
Contributor

@tobie tobie Jun 16, 2017

Choose a reason for hiding this comment

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

So I know this isn't introduced by this PR, but the refactoring makes it stand out. This creates bad incentives (i.e. you're better off not implementing Object.setPrototypeOf than implementing and not some of what follows).

Can we have a strategy to avoid this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is related to your question in gh-6246. As I mentioned in IRC, I'm not in a position to make a judgement call here, but I agree that we could use some clarity. I've opened gh-6266 to facilitate a discussion along these lines. Hopefully that results in a formally-documented policy.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, sort of. The broader question here is how do we handle tests we're unable to run because of lack of support for X.

I'm of the opinion we should throw in such cases, not silently avoid running a bunch of tests.

@jugglinmike
Copy link
Contributor Author

@domenic Thanks! You're right that this patch is in service of that Chromium test migration. The prototype traversal something that I don't quite like about that test. It makes the test more of an integration test for multiple web platform features. With the proper support in idlharness.js, the test could be re-factored as a series of independent tests--one for each platform object/interface prototype object pairing. If we had coverage in that way, then verification of the complete chain would be provided on a distributed basis by the current behavior of idlharness.js, and WPT's coverage would be equivalent to that Chromium test. Factored in that way, a Service Worker test wouldn't fail for a bug in EventTarget, but the bug would still be detected.

@domenic
Copy link
Member

domenic commented Jun 16, 2017

Hmm, I'd prefer we keep at least some integration test of the prototype migration, as it tests the actual intent. If you want to add more tests, that's reasonable, but let's keep the existing one.

@jugglinmike
Copy link
Contributor Author

@domenic I'll see what I can do in the Chromium patch, but in the mean time, would you mind merging this?

@domenic
Copy link
Member

domenic commented Jun 30, 2017

Sure. In the future if I give the green checkmark feel free to merge yourself; no need to block on me finding free time.

@domenic domenic merged commit 13b0e2d into web-platform-tests:master Jun 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants