Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
709923003:
Use ApplicationTestBase for MojoRendererTest. (Closed)
|
Created:
6 years, 1 month ago by xhwang Modified:
6 years, 1 month ago CC:
chromium-reviews, feature-media-reviews_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), ben+mojo_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionUse ApplicationTestBase for MojoRendererTest.
To run the test:
1, Checkout Mojo, see: https://github.com/domokit/mojo/blob/master/README.md
2, In <mojo_src>, run: ninja -C out/Debug root
3, In <chromium_src>, run: ninja -C out/Debug -j1024 media_renderer_apptest
4, In <mojo_src>, create symbol links:
ln -s <chromium_src>/out/Debug/libmojo_media_renderer_apptest.so out/Debug/
ln -s <chromium_src>/out/Debug/libmojo_media_renderer_app.so out/Debug/
ln -s <chromium_src>/out/Debug/libffmpegsumo.so out/Debug/
5, Run the test:
<mojo_src>/out/Debug/mojo_shell mojo:mojo_media_renderer_apptest
Note that this test doesn't do much now. We need add more test coverage to this
test.
BUG=432289
TEST=Test compiles and passes.
Committed: https://crrev.com/463f1f0eb19bc15b1d4342ef2cea6b00177c942f
Cr-Commit-Position: refs/heads/master@{#303881}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Works with https://codereview.chromium.org/710403002/ #
Total comments: 10
Patch Set 3 : #Patch Set 4 : rebase only #Patch Set 5 : Fix media.gyp #
Messages
Total messages: 25 (4 generated)
xhwang@chromium.org changed reviewers: + msw@chromium.org
msw: I am trying to use ApplicationTestBase for MojoRendererTest. But I get the
following linking errors. As far as I can tell, a lot of dependencies depends on
"//mojo/environment:chromium", but "test_support_standalone" depends on
"//mojo/public/cpp/environment:standalone". Both defines GetDefaultLogger etc.
I don't really understand the relation between //mojo/environment:chromium and
//mojo/public/cpp/environment:standalone (sorry for my ignorance). This BUILD.gn
file looks like a magic to me. I guess it needs some cleanup. Do you have any
suggestion for me? Thanks@
--------------------------------
ninja: Entering directory `out/Debug'
[1/1] Regenerating ninja files
[1/1] SOLINK ./libmojo_media_renderer_apptest.so
FAILED: /hdd2/workspace/goma/gomacc
../../third_party/llvm-build/Release+Asserts/bin/clang++ -shared
-Wl,--fatal-warnings -m
64 -fPIC -Wl,-z,noexecstack -Wl,-z,now -Wl,-z,relro -pthread
-B../../third_party/binutils/Linux_x64/Release/bin -fuse-ld=gol
d -Wl,--icf=safe -o ./libmojo_media_renderer_apptest.so
-Wl,-soname=libmojo_media_renderer_apptest.so @./libmojo_media_rende
rer_apptest.so.rsp && { readelf -d ./libmojo_media_renderer_apptest.so | grep
SONAME ; nm -gD -f p libmojo_media_renderer_ap
ptest.so | cut -f1-2 -d' '; } > ./libmojo_media_renderer_apptest.so.tmp && if !
cmp -s ./libmojo_media_renderer_apptest.so.t
mp ./libmojo_media_renderer_apptest.so.TOC; then mv
./libmojo_media_renderer_apptest.so.tmp ./libmojo_media_renderer_apptest
.so.TOC; fi
../../third_party/binutils/Linux_x64/Release/bin/ld.gold: error:
obj/mojo/public/cpp/environment/lib/standalone.environment.
o: multiple definition of 'mojo::Environment::GetDefaultLogger()'
../../third_party/binutils/Linux_x64/Release/bin/ld.gold:
obj/mojo/environment/mojo_environment_chromium.environment.o: previous
definition here
../../third_party/binutils/Linux_x64/Release/bin/ld.gold: error:
obj/mojo/public/cpp/environment/lib/standalone.environment.o: multiple
definition of 'mojo::Environment::GetDefaultAsyncWaiter()'
../../third_party/binutils/Linux_x64/Release/bin/ld.gold:
obj/mojo/environment/mojo_environment_chromium.environment.o: previous
definition here
../../third_party/binutils/Linux_x64/Release/bin/ld.gold: error:
obj/mojo/public/cpp/environment/lib/standalone.logging.o: multiple definition of
'mojo::internal::LogMessage::LogMessage(char const*, int, int)'
../../third_party/binutils/Linux_x64/Release/bin/ld.gold:
obj/mojo/public/cpp/environment/lib/mojo_environment_chromium.logging.o:
previous definition here
../../third_party/binutils/Linux_x64/Release/bin/ld.gold: error:
obj/mojo/public/cpp/environment/lib/standalone.logging.o: multiple definition of
'mojo::internal::LogMessage::LogMessage(char const*, int, int)'
../../third_party/binutils/Linux_x64/Release/bin/ld.gold:
obj/mojo/public/cpp/environment/lib/mojo_environment_chromium.logging.o:
previous definition here
../../third_party/binutils/Linux_x64/Release/bin/ld.gold: error:
obj/mojo/public/cpp/environment/lib/standalone.logging.o: multiple definition of
'mojo::internal::LogMessage::~LogMessage()'
../../third_party/binutils/Linux_x64/Release/bin/ld.gold:
obj/mojo/public/cpp/environment/lib/mojo_environment_chromium.logging.o:
previous definition here
../../third_party/binutils/Linux_x64/Release/bin/ld.gold: error:
obj/mojo/public/cpp/environment/lib/standalone.logging.o: multiple definition of
'mojo::internal::LogMessage::~LogMessage()'
../../third_party/binutils/Linux_x64/Release/bin/ld.gold:
obj/mojo/public/cpp/environment/lib/mojo_environment_chromium.logging.o:
previous definition here
clang: error: linker command failed with exit code 1 (use -v to see invocation)
ninja: build stopped: subcommand failed.
xhwang@chromium.org changed reviewers: + dalecurtis@chromium.org
+dalecurtis: This is the place we test MojoRendererImpl. Currently it's broken and I am trying to fix it. Once it's fixed, we should be able to add more tests to it. Ideally, we should be able to run RendererImplTest on MojoRendererImpl, but I don't know how easy it is to do so.
Thanks for fixing! https://codereview.chromium.org/709923003/diff/1/media/mojo/services/renderer... File media/mojo/services/renderer_unittest.cc (right): https://codereview.chromium.org/709923003/diff/1/media/mojo/services/renderer... media/mojo/services/renderer_unittest.cc:118: ApplicationTestBase::SetUp(); Why not first?
On 2014/11/10 19:33:16, xhwang wrote: > msw: I am trying to use ApplicationTestBase for MojoRendererTest. But I get the > following linking errors. As far as I can tell, a lot of dependencies depends on > "//mojo/environment:chromium", but "test_support_standalone" depends on > "//mojo/public/cpp/environment:standalone". Both defines GetDefaultLogger etc. > > I don't really understand the relation between //mojo/environment:chromium and > //mojo/public/cpp/environment:standalone (sorry for my ignorance). This BUILD.gn > file looks like a magic to me. I guess it needs some cleanup. Do you have any > suggestion for me? Thanks@ Use these deps in your BUILD.gn instead of the ones you have now: "//mojo/application", "//mojo/application:test_support", The "chromium" environment allows use of //base, the pure-mojo "standalone" environment does not allow use of //base. Since you're using //base in your renderer_apptest target, you'll want to add the "chromium" environment deps there, instead of the "standalone" enviroment deps that you have in Patch Set 1. Sorry it's not obvious, I'll be sharing an application testing doc soon!
On 2014/11/10 21:07:53, msw wrote: > On 2014/11/10 19:33:16, xhwang wrote: > > msw: I am trying to use ApplicationTestBase for MojoRendererTest. But I get > the > > following linking errors. As far as I can tell, a lot of dependencies depends > on > > "//mojo/environment:chromium", but "test_support_standalone" depends on > > "//mojo/public/cpp/environment:standalone". Both defines GetDefaultLogger etc. > > > > I don't really understand the relation between //mojo/environment:chromium and > > //mojo/public/cpp/environment:standalone (sorry for my ignorance). This > BUILD.gn > > file looks like a magic to me. I guess it needs some cleanup. Do you have any > > suggestion for me? Thanks@ > > Use these deps in your BUILD.gn instead of the ones you have now: > "//mojo/application", > "//mojo/application:test_support", > > The "chromium" environment allows use of //base, the pure-mojo "standalone" > environment does not allow use of //base. Since you're using //base in your > renderer_apptest target, you'll want to add the "chromium" environment deps > there, instead of the "standalone" enviroment deps that you have in Patch Set 1. > Sorry it's not obvious, I'll be sharing an application testing doc soon! Thanks for the comments! I don't see test_support under mojo/application... Am I missing something? https://code.google.com/p/chromium/codesearch#chromium/src/mojo/application/&...
On 2014/11/11 04:04:06, xhwang wrote: > On 2014/11/10 21:07:53, msw wrote: > > On 2014/11/10 19:33:16, xhwang wrote: > > > msw: I am trying to use ApplicationTestBase for MojoRendererTest. But I get > > the > > > following linking errors. As far as I can tell, a lot of dependencies > depends > > on > > > "//mojo/environment:chromium", but "test_support_standalone" depends on > > > "//mojo/public/cpp/environment:standalone". Both defines GetDefaultLogger > etc. > > > > > > I don't really understand the relation between //mojo/environment:chromium > and > > > //mojo/public/cpp/environment:standalone (sorry for my ignorance). This > > BUILD.gn > > > file looks like a magic to me. I guess it needs some cleanup. Do you have > any > > > suggestion for me? Thanks@ > > > > Use these deps in your BUILD.gn instead of the ones you have now: > > "//mojo/application", > > "//mojo/application:test_support", > > > > The "chromium" environment allows use of //base, the pure-mojo "standalone" > > environment does not allow use of //base. Since you're using //base in your > > renderer_apptest target, you'll want to add the "chromium" environment deps > > there, instead of the "standalone" enviroment deps that you have in Patch Set > 1. > > Sorry it's not obvious, I'll be sharing an application testing doc soon! > > Thanks for the comments! I don't see test_support under mojo/application... Am I > missing something? > > https://code.google.com/p/chromium/codesearch#chromium/src/mojo/application/&... Perhaps Chrome needs a Mojo roll? But my CL isn't that new... I meant this: https://github.com/domokit/mojo/blob/master/mojo/application/BUILD.gn
On 2014/11/11 04:09:41, msw wrote: > On 2014/11/11 04:04:06, xhwang wrote: > > On 2014/11/10 21:07:53, msw wrote: > > > On 2014/11/10 19:33:16, xhwang wrote: > > > > msw: I am trying to use ApplicationTestBase for MojoRendererTest. But I > get > > > the > > > > following linking errors. As far as I can tell, a lot of dependencies > > depends > > > on > > > > "//mojo/environment:chromium", but "test_support_standalone" depends on > > > > "//mojo/public/cpp/environment:standalone". Both defines GetDefaultLogger > > etc. > > > > > > > > I don't really understand the relation between //mojo/environment:chromium > > and > > > > //mojo/public/cpp/environment:standalone (sorry for my ignorance). This > > > BUILD.gn > > > > file looks like a magic to me. I guess it needs some cleanup. Do you have > > any > > > > suggestion for me? Thanks@ > > > > > > Use these deps in your BUILD.gn instead of the ones you have now: > > > "//mojo/application", > > > "//mojo/application:test_support", > > > > > > The "chromium" environment allows use of //base, the pure-mojo "standalone" > > > environment does not allow use of //base. Since you're using //base in your > > > renderer_apptest target, you'll want to add the "chromium" environment deps > > > there, instead of the "standalone" enviroment deps that you have in Patch > Set > > 1. > > > Sorry it's not obvious, I'll be sharing an application testing doc soon! > > > > Thanks for the comments! I don't see test_support under mojo/application... Am > I > > missing something? > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/mojo/application/&... > > Perhaps Chrome needs a Mojo roll? But my CL isn't that new... I meant this: > https://github.com/domokit/mojo/blob/master/mojo/application/BUILD.gn I just synced and still don't see the changes from upstream. Maybe we are not updating mojo/application anymore? https://github.com/domokit/mojo/wiki/Rolling-code-between-chromium-and-mojo
msw@chromium.org changed reviewers: + jamesr@chromium.org
On 2014/11/11 04:49:48, xhwang wrote: > On 2014/11/11 04:09:41, msw wrote: > > On 2014/11/11 04:04:06, xhwang wrote: > > > On 2014/11/10 21:07:53, msw wrote: > > > > Use these deps in your BUILD.gn instead of the ones you have now: > > > > "//mojo/application", > > > > "//mojo/application:test_support", > > > > > > Thanks for the comments! I don't see test_support under mojo/application... > > > Am I missing something? > > > https://code.google.com/p/chromium/codesearch#chromium/src/mojo/application/&... > > > > Perhaps Chrome needs a Mojo roll? But my CL isn't that new... I meant this: > > https://github.com/domokit/mojo/blob/master/mojo/application/BUILD.gn > > I just synced and still don't see the changes from upstream. Maybe we are not > updating mojo/application anymore? > > https://github.com/domokit/mojo/wiki/Rolling-code-between-chromium-and-mojo Hey James, does Chromium get mojo/application:test_support rolled in?
On 2014/11/11 08:41:20, msw wrote: > Hey James, does Chromium get mojo/application:test_support rolled in? No - mojo and chromium have separate copies of that.
On 2014/11/11 17:38:23, jamesr wrote: > On 2014/11/11 08:41:20, msw wrote: > > Hey James, does Chromium get mojo/application:test_support rolled in? > > No - mojo and chromium have separate copies of that. OK, I'll spin up a CL to land the test_support stuff there today.
On 2014/11/11 17:46:24, msw wrote: > On 2014/11/11 17:38:23, jamesr wrote: > > On 2014/11/11 08:41:20, msw wrote: > > > Hey James, does Chromium get mojo/application:test_support rolled in? > > > > No - mojo and chromium have separate copies of that. > > OK, I'll spin up a CL to land the test_support stuff there today. Great, thanks!
dalecurtis/msw: This CL now works if I patch in https://codereview.chromium.org/710403002/ PTAL https://codereview.chromium.org/709923003/diff/20001/media/media.gyp File media/media.gyp (left): https://codereview.chromium.org/709923003/diff/20001/media/media.gyp#oldcode1127 media/media.gyp:1127: 'target_name': 'mojo_media_renderer_apptest', Currently there's no way to get this test work with gyp. Removing the target for now. If we ever need gyp and it can actually work, we can bring it back.
https://codereview.chromium.org/709923003/diff/20001/media/mojo/services/medi... File media/mojo/services/media_renderer_apptest.cc (right): https://codereview.chromium.org/709923003/diff/20001/media/mojo/services/medi... media/mojo/services/media_renderer_apptest.cc:52: return nullptr; No Video? ~_~ https://codereview.chromium.org/709923003/diff/20001/media/mojo/services/medi... media/mojo/services/media_renderer_apptest.cc:114: void SetUp() override { It's unusual to have both a SetUp and a constructor, why not do this in the constructor?
https://codereview.chromium.org/709923003/diff/20001/media/mojo/services/BUIL... File media/mojo/services/BUILD.gn (right): https://codereview.chromium.org/709923003/diff/20001/media/mojo/services/BUIL... media/mojo/services/BUILD.gn:100: # Note that there's no GYP version for this test because ApplicationTestBase I guess this should be a TODO? I can work on this if it's needed. https://codereview.chromium.org/709923003/diff/20001/media/mojo/services/rend... File media/mojo/services/renderer_unittest.cc (left): https://codereview.chromium.org/709923003/diff/20001/media/mojo/services/rend... media/mojo/services/renderer_unittest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. q: is this file actually deleted in your checkout with this CL? (it shows up as an 'M' on the Rietveld overview, not 'D')
https://codereview.chromium.org/709923003/diff/20001/media/mojo/services/BUIL... File media/mojo/services/BUILD.gn (right): https://codereview.chromium.org/709923003/diff/20001/media/mojo/services/BUIL... media/mojo/services/BUILD.gn:100: # Note that there's no GYP version for this test because ApplicationTestBase On 2014/11/11 23:36:38, msw wrote: > I guess this should be a TODO? I can work on this if it's needed. Added TODO, but not a priority for us. https://codereview.chromium.org/709923003/diff/20001/media/mojo/services/medi... File media/mojo/services/media_renderer_apptest.cc (right): https://codereview.chromium.org/709923003/diff/20001/media/mojo/services/medi... media/mojo/services/media_renderer_apptest.cc:52: return nullptr; On 2014/11/11 23:32:20, DaleCurtis wrote: > No Video? ~_~ Yep.. I felt this code needs some major refactoring before we can add any real test case. So I felt it's okay to leave that to the next CL. https://codereview.chromium.org/709923003/diff/20001/media/mojo/services/medi... media/mojo/services/media_renderer_apptest.cc:114: void SetUp() override { On 2014/11/11 23:32:20, DaleCurtis wrote: > It's unusual to have both a SetUp and a constructor, why not do this in the > constructor? Following this example here: https://github.com/domokit/mojo/blob/master/examples/apptest/example_apptest.cc Actually in chromium I saw a lot of SetUp() use with constructors: https://code.google.com/p/chromium/codesearch#search/&q=SetUp%5C(%5C)%5C%20ov... Am I missing anything? https://codereview.chromium.org/709923003/diff/20001/media/mojo/services/rend... File media/mojo/services/renderer_unittest.cc (left): https://codereview.chromium.org/709923003/diff/20001/media/mojo/services/rend... media/mojo/services/renderer_unittest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2014/11/11 23:36:38, msw wrote: > q: is this file actually deleted in your checkout with this CL? > (it shows up as an 'M' on the Rietveld overview, not 'D') Yes, it's deleted. Maybe it's showing "M" because it's renamed?
lgtm https://codereview.chromium.org/709923003/diff/20001/media/mojo/services/medi... File media/mojo/services/media_renderer_apptest.cc (right): https://codereview.chromium.org/709923003/diff/20001/media/mojo/services/medi... media/mojo/services/media_renderer_apptest.cc:114: void SetUp() override { On 2014/11/12 00:29:08, xhwang wrote: > On 2014/11/11 23:32:20, DaleCurtis wrote: > > It's unusual to have both a SetUp and a constructor, why not do this in the > > constructor? > > Following this example here: > https://github.com/domokit/mojo/blob/master/examples/apptest/example_apptest.cc > > Actually in chromium I saw a lot of SetUp() use with constructors: > https://code.google.com/p/chromium/codesearch#search/&q=SetUp%5C(%5C)%5C%20ov... > > Am I missing anything? Sometimes you need to vend things that shouldn't be done until after construction or SetUp, so it's necessary. I don't know if that's the case here.
rebase only
lgtm
The CQ bit was checked by xhwang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/709923003/60002
Message was sent while issue was closed.
Committed patchset #5 (id:60002)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/463f1f0eb19bc15b1d4342ef2cea6b00177c942f Cr-Commit-Position: refs/heads/master@{#303881} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
