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

[wptrunner] Transmit test results via postMessage #13881

Closed

Conversation

jugglinmike
Copy link
Contributor

A recent improvement to wptrunner modified the way data structures are
transmitted from the browser to the WebDriver server [1]. One
consequence of this change is that the data structures are created in a
JavaScript realm which differs from the realm in which the "Execute
Script" WebDriver command is running. As of version 17, Microsoft Edge
does not correctly serialize cross-realm values, so referenced
improvement unintentionally precluded testing in that browser.

Transmit data structures between realms via the postMessage API to
avoid this problem.

[1] #13419


This is the reason we've recently been reporting such laughably incomplete results for Edge on wpt.fyi. I've also submitted WebDriver spec tests for the expected serialization behavior

A recent improvement to wptrunner modified the way data structures are
transmitted from the browser to the WebDriver server [1]. One
consequence of this change is that the data structures are created in a
JavaScript realm which differs from the realm in which the "Execute
Script" WebDriver command is running. As of version 17, Microsoft Edge
does not correctly serialize cross-realm values, so referenced
improvement unintentionally precluded testing in that browser.

Transmit data structures between realms via the `postMessage` API to
avoid this problem.

[1] web-platform-tests#13419
@wpt-pr-bot wpt-pr-bot added infra wptrunner The automated test runner, commonly called through ./wpt run labels Nov 2, 2018
@jugglinmike
Copy link
Contributor Author

@foolip
Copy link
Member

foolip commented Nov 2, 2018

@jugglinmike, infrastructure/ tests are failing on Travis, can you see about fixing those?

@jugglinmike
Copy link
Contributor Author

Looks like I was a little too aggressive in trying to remove global state. I think I've resolved this by caching the origin in the realm of the message recipient. This is a crucial section of code, though, so vetting it through all of WPT seems prudent:

https://tools.taskcluster.net/groups/Mmj2u8piTuyoDlvdad2gGg

This branch is currently based on ffeba23, so we should compare with the Taskcluster results for that commit:

https://tools.taskcluster.net/groups/dFx-WCDHRzSMGzj4wuxF6Q

@foolip
Copy link
Member

foolip commented Nov 3, 2018

@lukebjerring @Hexcles how can we get https://tools.taskcluster.net/groups/Mmj2u8piTuyoDlvdad2gGg into wpt.fyi for a comparison?

This is a perfect case for #13263 :)

@lukebjerring
Copy link
Contributor

The Taskcluster webhook takes all the task groups and turns them into artifact URLs, and then posts to the results receiver with a full list of all of those URLs.. might be worth extracting that code, and exposing it into another api endpoint which is Taskcluster specific.

However, as a one off, you could reproduce the same list of urls and send the POST event yourself?

@lukebjerring
Copy link
Contributor

So, with a little scrape of the UI:

Array.from(document.querySelectorAll('a')).map(a => /groups\/[^\/]+\/tasks\/([^\/]+)\//.exec(a.href)).filter(m => !!m).map(m => m[1]).filter(m => m !== "null")

Looks like the task ID is in the URL you gave (Mmj2u8piTuyoDlvdad2gGg) and the group IDs are:

["ZVChn1bdRcSX5I56CabVbQ", "ViEYc7BHSVCJi-CXO6cqaw", "XWoTo0t7TZqOWoeCvBcOWg", "IEOm-11CR82h2FwGpmosBg", "Yd_5Hnt-S4qsGeHPJhTBig", "R6GiWrXzTg-kejTnU30zvA", "Fau84efbRaCGblDHiGFHcA", "MwmJtqrKRNS77YBOVd5XGQ", "BAeb_zRBS0aFh0iIIlyCKA", "b4Hp03NYRRSekLSnCYn5qg", "bp0Gqv5WRZG0DBrj63HBhA", "DOPVhU7JQ92aPrcp7ppExw", "NRh6qioERYOqz6bmUG4ItQ", "HYtXlm4hQ6yU_nMvdIWoPQ", "Iea3Cya4RWm29-jrA6e1zQ", "OkivyodfR6K5hOA7k7Vfsw", "GiBI8IU4QsGaTMp0L26_2Q", "ToArMA6BShylgk2RBSj4zA", "QY-4hYBNSfKoQFKj2zCvIw", "J3apkKfVTZCOpf8yqvGICw", "PrNFDkkwTGW7Z8R8q8sQYw", "KmyFIDOmQ_yy5PZqCAOKjw", "dL_1ex-DSzmoGykBrbpVlA", "UVNAkAbMRYiKOdqBQIyaDA", "RQzufNthTeORh7EvgCe76w", "FGaW2YD0RV-VbMibuB2Gbg", "CtzBh4pVTbWQgK1lCW5xDg", "SYEXrRsvRWG-4l2u5hXFuw", "ZIF0d4wuTpe4FKeb_L0jJg", "OdplewErRBePy9Ip16EyNw", "Pth9wUmHSPGQRNJOauGayA", "ZnT3IIhXS4KxYiRTM1ly6w", "APC22dKoTOi6M8RcaOSBFg", "YZZOZ4lCQfyibIYOAbsXsg", "dMVwLtBAR8COF0nKk7wKTA", "NXoFXs8fQRqDinQ7HwO_BA", "NYS5CXqmR5e577VxRC0K2g", "Iq6I4zZCSzm7yhDYVgHGFg", "N2I9IpteRE6WnekVg7TunQ", "a5lDXY5dRcWRwLScsvPz6Q", "Okpv13iQSDyUUAj4vwcRbQ", "M_gtodHiSW-OhMnjqNYMvQ", "OlbqEI_ATSq1cmUgif-QtA", "O0Xj9tcbQoyPxhx3Jmg0pw", "d3l7NyrAQQeeZbwU2a7gXA", "MC0-dASJTNS0csqv5kZ3dQ", "AIGlcx0gSJ-dTsC92liuug", "AmAOmo5rT4qBG2XEpnmEvw", "KG_kQA2HTqmrhsIMGOsbUg", "RXc3as_fTTG4VSKyc7tibg", "LiL-UsQ_TGi_30EImKhgJQ", "dKVNfAaxS9G5_bXfNYz4VQ"]

@lukebjerring
Copy link
Contributor

lukebjerring commented Nov 3, 2018

In theory, the following scrape of URLs, and a hack for /admin/results/upload, might result in the output we want:
https://gist.github.com/lukebjerring/d4dd09930c4959d9c6611241da4dfc8e

I (cheekily) ran it as jugglinmike, and labelled it testdriver-message-passing, so https://wpt.fyi/test-runs?label=jugglinmike&max-count=100 will show if it's successful (and revision was set to ffeba23ebd)

EDIT: Yeah FF worked, so ran chrome too.
https://wpt.fyi/results/?sha=ffeba23ebd&products=firefox[taskcluster],firefox[jugglinmike]&diff
https://wpt.fyi/results/?sha=ffeba23ebd&products=chrome[taskcluster],chrome[jugglinmike]&diff

@foolip
Copy link
Member

foolip commented Nov 3, 2018

Amazing, thanks @lukebjerring!

@jugglinmikes, looks like there are regressions to investigate here.

Copy link
Member

@gsnedders gsnedders left a comment

Choose a reason for hiding this comment

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

@jugglinmike plz look through the regressions @lukebjerring linked

@jugglinmike
Copy link
Contributor Author

I took the weekend off, but this is on the docket for today.

@jgraham
Copy link
Contributor

jgraham commented Nov 5, 2018

FWIW we eventually need to work on getting this to work without window.opener being used at all, either via postMessage or otherwise. That's because there's a feature coming to allow nulling out the opener on window.open and testing that probably requires the harness to run tests in a top level browsing context. I started working on that, but it's a bigger change and has a few tricky parts.

Copy link
Contributor

@jgraham jgraham left a comment

Choose a reason for hiding this comment

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

I think what you're doing here is OK, but it's a larger than necessary change from the current design which makes it harder to reason about the correctness.

if (!event) {

if (event.data.type === "testdriver-resume") {
source = event.source;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really understand the point of this; it seems like the identity of the test window is statically known here; it's window.win. Why do we need to keep updating it?

};

source.postMessage({
type: "testdriver-next message",
Copy link
Contributor

Choose a reason for hiding this comment

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

This type string doesn't match anything else.

window.opener.testdriver_callback = callback;
window.opener.process_next_event();

window.addEventListener("message", function handle(event) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like the pattern where we keep adding and removing message listeners; we've had bags with such code before. Ideally we define the message handler once in testharnessreport.js and just have this script update the current callback and resume the test.

Copy link
Contributor

@jgraham jgraham left a comment

Choose a reason for hiding this comment

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

Wrong button, sorry!

@jugglinmike
Copy link
Contributor Author

On Friday, we learned that in addition to tripping up Edge, gh-13419 interferes with the semantics of some tests (see gh-13876). That prompted @jgraham to begin work on an alternate solution which reduces the number of browsing contexts to one. As James noted above, that approach will also enable contributors to test more parts of the platform.

We discussed simplifying this patch to use as a short-term fix. @jgraham shared some details about what they had in mind, but I'm having trouble interpreting them. What I have locally is no less complicated than this patch, and it's still not quite right, somehow.

I'm time-boxing my effort on this because we expect the change to be short-lived. That's why I'm proposing an even simpler change: gh-13938. That will not fix support for the invalidated tests, but it will correct the regression to Microsoft Edge. That may be preferable to reverting gh-13419 altogether.

@jugglinmike
Copy link
Contributor Author

Superseded by gh-13938.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infra wptrunner The automated test runner, commonly called through ./wpt run
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants