Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10808115:
Run media_unittests on android test bots. (Closed)
|
Created:
8 years, 5 months ago by nilesh Modified:
8 years, 5 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, Xianzhu Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRun media_unittests on android test bots.
BUG=137131
TEST=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=148493
Patch Set 1 #
Total comments: 11
Patch Set 2 : #
Total comments: 2
Patch Set 3 : Override TestSuite::Initialize to do additional initialization. #Patch Set 4 : Remove duplicate code to init logging. #
Total comments: 8
Patch Set 5 : Addressed nits, removed some unused header inclusion. #
Messages
Total messages: 14 (0 generated)
Ami: Please look at media/ Yaron: Everything else.
lgtm https://chromiumcodereview.appspot.com/10808115/diff/1/build/android/gtest_fi... File build/android/gtest_filter/media_unittests_disabled (right): https://chromiumcodereview.appspot.com/10808115/diff/1/build/android/gtest_fi... build/android/gtest_filter/media_unittests_disabled:4: # b/6336635 Switch to crbug.com (obsolte these and file new bugs)
https://chromiumcodereview.appspot.com/10808115/diff/1/build/android/gtest_fi... File build/android/gtest_filter/media_unittests_disabled (right): https://chromiumcodereview.appspot.com/10808115/diff/1/build/android/gtest_fi... build/android/gtest_filter/media_unittests_disabled:2: # This file was automatically generated by build/android/run_tests.py run_tests.py knows how to pick out bug numbers?? https://chromiumcodereview.appspot.com/10808115/diff/1/media/base/run_all_uni... File media/base/run_all_unittests.cc (right): https://chromiumcodereview.appspot.com/10808115/diff/1/media/base/run_all_uni... media/base/run_all_unittests.cc:31: // testing/android/native_test_wrapper.cc before main() is called. Do you mean native_test_launcher.cc? Is this pattern really the right way to go? ISTM you'll end up having to have an equivalent of this code in every unittests target. Why not instead omit the AEM from n_t_l.cc and let the main()'s continue to own their AtExitManagers? https://chromiumcodereview.appspot.com/10808115/diff/1/media/base/run_all_uni... media/base/run_all_unittests.cc:32: // The same thing is also done in base/test/test_suite.cc This line isn't useful to commit IMO.
PTAL. https://chromiumcodereview.appspot.com/10808115/diff/1/build/android/gtest_fi... File build/android/gtest_filter/media_unittests_disabled (right): https://chromiumcodereview.appspot.com/10808115/diff/1/build/android/gtest_fi... build/android/gtest_filter/media_unittests_disabled:2: # This file was automatically generated by build/android/run_tests.py On 2012/07/24 23:37:35, Ami Fischman wrote: > run_tests.py knows how to pick out bug numbers?? Sorry. Removed. https://chromiumcodereview.appspot.com/10808115/diff/1/build/android/gtest_fi... build/android/gtest_filter/media_unittests_disabled:4: # b/6336635 On 2012/07/24 23:29:42, Yaron wrote: > Switch to http://crbug.com (obsolte these and file new bugs) Done. https://chromiumcodereview.appspot.com/10808115/diff/1/media/base/run_all_uni... File media/base/run_all_unittests.cc (right): https://chromiumcodereview.appspot.com/10808115/diff/1/media/base/run_all_uni... media/base/run_all_unittests.cc:31: // testing/android/native_test_wrapper.cc before main() is called. On 2012/07/24 23:37:35, Ami Fischman wrote: > Do you mean native_test_launcher.cc? Yes. Corrected. > Is this pattern really the right way to go? ISTM you'll end up having to have > an equivalent of this code in every unittests target. Why not instead omit the > AEM from n_t_l.cc and let the main()'s continue to own their AtExitManagers? There is code executed in n_t_l.cc:RunTests (before main in called) which needs an AtExitManager. Unfortunately this means that we need this #ifdef in all unittest targets. I don't have a cleaner solution. https://chromiumcodereview.appspot.com/10808115/diff/1/media/base/run_all_uni... media/base/run_all_unittests.cc:32: // The same thing is also done in base/test/test_suite.cc On 2012/07/24 23:37:35, Ami Fischman wrote: > This line isn't useful to commit IMO. Removed.
https://chromiumcodereview.appspot.com/10808115/diff/1/media/base/run_all_uni... File media/base/run_all_unittests.cc (right): https://chromiumcodereview.appspot.com/10808115/diff/1/media/base/run_all_uni... media/base/run_all_unittests.cc:31: // testing/android/native_test_wrapper.cc before main() is called. On 2012/07/25 00:19:45, nileshagrawal1 wrote: > On 2012/07/24 23:37:35, Ami Fischman wrote: > > Do you mean native_test_launcher.cc? > > Yes. Corrected. > > > Is this pattern really the right way to go? ISTM you'll end up having to have > > an equivalent of this code in every unittests target. Why not instead omit > the > > AEM from n_t_l.cc and let the main()'s continue to own their AtExitManagers? > > There is code executed in n_t_l.cc:RunTests (before main in called) which needs > an AtExitManager. Unfortunately this means that we need this #ifdef in all > unittest targets. I don't have a cleaner solution. I can look into having multiple AtExitManager not result in a crash. The DCHECK in AtExitManager is skipped for COMPONENET_BUILD.
On 2012/07/25 00:23:11, nileshagrawal1 wrote: > https://chromiumcodereview.appspot.com/10808115/diff/1/media/base/run_all_uni... > File media/base/run_all_unittests.cc (right): > > https://chromiumcodereview.appspot.com/10808115/diff/1/media/base/run_all_uni... > media/base/run_all_unittests.cc:31: // testing/android/native_test_wrapper.cc > before main() is called. > On 2012/07/25 00:19:45, nileshagrawal1 wrote: > > On 2012/07/24 23:37:35, Ami Fischman wrote: > > > Do you mean native_test_launcher.cc? > > > > Yes. Corrected. > > > > > Is this pattern really the right way to go? ISTM you'll end up having to > have > > > an equivalent of this code in every unittests target. Why not instead omit > > the > > > AEM from n_t_l.cc and let the main()'s continue to own their AtExitManagers? > > > > There is code executed in n_t_l.cc:RunTests (before main in called) which > needs > > an AtExitManager. Unfortunately this means that we need this #ifdef in all > > unittest targets. I don't have a cleaner solution. > > I can look into having multiple AtExitManager not result in a crash. The DCHECK > in AtExitManager is skipped for COMPONENET_BUILD. Looks like there is no easy way to remove the DCHECK(!g_top_manager) for android APK tests.
I feel bad holding your CL up, but I feel like I'm actually being asked to bless the upstreaming of n_t_l.cc (much more than the simple #ifdef in media/, which is the reason you pulled me in to this review), and I have a problem with that file as implemented. You can tell me to mind my own business and that you'll get a proper review for upstreaming n_t_l later, and go ahead with this CL. https://chromiumcodereview.appspot.com/10808115/diff/1/media/base/run_all_uni... File media/base/run_all_unittests.cc (right): https://chromiumcodereview.appspot.com/10808115/diff/1/media/base/run_all_uni... media/base/run_all_unittests.cc:31: // testing/android/native_test_wrapper.cc before main() is called. On 2012/07/25 00:19:45, nileshagrawal1 wrote: > There is code executed in n_t_l.cc:RunTests (before main in called) which needs > an AtExitManager. Unfortunately this means that we need this #ifdef in all > unittest targets. I don't have a cleaner solution. Can you make the pre-main() code not need an AEM? Or better yet not execute code pre-main? For example, does it make sense to have a base::InitializeAndroidForTesting() mimicking media::InitializeMediaLibraryForTesting() below? This would still require modifying each test target's main(), but at least it'd be obviouser why, and you wouldn't run into this sort of double-AEM-init problem. My real worry about this approach is that it sets the precedent that each main() needs to account for execution outside of itself, when pretty much every main() ever written is written with the assumption that the process starts at the top of main(). As an example of this assumption, the CommandLine::Init() above doesn't inspect its return code (and is a no-op when n_t_l.cc calls CL::Init() itself). https://chromiumcodereview.appspot.com/10808115/diff/8001/build/android/gtest... File build/android/gtest_filter/media_unittests_disabled (right): https://chromiumcodereview.appspot.com/10808115/diff/8001/build/android/gtest... build/android/gtest_filter/media_unittests_disabled:11: # Death tests are not supported on APK Is there a bug for this?
I appreciate your effort to get things fixed the right way. Your comments are most welcome. We are in "pay technical debt" mode :) Let me know how this patch looks. http://codereview.chromium.org/10808115/diff/1/media/base/run_all_unittests.cc File media/base/run_all_unittests.cc (right): http://codereview.chromium.org/10808115/diff/1/media/base/run_all_unittests.c... media/base/run_all_unittests.cc:31: // testing/android/native_test_wrapper.cc before main() is called. On 2012/07/25 17:09:59, Ami Fischman wrote: > On 2012/07/25 00:19:45, nileshagrawal1 wrote: > > There is code executed in n_t_l.cc:RunTests (before main in called) which > needs > > an AtExitManager. Unfortunately this means that we need this #ifdef in all > > unittest targets. I don't have a cleaner solution. I take back my comment that we need similar code in all unittest targets. Most of these can reply on the exitmanager being created (or not) in base/test/test_suite.cc Like what I have tried in this CL. > > Can you make the pre-main() code not need an AEM? Or better yet not execute AtExitManager is needed because we need to setup the global application context (in base::android::InitApplicationContext). g_application_context in jni_android.cc is a LazyInstance and it needs an exitmanager. > code pre-main? For example, does it make sense to have a > base::InitializeAndroidForTesting() mimicking > media::InitializeMediaLibraryForTesting() below? > This would still require modifying each test target's main(), but at least it'd > be obviouser why, and you wouldn't run into this sort of double-AEM-init > problem. This approach is cleaner, but the issue with this is that we can't pass arguments to main. The java side provides an application context which needs to be set. We can export another method (not main) which can take arguments. A hacky way would be to make base::InitializeAndroidForTesting() call back statically into JAVA to get the application context. > My real worry about this approach is that it sets the precedent that each main() > needs to account for execution outside of itself, when pretty much every main() > ever written is written with the assumption that the process starts at the top > of main(). > As an example of this assumption, the CommandLine::Init() above doesn't inspect > its return code (and is a no-op when n_t_l.cc calls CL::Init() itself). http://codereview.chromium.org/10808115/diff/8001/build/android/gtest_filter/... File build/android/gtest_filter/media_unittests_disabled (right): http://codereview.chromium.org/10808115/diff/8001/build/android/gtest_filter/... build/android/gtest_filter/media_unittests_disabled:11: # Death tests are not supported on APK On 2012/07/25 17:09:59, Ami Fischman wrote: > Is there a bug for this? Done. http://codereview.chromium.org/10808115/diff/7004/media/base/run_all_unittest... File media/base/run_all_unittests.cc (left): http://codereview.chromium.org/10808115/diff/7004/media/base/run_all_unittest... media/base/run_all_unittests.cc:18: // By default command-line parsing happens only in TestSuite::Run(), but I think this can be removed now. TestSuite ctor calls PreInitialize which inits the command line. TestSuite::Run() calls Initialize() and we setup logging in TestSuite::Initialize() before media::InitializeMediaLibraryForTesting() is called. Arguments passed to InitLogging are slightly different here, but it looks like they are such to avoid any side effects. Let me know if my understanding is wrong.
LGTM % nits thanks for the cleanup! http://codereview.chromium.org/10808115/diff/7004/media/base/run_all_unittest... File media/base/run_all_unittests.cc (right): http://codereview.chromium.org/10808115/diff/7004/media/base/run_all_unittest... media/base/run_all_unittests.cc:13: TestSuiteNoAtExit(int argc, char** argv) : TestSuite(argc, argv, true) {} s/, true// instead? http://codereview.chromium.org/10808115/diff/7004/media/base/run_all_unittest... media/base/run_all_unittests.cc:20: // Run TestSuite::Initialize first so that logging in initialized. s/in/is/ http://codereview.chromium.org/10808115/diff/7004/media/base/run_all_unittest... media/base/run_all_unittests.cc:22: media::InitializeMediaLibraryForTesting(); It's strange-looking that this will be called multiple times, but luckily it's a no-op after the first time. How about a comment: // Run this here instead of main() to ensure an AtExitManager is already present. ?
Thanks for the detailed review. http://codereview.chromium.org/10808115/diff/7004/media/base/run_all_unittest... File media/base/run_all_unittests.cc (right): http://codereview.chromium.org/10808115/diff/7004/media/base/run_all_unittest... media/base/run_all_unittests.cc:13: TestSuiteNoAtExit(int argc, char** argv) : TestSuite(argc, argv, true) {} On 2012/07/25 23:06:01, Ami Fischman wrote: > s/, true// instead? Done. http://codereview.chromium.org/10808115/diff/7004/media/base/run_all_unittest... media/base/run_all_unittests.cc:20: // Run TestSuite::Initialize first so that logging in initialized. On 2012/07/25 23:06:01, Ami Fischman wrote: > s/in/is/ Done. http://codereview.chromium.org/10808115/diff/7004/media/base/run_all_unittest... media/base/run_all_unittests.cc:22: media::InitializeMediaLibraryForTesting(); On 2012/07/25 23:06:01, Ami Fischman wrote: > It's strange-looking that this will be called multiple times, but luckily it's a > no-op after the first time. Maybe I am missing something, but Initialize is called only once and hence media::InitializeMediaLibraryForTesting() will also be called only once ? > How about a comment: > // Run this here instead of main() to ensure an AtExitManager is already > present. > ? Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nileshagrawal@chromium.org/10808115/15007
Change committed as 148493
http://codereview.chromium.org/10808115/diff/7004/media/base/run_all_unittest... File media/base/run_all_unittests.cc (right): http://codereview.chromium.org/10808115/diff/7004/media/base/run_all_unittest... media/base/run_all_unittests.cc:22: media::InitializeMediaLibraryForTesting(); On 2012/07/26 00:54:02, nileshagrawal1 wrote: > On 2012/07/25 23:06:01, Ami Fischman wrote: > > It's strange-looking that this will be called multiple times, but luckily it's > a > > no-op after the first time. > > Maybe I am missing something, but Initialize is called only once and hence > media::InitializeMediaLibraryForTesting() will also be called only once ? I was thinking of gtest's SetUp method, which is called for each test case, but that's not what this is. Never mind. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
