Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2775633006:
Add unittests for gl_surface_egl to cover the changes in crrev.com/2698573002. (Closed)
DescriptionAdd unittests for gl_surface_egl to cover the changes in crrev.com/2698573002.
Review-Url: https://codereview.chromium.org/2775633006
Cr-Commit-Position: refs/heads/master@{#460891}
Committed: https://chromium.googlesource.com/chromium/src/+/9defbfb506f965590425064b6c38a072b96f6470
Patch Set 1 #
Total comments: 6
Patch Set 2 : Address John's comments #
Total comments: 4
Patch Set 3 : Address John's comments #Patch Set 4 : Fix test failure on linux_chromium_chromeos_ozone_rel_ng #Messages
Total messages: 25 (16 generated)
leilei@chromium.org changed reviewers: + bajones@chromium.org, jbauman@chromium.org, klausw@chromium.org
I think this should be in gl_surface_egl_unittest.cc, and we should try to only run the test if we're on a platform that supports EGL. https://codereview.chromium.org/2775633006/diff/1/ui/gl/gl_surface_unittest.cc File ui/gl/gl_surface_unittest.cc (right): https://codereview.chromium.org/2775633006/diff/1/ui/gl/gl_surface_unittest.c... ui/gl/gl_surface_unittest.cc:16: TEST_F(GLSurfaceTest, SurfaceFormatTest) { Use TEST instead of TEST_F. https://codereview.chromium.org/2775633006/diff/1/ui/gl/gl_surface_unittest.c... ui/gl/gl_surface_unittest.cc:17: gl::GLSurfaceTestSupport::InitializeOneOffImplementation( Don't use gl:: in the gl namespace. https://codereview.chromium.org/2775633006/diff/1/ui/gl/gl_surface_unittest.c... ui/gl/gl_surface_unittest.cc:32: EXPECT_EQ(24, attrib); I think these should be EXPECT_LE, as it's possible there aren't configs that have exactly this number of bits.
The CQ bit was checked by jbauman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Rename gl_surface_unittest.cc to gl_surface_egl_unittest.cc and only include it if egl is used. PTAL. https://codereview.chromium.org/2775633006/diff/1/ui/gl/gl_surface_unittest.cc File ui/gl/gl_surface_unittest.cc (right): https://codereview.chromium.org/2775633006/diff/1/ui/gl/gl_surface_unittest.c... ui/gl/gl_surface_unittest.cc:16: TEST_F(GLSurfaceTest, SurfaceFormatTest) { On 2017/03/24 23:48:48, jbauman wrote: > Use TEST instead of TEST_F. Done. https://codereview.chromium.org/2775633006/diff/1/ui/gl/gl_surface_unittest.c... ui/gl/gl_surface_unittest.cc:17: gl::GLSurfaceTestSupport::InitializeOneOffImplementation( On 2017/03/24 23:48:48, jbauman wrote: > Don't use gl:: in the gl namespace. Done. https://codereview.chromium.org/2775633006/diff/1/ui/gl/gl_surface_unittest.c... ui/gl/gl_surface_unittest.cc:32: EXPECT_EQ(24, attrib); On 2017/03/24 23:48:48, jbauman wrote: > I think these should be EXPECT_LE, as it's possible there aren't configs that > have exactly this number of bits. Done.
https://codereview.chromium.org/2775633006/diff/20001/ui/gl/gl_surface_egl_un... File ui/gl/gl_surface_egl_unittest.cc (right): https://codereview.chromium.org/2775633006/diff/20001/ui/gl/gl_surface_egl_un... ui/gl/gl_surface_egl_unittest.cc:16: TEST_F(GLSurfaceEGLTest, SurfaceFormatTest) { Still use TEST() here instead. https://codereview.chromium.org/2775633006/diff/20001/ui/gl/gl_surface_egl_un... ui/gl/gl_surface_egl_unittest.cc:34: EXPECT_EQ(8, attrib); Change this to EXPECT_LE.
https://codereview.chromium.org/2775633006/diff/20001/ui/gl/gl_surface_egl_un... File ui/gl/gl_surface_egl_unittest.cc (right): https://codereview.chromium.org/2775633006/diff/20001/ui/gl/gl_surface_egl_un... ui/gl/gl_surface_egl_unittest.cc:16: TEST_F(GLSurfaceEGLTest, SurfaceFormatTest) { On 2017/03/28 00:46:33, jbauman wrote: > Still use TEST() here instead. Ooops, done. https://codereview.chromium.org/2775633006/diff/20001/ui/gl/gl_surface_egl_un... ui/gl/gl_surface_egl_unittest.cc:34: EXPECT_EQ(8, attrib); On 2017/03/28 00:46:34, jbauman wrote: > Change this to EXPECT_LE. Done.
The CQ bit was checked by jbauman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by leilei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
All CQs passed. PTAL.
lgtm
The CQ bit was checked by leilei@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490908992805090,
"parent_rev": "89874314ccd024e23616e88f48b26d09359110f0", "commit_rev":
"9defbfb506f965590425064b6c38a072b96f6470"}
Message was sent while issue was closed.
Description was changed from ========== Add unittests for gl_surface_egl to cover the changes in crrev.com/2698573002. ========== to ========== Add unittests for gl_surface_egl to cover the changes in crrev.com/2698573002. Review-Url: https://codereview.chromium.org/2775633006 Cr-Commit-Position: refs/heads/master@{#460891} Committed: https://chromium.googlesource.com/chromium/src/+/9defbfb506f965590425064b6c38... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/9defbfb506f965590425064b6c38... |
