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

Media Capabilities: make framerate a double instead of a DOMString. #18440

Merged
merged 2 commits into from Nov 20, 2019

Conversation

@chromium-wpt-export-bot
Copy link
Collaborator

chromium-wpt-export-bot commented Aug 15, 2019

Spec change: w3c/media-capabilities#128

Bug: 994017
Change-Id: Iab036264fe19a6676c97bb12648321408d91f283
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1755046
Commit-Queue: Mounir Lamouri <mlamouri@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Chrome Cunningham <chcunningham@chromium.org>
Cr-Commit-Position: refs/heads/master@{#716773}

Copy link
Collaborator

wpt-pr-bot left a comment

Already reviewed downstream.

@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-1755046 branch 4 times, most recently from b775c44 to 8dc4438 Nov 18, 2019
Spec change: w3c/media-capabilities#128

Bug: 994017
Change-Id: Iab036264fe19a6676c97bb12648321408d91f283
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1755046
Commit-Queue: Mounir Lamouri <mlamouri@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Chrome Cunningham <chcunningham@chromium.org>
Cr-Commit-Position: refs/heads/master@{#716773}
@stephenmcgruer
Copy link
Contributor

stephenmcgruer commented Nov 20, 2019

Looks like a slow test (on Chrome dev, at least):

 1:07.56 INFO ## Slow tests ##

 1:07.56 INFO |                     Test                    | Result | Longest duration (ms) | Timeout (ms) |
 1:07.56 INFO |---------------------------------------------|--------|-----------------------|--------------|
 1:07.56 INFO | `/media-capabilities/decodingInfo.any.html` | `OK`   | `10109`               | `10000`      |
 1:07.56 INFO 
 1:07.56 INFO ::: Running tests in a loop 10 times : PASS
 1:07.56 INFO ::: Running tests in a loop with restarts 5 times : FAIL
 1:07.56 INFO :::
 1:07.56 ERROR ::: Test verification FAIL

I suspect the test was already slow based on the changes in content, and we had just been scraping the edge for a while. Need to check if its marked slow already - if not, then let's do that. In either case, will warn the author of the CL after we mark the test as slow. @KyleJu do you have bandwidth available to do that?

kyle Ju
@KyleJu
Copy link

KyleJu commented Nov 20, 2019

Looks like a slow test (on Chrome dev, at least):

 1:07.56 INFO ## Slow tests ##

 1:07.56 INFO |                     Test                    | Result | Longest duration (ms) | Timeout (ms) |
 1:07.56 INFO |---------------------------------------------|--------|-----------------------|--------------|
 1:07.56 INFO | `/media-capabilities/decodingInfo.any.html` | `OK`   | `10109`               | `10000`      |
 1:07.56 INFO 
 1:07.56 INFO ::: Running tests in a loop 10 times : PASS
 1:07.56 INFO ::: Running tests in a loop with restarts 5 times : FAIL
 1:07.56 INFO :::
 1:07.56 ERROR ::: Test verification FAIL

I suspect the test was already slow based on the changes in content, and we had just been scraping the edge for a while. Need to check if its marked slow already - if not, then let's do that. In either case, will warn the author of the CL after we mark the test as slow. @KyleJu do you have bandwidth available to do that?

Marked as a slow test and left a comment on the CL

@chromium-wpt-export-bot chromium-wpt-export-bot merged commit 2142380 into master Nov 20, 2019
10 checks passed
10 checks passed
Azure Pipelines #20191120.60 succeeded
Details
Azure Pipelines (./wpt test-jobs) ./wpt test-jobs succeeded
Details
Azure Pipelines (affected tests without changes: Safari Technology Preview) affected tests without changes: Safari Technology Preview succeeded
Details
Azure Pipelines (affected tests: Safari Technology Preview) affected tests: Safari Technology Preview succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests) wpt.fyi hook: safari-preview-affected-tests succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests-without-changes) wpt.fyi hook: safari-preview-affected-tests-without-changes succeeded
Details
Community-TC (pull_request) TaskGroup: success
Details
wpt.fyi - chrome[experimental] Chrome results
Details
wpt.fyi - firefox[experimental] Firefox results
Details
wpt.fyi - safari[experimental] Safari results
Details
@chromium-wpt-export-bot chromium-wpt-export-bot deleted the chromium-export-cl-1755046 branch Nov 20, 2019
@mounirlamouri
Copy link
Contributor

mounirlamouri commented Nov 25, 2019

@KyleJu @stephenmcgruer how does "Running tests in a loop with restarts 5 times" translates in Chrome's web_tests runner? I can't reproduce this and I wonder if I don't use the right configs or if it's because WPT is using //chrome while we do not internally in Blink. @Rob--W @foolip FYI

@chcunningham I wonder if the issue could be the loading of the DB assuming the above is correct and it can't be repro in Blink.

@stephenmcgruer
Copy link
Contributor

stephenmcgruer commented Nov 26, 2019

Hey, sorry for the delay @mounirlamouri . "Running tests in a loop with restarts 5 times" would be approximately equivalent to passing --repeat-each=5 and --restart-shell-between-tests to run_web_tests.py (cc @LukeZielinski). However what we're actually looking for here is that any single run of the test took long enough that it threatened to timeout, so a single run should reproduce it.

Locally running this a few times, I got one result of 9.8 seconds which is close to the timeout, and one result of 10.6 that is over it:

...
Ran 1 tests finished in 9.8 seconds.
$ ./wpt run --no-pause-after-test --binary ~/chromium/src/out/Release/chrome chrome media-capabilities/decodingInfo.any.html
...
Ran 1 tests finished in 10.6 seconds.

Now I'm using a local build so not as fast as a release one (which is what wpt.fyi uses), but still a quite slow test!

Also if you look at https://wpt.fyi/results/media-capabilities/decodingInfo.any.html?label=experimental&label=master&aligned, Chrome took 2.927 seconds to run this test file (on the run I looked at) vs 0.42 seconds for Firefox.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.