Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8391006:
aura: Set up the mock compositor to get gfx_unittests green on linux. (Closed)
|
Created:
9 years, 1 month ago by sadrul Modified:
9 years, 1 month ago CC:
chromium-reviews, Paweł Hajdan Jr. Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptionaura: Remove failing ScreenTest for aura.
BUG=none
TEST=gfx_unittets on linux_aura should go green
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107592
Patch Set 1 #Patch Set 2 : . #
Total comments: 4
Patch Set 3 : . #
Total comments: 2
Patch Set 4 : . #Patch Set 5 : . #
Total comments: 3
Messages
Total messages: 19 (0 generated)
This seems to fix gfx_unittests for aura: http://build.chromium.org/p/tryserver.chromium/builders/linux_aura/builds/356 I am using defined(USE_AURA) ... would defined(VIEWS_COMPOSITOR) be better?
http://codereview.chromium.org/8391006/diff/2001/ui/gfx/test_suite.cc File ui/gfx/test_suite.cc (right): http://codereview.chromium.org/8391006/diff/2001/ui/gfx/test_suite.cc#newcode65 ui/gfx/test_suite.cc:65: ui::ResourceBundle::CleanupSharedInstance(); think we need a ui::CompositorTestSupport::Terminate() http://codereview.chromium.org/8391006/diff/2001/ui/ui_unittests.gypi File ui/ui_unittests.gypi (right): http://codereview.chromium.org/8391006/diff/2001/ui/ui_unittests.gypi#newcode183 ui/ui_unittests.gypi:183: 'gfx/compositor/compositor.gyp:compositor', It's only necessary to pull in compositor if you're going to run with a real compositor. If it's always mock, you only need the compositor_test_support.
+sky for OWNERS http://codereview.chromium.org/8391006/diff/2001/ui/gfx/test_suite.cc File ui/gfx/test_suite.cc (right): http://codereview.chromium.org/8391006/diff/2001/ui/gfx/test_suite.cc#newcode65 ui/gfx/test_suite.cc:65: ui::ResourceBundle::CleanupSharedInstance(); On 2011/10/25 20:13:53, jonathan.backer wrote: > think we need a ui::CompositorTestSupport::Terminate() Done. http://codereview.chromium.org/8391006/diff/2001/ui/ui_unittests.gypi File ui/ui_unittests.gypi (right): http://codereview.chromium.org/8391006/diff/2001/ui/ui_unittests.gypi#newcode183 ui/ui_unittests.gypi:183: 'gfx/compositor/compositor.gyp:compositor', On 2011/10/25 20:13:53, jonathan.backer wrote: > It's only necessary to pull in compositor if you're going to run with a real > compositor. If it's always mock, you only need the compositor_test_support. That's what I thought too, but without this, I get linking errors: /usr/bin/ld: obj/ui/gfx/compositor/compositor_test_support.a(obj/ui/gfx/compositor/compositor_test_support.layer.o): in function ui::Layer::SetAnimation(ui::Animation*):../../ui/gfx/compositor/layer.cc:134: error: undefined reference to 'ui::LayerAnimationManager::LayerAnimationManager(ui::Layer*)' /usr/bin/ld: obj/ui/gfx/compositor/compositor_test_support.a(obj/ui/gfx/compositor/compositor_test_support.layer.o): in function ui::Layer::SetAnimation(ui::Animation*):../../ui/gfx/compositor/layer.cc:136: error: undefined reference to 'ui::LayerAnimationManager::SetAnimation(ui::Animation*)' / etc,
http://codereview.chromium.org/8391006/diff/4002/ui/gfx/test_suite.cc File ui/gfx/test_suite.cc (right): http://codereview.chromium.org/8391006/diff/4002/ui/gfx/test_suite.cc#newcode18 ui/gfx/test_suite.cc:18: #if defined(USE_AURA) What gfx tests are actually using the compositor?
Keep the dependency. I was just chatting with piman@ and I'm going to to make the test compositor depend on the real compositor target. > That's what I thought too, but without this, I get linking errors: > > /usr/bin/ld: > obj/ui/gfx/compositor/compositor_test_support.a(obj/ui/gfx/compositor/compositor_test_support.layer.o): > in function > ui::Layer::SetAnimation(ui::Animation*):../../ui/gfx/compositor/layer.cc:134: > error: undefined reference to > 'ui::LayerAnimationManager::LayerAnimationManager(ui::Layer*)' > /usr/bin/ld: > obj/ui/gfx/compositor/compositor_test_support.a(obj/ui/gfx/compositor/compositor_test_support.layer.o): > in function > ui::Layer::SetAnimation(ui::Animation*):../../ui/gfx/compositor/layer.cc:136: > error: undefined reference to > 'ui::LayerAnimationManager::SetAnimation(ui::Animation*)' > / > > etc,
http://codereview.chromium.org/8391006/diff/4002/ui/gfx/test_suite.cc File ui/gfx/test_suite.cc (right): http://codereview.chromium.org/8391006/diff/4002/ui/gfx/test_suite.cc#newcode18 ui/gfx/test_suite.cc:18: #if defined(USE_AURA) On 2011/10/25 21:21:57, sky wrote: > What gfx tests are actually using the compositor? ScreenTest (ui/gfx/screen_unittest.cc): http://build.chromium.org/p/chromium.fyi/builders/Linux%20Aura/builds/1321/st...
This means we have a circular dependency (ui depends on aura and aura depends on ui). This test should be moved to aura. -Scott On Tue, Oct 25, 2011 at 2:24 PM, <sadrul@chromium.org> wrote: > > http://codereview.chromium.org/8391006/diff/4002/ui/gfx/test_suite.cc > File ui/gfx/test_suite.cc (right): > > http://codereview.chromium.org/8391006/diff/4002/ui/gfx/test_suite.cc#newcode18 > ui/gfx/test_suite.cc:18: #if defined(USE_AURA) > On 2011/10/25 21:21:57, sky wrote: >> >> What gfx tests are actually using the compositor? > > ScreenTest (ui/gfx/screen_unittest.cc): > http://build.chromium.org/p/chromium.fyi/builders/Linux%20Aura/builds/1321/st... > > http://codereview.chromium.org/8391006/ >
On 2011/10/25 21:48:01, sky wrote: > This means we have a circular dependency (ui depends on aura and aura > depends on ui). This test should be moved to aura. I cannot say I completely understand what you mean: ui_unittests already has a dependency on aura, and this adds a dependency to compositor to ui_unittests. > > -Scott > > On Tue, Oct 25, 2011 at 2:24 PM, <mailto:sadrul@chromium.org> wrote: > > > > http://codereview.chromium.org/8391006/diff/4002/ui/gfx/test_suite.cc > > File ui/gfx/test_suite.cc (right): > > > > > http://codereview.chromium.org/8391006/diff/4002/ui/gfx/test_suite.cc#newcode18 > > ui/gfx/test_suite.cc:18: #if defined(USE_AURA) > > On 2011/10/25 21:21:57, sky wrote: > >> > >> What gfx tests are actually using the compositor? > > > > ScreenTest (ui/gfx/screen_unittest.cc): > > > http://build.chromium.org/p/chromium.fyi/builders/Linux%2520Aura/builds/1321/... > > > > http://codereview.chromium.org/8391006/ > >
On Tue, Oct 25, 2011 at 2:56 PM, <sadrul@chromium.org> wrote: > On 2011/10/25 21:48:01, sky wrote: >> >> This means we have a circular dependency (ui depends on aura and aura >> depends on ui). This test should be moved to aura. > > I cannot say I completely understand what you mean: ui_unittests already has > a > dependency on aura, and this adds a dependency to compositor to > ui_unittests. This dependency was added explicitly for this test. We should remove the dependency and move the test to aura. -Scott > > >> -Scott > >> On Tue, Oct 25, 2011 at 2:24 PM, <mailto:sadrul@chromium.org> wrote: >> > >> > http://codereview.chromium.org/8391006/diff/4002/ui/gfx/test_suite.cc >> > File ui/gfx/test_suite.cc (right): >> > >> > > > http://codereview.chromium.org/8391006/diff/4002/ui/gfx/test_suite.cc#newcode18 >> >> > ui/gfx/test_suite.cc:18: #if defined(USE_AURA) >> > On 2011/10/25 21:21:57, sky wrote: >> >> >> >> What gfx tests are actually using the compositor? >> > >> > ScreenTest (ui/gfx/screen_unittest.cc): >> > > > http://build.chromium.org/p/chromium.fyi/builders/Linux%2520Aura/builds/1321/... >> >> > >> > http://codereview.chromium.org/8391006/ >> > > > > > http://codereview.chromium.org/8391006/ >
On 2011/10/25 21:59:09, sky wrote: > On Tue, Oct 25, 2011 at 2:56 PM, <mailto:sadrul@chromium.org> wrote: > > On 2011/10/25 21:48:01, sky wrote: > >> > >> This means we have a circular dependency (ui depends on aura and aura > >> depends on ui). This test should be moved to aura. > > > > I cannot say I completely understand what you mean: ui_unittests already has > > a > > dependency on aura, and this adds a dependency to compositor to > > ui_unittests. > > This dependency was added explicitly for this test. We should remove > the dependency and move the test to aura. Aaah, I understand now. Will update accordingly. > > -Scott > > > > > > >> -Scott > > > >> On Tue, Oct 25, 2011 at 2:24 PM, <mailto:sadrul@chromium.org> wrote: > >> > > >> > http://codereview.chromium.org/8391006/diff/4002/ui/gfx/test_suite.cc > >> > File ui/gfx/test_suite.cc (right): > >> > > >> > > > > > > http://codereview.chromium.org/8391006/diff/4002/ui/gfx/test_suite.cc#newcode18 > >> > >> > ui/gfx/test_suite.cc:18: #if defined(USE_AURA) > >> > On 2011/10/25 21:21:57, sky wrote: > >> >> > >> >> What gfx tests are actually using the compositor? > >> > > >> > ScreenTest (ui/gfx/screen_unittest.cc): > >> > > > > > > http://build.chromium.org/p/chromium.fyi/builders/Linux%252520Aura/builds/132... > >> > >> > > >> > http://codereview.chromium.org/8391006/ > >> > > > > > > > > > http://codereview.chromium.org/8391006/ > >
+derat I moved the test from ui_ to aura_. But now that I look at the original CL that introduced this test, it feels like this is perhaps not right? Should there be a screen_unittest.cc in ui/gfx/ for non-aura cases, included in gfx_unittests, and a screen_unittest.cc in ui/aura/, included in aura_unittests?
On 2011/10/25 22:35:24, sadrul wrote: > +derat > > I moved the test from ui_ to aura_. But now that I look at the original CL that > introduced this test, it feels like this is perhaps not right? Should there be a > screen_unittest.cc in ui/gfx/ for non-aura cases, included in gfx_unittests, and > a screen_unittest.cc in ui/aura/, included in aura_unittests? The tests are intended to run on all platforms (at least, I was moving code that previously did). That being said, I think it's quite unlikely that these particular tests will ever catch a bug, and I don't like unittests that aren't really unittests, so...
Would you suggest removing the test altogether? or this LGTY?
On 2011/10/26 02:54:01, sadrul wrote: > Would you suggest removing the test altogether? or this LGTY? I'd be okay with either removing it altogether or keeping it for all platforms. The Aura version of Screen is probably the least interesting and least likely to break out of all the platforms. I don't think that it makes sense to make this test be Aura-only.
I have kept the test in its old place, and disabled the test for aura. PTAL.
LGTM. Thanks! http://codereview.chromium.org/8391006/diff/8001/ui/ui_unittests.gypi File ui/ui_unittests.gypi (right): http://codereview.chromium.org/8391006/diff/8001/ui/ui_unittests.gypi#newcode180 ui/ui_unittests.gypi:180: ['use_aura==1', { nit: delete this section
http://codereview.chromium.org/8391006/diff/8001/ui/ui_unittests.gypi File ui/ui_unittests.gypi (right): http://codereview.chromium.org/8391006/diff/8001/ui/ui_unittests.gypi#newcode180 ui/ui_unittests.gypi:180: ['use_aura==1', { On 2011/10/27 16:05:26, Daniel Erat wrote: > nit: delete this section I think we still need to disable the test for aura.
http://codereview.chromium.org/8391006/diff/8001/ui/ui_unittests.gypi File ui/ui_unittests.gypi (right): http://codereview.chromium.org/8391006/diff/8001/ui/ui_unittests.gypi#newcode180 ui/ui_unittests.gypi:180: ['use_aura==1', { On 2011/10/27 16:09:16, sadrul wrote: > On 2011/10/27 16:05:26, Daniel Erat wrote: > > nit: delete this section > > I think we still need to disable the test for aura. Sorry, I'm not sure how I misread this so horrendously. Carry on. :-)
LGTM |
