Skip to content
This repository has been archived by the owner on Nov 6, 2019. It is now read-only.

Safari Tech Preview test results are different on local machine #619

Open
beaufortfrancois opened this issue Oct 17, 2018 · 25 comments
Open

Comments

@beaufortfrancois
Copy link

beaufortfrancois commented Oct 17, 2018

I'm seeing some timeouts on Picture-in-Picture idl harness tests that I'm not seeing locally with Safari Tech Preview Release 67 (on macOS 10.13.6).

Note that it is the same with Safari stable locally.

Local machine
image

web-platform-tests dashboard
image

@foolip
Copy link
Member

foolip commented Oct 17, 2018

@jugglinmike do you have logs available for these runs, to see if they provide any clues?

@beaufortfrancois
Copy link
Author

beaufortfrancois commented Oct 17, 2018

Note that I actually get timeouts when running from command line (see below)
Same for Safari Stable and Tech Preview.

./wpt run safari picture-in-picture/idlharness.window.html

Running 1 tests in web-platform-tests



  ▶ TIMEOUT [expected OK] /picture-in-picture/idlharness.window.html



  ▶ Unexpected subtest result in /picture-in-picture/idlharness.window.html:

  │ TIMEOUT [expected PASS] idl_test setup

  └   → Test timed out

@beaufortfrancois
Copy link
Author

beaufortfrancois commented Oct 17, 2018

It turns out that if I attach <video> to document.body, test doesn't time out anymore.
Note that test times out because loadedmetadata event is NOT fired when <video> is not attached to document.body.
It seems to be specific to running from command line (using Automation mode).

 function loadVideo(activeDocument, sourceUrl) {
   return new Promise((resolve, reject) => {
     const document = activeDocument || window.document;
     const video = document.createElement('video');
+    document.body.appendChild('video'); // Needed only for Safari Automation mode.
     video.src = sourceUrl || getVideoURI('/media/movie_5');
     video.onloadedmetadata = () => { resolve(video); };
     video.onerror = error => { reject(error); };
   });
 }

https://github.com/web-platform-tests/wpt/blob/2ac8a23714d941c1a544f2b897a3c77f65425ae9/picture-in-picture/resources/picture-in-picture-helpers.js

@gsnedders
Copy link
Member

gsnedders commented Oct 17, 2018

@burg any idea what's up here with Safari behaving differently under automation?

@burg
Copy link

burg commented Oct 18, 2018

I'm not quite sure what "Automation mode" means. Does this run via WebDriver, or not?

@beaufortfrancois
Copy link
Author

Yes @burg

@burg
Copy link

burg commented Oct 18, 2018

What do I run to test this locally from the wpt repository with a custom driver path?

@burg
Copy link

burg commented Oct 18, 2018

Two avenues come to mind:

  • Is the window visible on the screen? If not, this might cause trouble. A video recording from a failing run would help triage this aspect.
  • There may be a misconfiguration of the Automation window. safaridriver may need to set WKWebViewConfiguration._mediaDataLoadsAutomatically = YES to behave the same as Safari, but I can't check this easily.

@gsnedders
Copy link
Member

@burg

./wpt run safari --webdriver-binary /Applications/Safari\ Technology\ Preview.app/Contents/MacOS/safaridriver picture-in-picture/idlharness.window.html

@foolip
Copy link
Member

foolip commented Oct 18, 2018

--channel=preview should also work after web-platform-tests/wpt#13460 too.

@burg
Copy link

burg commented Oct 19, 2018

Great, I'm tracking this internally as rdar://problem/45384220. I'll update this issue when I figure it out.

@beaufortfrancois
Copy link
Author

@burg, here's a 28s screen recording: https://photos.app.goo.gl/qsxSgHijiWEfVW649

@foolip
Copy link
Member

foolip commented Oct 19, 2018

That's cool :)

Note that if you ever want to run a single test and don't want the window to stay open you can use --no-pause.

