web-platform-tests / results-collection

Other
41 stars 46 forks source link

Turn Server-Timing experimental feature on #621

Open cvazac opened 6 years ago

cvazac commented 6 years ago

I'm attempting to improve the server-timing results on Safari.

image

I will remove this if/when Safari enables this by default.

cvazac commented 6 years ago

@jugglinmike can you PTAL?

jugglinmike commented 6 years ago

Thanks for the patch, @cvazac!

In order to help others reproduce the results they find on wpt.fyi, we should document this step more widely. Can you patch WPT's instructions on running the tests in Safari (as published at https://web-platform-tests.org/running-tests/safari.html)?

As for the change itself, this may not the best place to insert the new setting. The name "safari-disable-popup-blocker.sh` doesn't describe a change like this. Also, note that this script only runs for the stable release of Safari. Should this configuration also apply to Safari Technology Preview?

I can help work this in to another script, though it'll be a few days before I have the time.

cvazac commented 6 years ago

Thx for the review @jugglinmike!

I made a new file for the experimental flag code - but I couldn't figure out how to test it locally.

Yes, this should run in TP as well.

PR for safari.md documentation change.

jugglinmike commented 6 years ago

Yes, this should run in TP as well.

Excuse me, I forgot that we have a precedent for this kind of thing.

We only apply Chrome's --enable-experimental-web-platform-features when testing the browser's experimental release. That makes the results for the "stable" release representative of a typical user's experience using the stock browser, and it also gives implementers visibility into the cutting edge. We should apply the same policy to Safari and Server Timing.

In addition to updating the script itself, there's another small change we'll need to make to support STP. We never know when the next release will be published, so in order to always test the latest version, we install the browser every time we attempt to collect results. In order for the new script to modify the freshly-installed browser, we'll have to invoke it after that "build step" in the Buildbot configuration file.

jugglinmike commented 6 years ago

Also cc @foolip, who has been automating Safari in another environment.

foolip commented 6 years ago

It would be great to get this into tools/wpt/run.py: https://github.com/web-platform-tests/wpt/blob/bfc7ce508b754e84e95830c2186f1fa8eb9f179e/tools/wpt/run.py#L378-L385

See Chrome.setup_kwargs and Firefox.setup_kwargs in that file for precedent. If we put it into wpt itself, it'll also work for the Safari TP runs we'll be doing on PRs. cc @lukebjerring

jugglinmike commented 6 years ago

This isn't directly analogous to the settings for Chrome and Firefox because it involves system-wide configuration (not a command-line argument). Contributors might be surprised to find their browser behaving differently after invoking wpt run. Maybe that could be alright if we prompted the user for permission or if we attempted to clean up the setting. A separate concern is how these changes are applied. If the browser is running, we have to terminate it before the changes take effect. Doing that in wpt run could also be disruptive for a contributor working from their development environment.

Discussion along these lines should probably take place in an issue filed against the WPT CLI. We've already asked @cvazac to do a fair amount of leg work (and they've been very accommodating!), so I've written something up at https://github.com/web-platform-tests/wpt/issues/13976. In the mean time, I'd be happy to accept a patch for this project--we can remove it when/if it becomes superfluous.

youennf commented 6 years ago

There are other flags that we might want to turn on as well (IntersectionObserver, MediaRecorder, WebRTC MDNS ICE candidates...) on a case by case basis since not all features will be stable enough to survive WPT testing.

I am not sure we should turn these features on within stable Safari. It makes more sense for STP.

cvazac commented 6 years ago

Should safari-disable-popup-blocker.sh be moved after install-browser.sh as well?

foolip commented 6 years ago

@cvazac @youennf are setting shared by Safari stable and TP, or are there settings that are only used by STP? Something that would make this easier is if it were possible to pass these settings to Safari as command line arguments to safaridriver. Do you think that'd make sense? cc @burg

burg commented 6 years ago

They have separate settings. Safari is not big on adding command line arguments if possible. Would your use case be satisfied if experimental features could be toggled by WebDriver capabilities?

Sent from my Apple IIc

On Nov 10, 2018, at 08:08, Philip Jägenstedt notifications@github.com wrote:

@cvazac @youennf are setting shared by Safari stable and TP, or are there settings that are only used by STP? Something that would make this easier is if it were possible to pass these settings to Safari as command line arguments to safaridriver. Do you think that'd make sense? cc @burg

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

foolip commented 6 years ago

Yes, that would be great I think, if they then have no effect beyond the WebDriver session.

cvazac commented 5 years ago

Picking back up on this... if I'm re-reading this correctly, we want to turn on some features for TP only. Has anything upstream made this easier - or should I make those changes from this PR?

cc @jugglinmike @foolip @burg

foolip commented 5 years ago

@cvazac the code can live in wpt no problem, only issue is that it changes global state for a browser the user might also be using for browsing. Just putting it behind a flag that's enabled when running in CI would work for now.

foolip commented 5 years ago

ping @cvazac, are you still interested in this?

cvazac commented 5 years ago

@foolip Yes, I would love to clean up the Safari results. Unfortunately, the path to get there (^^^) is not at all clear to me. :(

foolip commented 5 years ago

@cvazac is putting the behavior behind a flag that we only use in CI an OK path? Are there other blockers too?

foolip commented 5 years ago

I've sent https://github.com/web-platform-tests/wpt/pull/16177 to do this for the Azure Pipelines setup which is now providing most of the results seen on wpt.fyi.