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

HTML: Fix test when run through ./wpt run #12885

Merged
merged 2 commits into from
Sep 6, 2018
Merged

Conversation

TimothyGu
Copy link
Member

When run through ./wpt run, the test window is created by another window through window.open(), so opener is not null in the main page (when we expect it to be null).

@foolip, can you double check this looks correct?

When run through ./wpt run, the test window is created by another window
through window.open(), so opener is not null in the main page (when we
expect it to be null).
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.

Looks good, ./wpt run does open new windows and this fix looks correct.

@TimothyGu TimothyGu merged commit fe34dfb into master Sep 6, 2018
@TimothyGu TimothyGu deleted the document-open-fix-reload branch September 6, 2018 20:53
@domenic
Copy link
Member

domenic commented Sep 6, 2018

@foolip it's worrying to me that test authors have to be aware of this. Is there anything we could do?

@foolip
Copy link
Member

foolip commented Sep 6, 2018

@domenic I think the code in question is here:
https://github.com/web-platform-tests/wpt/blob/master/tools/wptrunner/wptrunner/executors/testharness_webdriver.js#L18

I don't think we could use window.open("%(abs_url)s", "%(window_id)s", "noopener") here, because the two windows communicate to make testdriver.js work:
https://github.com/web-platform-tests/wpt/blob/master/tools/wptrunner/wptrunner/testdriver-extra.js

Not sure if there are other places that depend on it as well. @gsnedders, do you know? Any ideas for how we could make this trap go away?

@gsnedders
Copy link
Member

The fundamental issue is we need some way to return from an execute_async_script call when testharness.js completes. Notably, this provides a callback function you need to call to return.

Currently we do this by creating a listener for message on the parent, then opening the test in a new window, and posting the results from testharnessreport.js to the opener.

One possible option would be to instead save the callback function on the global (on something like __wpt_really_really_dont_touch_me_callback_fn) and then call that from testharnessreport.js directly. Of course, that has other downsides (like navigating the window means you can't return results any more, though that would be pretty weird to start with…).

@foolip
Copy link
Member

foolip commented Sep 7, 2018

@TimothyGu, WDYT, is this a problem that is important to fix?

@jgraham
Copy link
Contributor

jgraham commented Sep 7, 2018

Navigating the window already doesn't work, I think.

Fundamentally I'm not aware of any* way to create a solution that's totally transparent. You either need to play in the global scope of the test, or give the test a window but accept that properties like opener will be non-null. testharness.js is pretty explicitly designed for the latter behaviour (with all the code for communicating results to parent windows/frames).

  • Except something gecko-specific involving Xrays, or restarting the browser after each test.

@domenic
Copy link
Member

domenic commented Sep 7, 2018

Personally I think playing in the global scope of the test is much less dangerous than messing with web platform features that might themselves be under test.

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.

6 participants