@beaufortfrancois
Copy link
Author

beaufortfrancois commented Oct 26, 2018

FWIW @jernoble and I noticed that HTMLMediaElement logs were not present in the Console.app to help debugging this.

Moreover, on @padenot's machine, test was actually flaky, not always a timeout.

Update: I was able to get flakyness as well.

@beaufortfrancois
Copy link
Author

@burg Let me know if I can help.

@foolip
Copy link
Member

foolip commented Oct 31, 2018

@beaufortfrancois once web-platform-tests/wpt#13769 has landed, you should be able to submit a PR that tweaks .azure-pipelines.yml a bit to run the test using Safari Technology Preview, for a second data point. If it also is different from a local machine, then at least it's easy (ish) to iterate on the Azure Pipelines setup to work out why it's different.

@beaufortfrancois
Copy link
Author

@burg Any progress on this issue?

@beaufortfrancois
Copy link
Author

I executed ./wpt run --channel=preview safari picture-in-picture/idlharness.window.html again this morning and I'm still seeing the timeout issue.
As I said in #619 (comment), attaching the video to the body fixes the timeout.

@burg Were you able to figure this one out? @jernoble may help as well to diagnose.
@foolip Is there anything else I can do as well to help?

$ ./wpt run --channel=preview safari picture-in-picture/idlharness.window.html 
Running 1 tests in web-platform-tests

  ▶ TIMEOUT [expected OK] /picture-in-picture/idlharness.window.html

  ▶ Unexpected subtest result in /picture-in-picture/idlharness.window.html:
  │ TIMEOUT [expected PASS] idl_test setup
  └   → Test timed out

$ ./wpt run safari picture-in-picture/idlharness.window.html 
safaridriver: unrecognized option `--version'
Running 1 tests in web-platform-tests

  ▶ TIMEOUT [expected OK] /picture-in-picture/idlharness.window.html

  ▶ Unexpected subtest result in /picture-in-picture/idlharness.window.html:
  │ TIMEOUT [expected PASS] idl_test setup
  └   → Test timed out

https://wpt.fyi/results/picture-in-picture/idlharness.window.html?label=master&label=experimental&product=chrome&product=safari

@foolip
Copy link
Member

foolip commented Jan 22, 2019

@beaufortfrancois if you're getting unrecognized option --version'then you're probably not using the most recent Safari Technology Preview. Does/Applications/Safari Technology Preview.app/Contents/MacOS/safaridriver --version` also give you that error? If so can you update it?

The problem, however, seems to be related to picture-in-picture-helpers.js. Can you just attach the video to the document if that fixes the problem? You could file a WebKit bug and link to it in a comment.

@foolip
Copy link
Member

foolip commented Jan 22, 2019

Sorry, there were two ./wpt run invocations in your code block, and only the second one shows the warning. That's as expected until the next Safari stable release.

@beaufortfrancois
Copy link
Author

beaufortfrancois commented Jan 22, 2019

My goal with these tests is to make sure a video doesn't have to be attached to the DOM to be able to enter Picture-in-Picture, so it should fail (because it is not supported yet) but not time out.

To be clear, this issue happens only in SafariDriver, not Safari browser.

@foolip
Copy link
Member

foolip commented Jan 22, 2019

@beaufortfrancois is there anything in https://github.com/WebKit/webkit/blob/master/Source/WebCore/html/HTMLMediaElement.cpp that provides hints? Without reading code, my guess would be that the window isn't considered focused or visible when run under automation, and that there's some code that waits for it to be focused/visible.

@beaufortfrancois
Copy link
Author

It may be mediaDataLoadsAutomatically as suggested earlier by @burg but I'm not sure.

@burg
Copy link

burg commented Jan 22, 2019

I haven't had time to look into this, but it's still being tracked.

@beaufortfrancois
Copy link
Author

@burg Is there any chance you can prioritize this or we can help?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants