Update expectations for video-rotation.html to Failure Pass [chromium/src : master]
CJ DiMeglio (Gerrit)
This change is ready for review.
Patch set 4:Commit-Queue +1
To view, visit change 917200. To unsubscribe, or for help writing mail filters, visit settings.
Mounir Lamouri (Gerrit)
Patch set 4:Commit-Queue +1
1 comment:
File third_party/WebKit/LayoutTests/TestExpectations:
Patch Set #4, Line 3259:
crbug.com/811605 media/video-poster-after-loadedmetadata.html [ Failure Pass ]Is this change intentional? (adding video-poster-after-loaded-metadata.html)
To view, visit change 917200. To unsubscribe, or for help writing mail filters, visit settings.
CJ DiMeglio (Gerrit)
Patch set 5:Commit-Queue +1
1 comment:
File third_party/WebKit/LayoutTests/TestExpectations:
Patch Set #4, Line 3259:
crbug.com/417782 [ Linux ] virtual/android/fullscreen/video-scrolled-iframe.html [ Failure ]Is this change intentional? (adding video-poster-after-loaded-metadata. […]
It showed up during merger, but now its gone. Recent pull should now reflect the current & correct state of the world.
To view, visit change 917200. To unsubscribe, or for help writing mail filters, visit settings.
Mounir Lamouri (Gerrit)
Patch set 5:Code-Review +1Commit-Queue +2
Commit Bot (Gerrit)
Try jobs failed on following builders:
win7_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win7_chromium_rel_ng/builds/103718)
CJ DiMeglio (Gerrit)
Patch set 6:Commit-Queue +2
Commit Bot (Gerrit)
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Merge branch 'master' into passfail" https://chromium-review.googlesource.com/c/917200/6
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/917200/6
Bot data: {"action": "start", "triggered_at": "2018-02-27T00:32:37.0Z", "cq_cfg_revision": "b4e53669e7c77d75afecebd9b6f6c5a434d389f6", "revision": "a005d7772aa3f105c865800bd8597e795ea7fdbb"}
Commit Bot (Gerrit)
Try jobs failed on following builders:
linux_chromium_rel_ng on luci.chromium.try (JOB_FAILED, https://ci.chromium.org/p/chromium/builders/luci.chromium.try/linux_chromium_rel_ng/34578)
CJ DiMeglio (Gerrit)
So this is showing a failure for video_rotation. Isn't Failure Pass supposed to suppress that?
Frank Liberato (Gerrit)
Patch Set 6:
So this is showing a failure for video_rotation. Isn't Failure Pass supposed to suppress that?
the failure is a timeout, which probably isn't covered by a Failure expectation. is it supposed to time out? i thought that it would fail outright when the codec wasn't available.
thanks
-fl
Mounir Lamouri (Gerrit)
I think the timeout makes sense as the test isn't calling `testRunner.notifyDone()`. Though, in general, it's not super clear to me what the test is testing. It seems that we would want a pixel comparison test here. You wouldn't need `waitUntilDone()`. You can find more information about these tests here: https://chromium.googlesource.com/chromium/src/+/master/docs/testing/writing_layout_tests.md#Pixel-Tests
Also, you might want to simply set the `src=` in the HTML code instead of setting them in on load.
Mounir Lamouri (Gerrit)
Patch set 6:-Code-Review
CJ DiMeglio (Gerrit)
Patch Set 6:
I think the timeout makes sense as the test isn't calling `testRunner.notifyDone()`. Though, in general, it's not super clear to me what the test is testing. It seems that we would want a pixel comparison test here. You wouldn't need `waitUntilDone()`. You can find more information about these tests here: https://chromium.googlesource.com/chromium/src/+/master/docs/testing/writing_layout_tests.md#Pixel-Tests
Also, you might want to simply set the `src=` in the HTML code instead of setting them in on load.
Done
Mounir Lamouri (Gerrit)
Patch Set 7:
Patch Set 6:
I think the timeout makes sense as the test isn't calling `testRunner.notifyDone()`. Though, in general, it's not super clear to me what the test is testing. It seems that we would want a pixel comparison test here. You wouldn't need `waitUntilDone()`. You can find more information about these tests here: https://chromium.googlesource.com/chromium/src/+/master/docs/testing/writing_layout_tests.md#Pixel-Tests
Also, you might want to simply set the `src=` in the HTML code instead of setting them in on load.
Done
This is missing the expectations file. Did you forgot to `git add` it? If you look at https://test-results.appspot.com/data/layout_results/linux_chromium_rel_ng/36715/layout-test-results/results.html you will see the error. Note that I think we should probably upload a png expectation instead of a txt one as it will be easier for everyone to understand what's up.
CJ DiMeglio (Gerrit)
Patch Set 7:
Patch Set 7:
Patch Set 6:
I think the timeout makes sense as the test isn't calling `testRunner.notifyDone()`. Though, in general, it's not super clear to me what the test is testing. It seems that we would want a pixel comparison test here. You wouldn't need `waitUntilDone()`. You can find more information about these tests here: https://chromium.googlesource.com/chromium/src/+/master/docs/testing/writing_layout_tests.md#Pixel-Tests
Also, you might want to simply set the `src=` in the HTML code instead of setting them in on load.
Done
This is missing the expectations file. Did you forgot to `git add` it? If you look at https://test-results.appspot.com/data/layout_results/linux_chromium_rel_ng/36715/layout-test-results/results.html you will see the error. Note that I think we should probably upload a png expectation instead of a txt one as it will be easier for everyone to understand what's up.
Just to make sure we are the same page, the expectations file shouldnt have changed. But either way, would adding a Needs-Rebase to the TestExpecations file be sufficient?
Mounir Lamouri (Gerrit)
Patch Set 7:
Patch Set 7:
Patch Set 7:
Patch Set 6:
I think the timeout makes sense as the test isn't calling `testRunner.notifyDone()`. Though, in general, it's not super clear to me what the test is testing. It seems that we would want a pixel comparison test here. You wouldn't need `waitUntilDone()`. You can find more information about these tests here: https://chromium.googlesource.com/chromium/src/+/master/docs/testing/writing_layout_tests.md#Pixel-Tests
Also, you might want to simply set the `src=` in the HTML code instead of setting them in on load.
Done
This is missing the expectations file. Did you forgot to `git add` it? If you look at https://test-results.appspot.com/data/layout_results/linux_chromium_rel_ng/36715/layout-test-results/results.html you will see the error. Note that I think we should probably upload a png expectation instead of a txt one as it will be easier for everyone to understand what's up.
Just to make sure we are the same page, the expectations file shouldnt have changed. But either way, would adding a Needs-Rebase to the TestExpecations file be sufficient?
Yes, the expectation file shouldn't have changed but there do not seem to have been added. Is it possible that you have it locally but never committed it? You are probably right for the automatic rebase but it might be better to do this locally as it will allow to compare the result with what we actually expect (avoid adding a test testing a broken behaviour). You should be able to create a new expectation file with `--reset-results` when you run the test.
CJ DiMeglio (Gerrit)
Patch Set 7:
Patch Set 7:
Patch Set 7:
Patch Set 7:
Patch Set 6:
I think the timeout makes sense as the test isn't calling `testRunner.notifyDone()`. Though, in general, it's not super clear to me what the test is testing. It seems that we would want a pixel comparison test here. You wouldn't need `waitUntilDone()`. You can find more information about these tests here: https://chromium.googlesource.com/chromium/src/+/master/docs/testing/writing_layout_tests.md#Pixel-Tests
Also, you might want to simply set the `src=` in the HTML code instead of setting them in on load.
Done
This is missing the expectations file. Did you forgot to `git add` it? If you look at https://test-results.appspot.com/data/layout_results/linux_chromium_rel_ng/36715/layout-test-results/results.html you will see the error. Note that I think we should probably upload a png expectation instead of a txt one as it will be easier for everyone to understand what's up.
Just to make sure we are the same page, the expectations file shouldnt have changed. But either way, would adding a Needs-Rebase to the TestExpecations file be sufficient?
Yes, the expectation file shouldn't have changed but there do not seem to have been added. Is it possible that you have it locally but never committed it? You are probably right for the automatic rebase but it might be better to do this locally as it will allow to compare the result with what we actually expect (avoid adding a test testing a broken behaviour). You should be able to create a new expectation file with `--reset-results` when you run the test.
Ah I didn't know about --reset-results. Thanks!
Unfortunately, I hit a bug in Gerrit that makes it so that i cannot upload merged branches (rebase only). I had to move my changes into another CL (958262). I will send that out shortly.
CJ DiMeglio (Gerrit)
This is the exact same CL as https://chromium-review.googlesource.com/c/chromium/src/+/917200
I just hit an unfortunate Gerrit bug that prevented me from uploading any merged changes. And since up until that moment I was syncing by using git pull, I had to start fresh. Sorry for the confusion!
Patch set 1:Commit-Queue +1
To view, visit change 958262. To unsubscribe, or for help writing mail filters, visit settings.
CJ DiMeglio (Gerrit)
CJ DiMeglio abandoned this change.
To view, visit change 917200. To unsubscribe, or for help writing mail filters, visit settings.
Mounir Lamouri (Gerrit)
Patch Set 1: Commit-Queue+1
This is the exact same CL as https://chromium-review.googlesource.com/c/chromium/src/+/917200
I just hit an unfortunate Gerrit bug that prevented me from uploading any merged changes. And since up until that moment I was syncing by using git pull, I had to start fresh. Sorry for the confusion!
Looks good, thanks! :) Sounds like the Mac test is crashing though :(
See the link to the results: https://test-results.appspot.com/data/layout_results/mac_chromium_rel_ng/669340/layout-test-results/results.html
To view, visit change 958262. To unsubscribe, or for help writing mail filters, visit settings.
CJ DiMeglio (Gerrit)
Patch Set 2:
Patch Set 1: Commit-Queue+1
This is the exact same CL as https://chromium-review.googlesource.com/c/chromium/src/+/917200
I just hit an unfortunate Gerrit bug that prevented me from uploading any merged changes. And since up until that moment I was syncing by using git pull, I had to start fresh. Sorry for the confusion!Looks good, thanks! :) Sounds like the Mac test is crashing though :(
See the link to the results: https://test-results.appspot.com/data/layout_results/mac_chromium_rel_ng/669340/layout-test-results/results.html
I am not too sure why the gpu would be crashing like this with this change. Really puzzling.
Mounir Lamouri (Gerrit)
Mounir Lamouri would like ccameron to review this change.
Update expectations for video-rotation.html to Failure Pass
Because propriety codecs are not supported on some test
configurations, video-rotation.html is updated to be
Failure Pass instead of just Skipped, as it gives us better
code coverage.
Bug:750313
Change-Id: Ia26c4cc0fdab21adb308b228a95c234577060af8
---
M third_party/WebKit/LayoutTests/TestExpectations
A third_party/WebKit/LayoutTests/media/video-rotation-expected.png
A third_party/WebKit/LayoutTests/media/video-rotation-expected.txt
M third_party/WebKit/LayoutTests/media/video-rotation.html
4 files changed, 28 insertions(+), 18 deletions(-)
Mounir Lamouri (Gerrit)
Patch Set 4:
Patch Set 2:
Patch Set 1: Commit-Queue+1
This is the exact same CL as https://chromium-review.googlesource.com/c/chromium/src/+/917200
I just hit an unfortunate Gerrit bug that prevented me from uploading any merged changes. And since up until that moment I was syncing by using git pull, I had to start fresh. Sorry for the confusion!Looks good, thanks! :) Sounds like the Mac test is crashing though :(
See the link to the results: https://test-results.appspot.com/data/layout_results/mac_chromium_rel_ng/669340/layout-test-results/results.html
I am not too sure why the gpu would be crashing like this with this change. Really puzzling.
To give more context, it is a DCHECK:
```
STDERR: [38027:775:0313/202035.329555:FATAL:gl_image_io_surface.mm(320)] Check failed: yuv_to_rgb_converter.
```
Sounds like the converter is null and I'm not entirely certain how it can happen.
+ccameron@ who may have some insights here.
ccameron (Gerrit)
GLContextCGL should be creating our yuv-rgb converter there (it shouldn't be able to fail). I don't think it's possible to use a GLImageIOSurface with any other kind of GL context... so that's odd.
Given that this is forward progress, it lgtm. Still very strange.
CJ DiMeglio (Gerrit)
Patch Set 4:
GLContextCGL should be creating our yuv-rgb converter there (it shouldn't be able to fail). I don't think it's possible to use a GLImageIOSurface with any other kind of GL context... so that's odd.
Given that this is forward progress, it lgtm. Still very strange.
Is there already a pre-existing bug I could point to, or shall I make one? My plan is to label this as Crash in TestExpectation explicitly for Mac.
Mounir Lamouri (Gerrit)
Patch Set 4:
Patch Set 4:
GLContextCGL should be creating our yuv-rgb converter there (it shouldn't be able to fail). I don't think it's possible to use a GLImageIOSurface with any other kind of GL context... so that's odd.
Given that this is forward progress, it lgtm. Still very strange.
Is there already a pre-existing bug I could point to, or shall I make one? My plan is to label this as Crash in TestExpectation explicitly for Mac.
FWIW, I did not find one so I would recommend opening one and assigning it to ccameron@.
Wez (Gerrit)
1 comment:
File third_party/WebKit/LayoutTests/TestExpectations:
Patch Set #4, Line 3333:
crbug.com/794338 media/video-rotation.html [ Failure Pass ]Looks like we need an expectation like:
crbug.com/794338 [ Mac ] media/video-rotation.html [ Crash ]
Does the test also crash on non_mac, or actually pass or fail?
To view, visit change 958262. To unsubscribe, or for help writing mail filters, visit settings.
Mounir Lamouri (Gerrit)
lgtm
Patch set 5:Code-Review +1Commit-Queue +1
CJ DiMeglio (Gerrit)
Patch set 5:Commit-Queue +2
Commit Bot (Gerrit)
Commit Bot merged this change.
Update expectations for video-rotation.html to Failure Pass
Because propriety codecs are not supported on some test
configurations, video-rotation.html is updated to be
Failure Pass instead of just Skipped, as it gives us better
code coverage.
Bug: 750313
Change-Id: Ia26c4cc0fdab21adb308b228a95c234577060af8
Reviewed-on: https://chromium-review.googlesource.com/958262
Reviewed-by: Mounir Lamouri <mlam...@chromium.org>
Commit-Queue: CJ DiMeglio <lethala...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545859}
---
M third_party/WebKit/LayoutTests/TestExpectations
A third_party/WebKit/LayoutTests/media/video-rotation-expected.png
A third_party/WebKit/LayoutTests/media/video-rotation-expected.txt
M third_party/WebKit/LayoutTests/media/video-rotation.html
4 files changed, 29 insertions(+), 18 deletions(-)
Dominic Battré (Gerrit)
Dominic Battré reverted this change.
To view, visit change 958262. To unsubscribe, or for help writing mail filters, visit settings.