Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
307943009:
Android: adds missing dependencies for gfx_unittests. (Closed)
|
Created:
6 years, 6 months ago by bulach Modified:
6 years, 6 months ago CC:
chromium-reviews, Changwan Ryu Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAndroid: adds missing dependencies for gfx_unittests.
BUG=379037
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274483
Patch Set 1 #
Total comments: 4
Patch Set 2 : Comments #Patch Set 3 : Moves disabled tests #
Total comments: 4
Patch Set 4 : Remove autogenerated comment #
Total comments: 5
Messages
Total messages: 18 (0 generated)
ptal
thanks Marcus. Just to confirm, this makes the test suite run, but does it still crash? https://codereview.chromium.org/307943009/diff/1/ui/gfx/gfx_tests.gyp File ui/gfx/gfx_tests.gyp (right): https://codereview.chromium.org/307943009/diff/1/ui/gfx/gfx_tests.gyp#newcode133 ui/gfx/gfx_tests.gyp:133: 'gfx_unittests', my sort command keeps this last. please, have it below. https://codereview.chromium.org/307943009/diff/1/ui/gfx/test/run_all_unittest... File ui/gfx/test/run_all_unittests.cc (right): https://codereview.chromium.org/307943009/diff/1/ui/gfx/test/run_all_unittest... ui/gfx/test/run_all_unittests.cc:49: gfx::android::RegisterJni(base::android::AttachCurrentThread()); please, have this in GfxTestSuite::Initialize() and after base::TestSuite::Initialize() call.
thanks thiago! yes, it runs but crashes now on FontTest.LoadArial, in a NOTIMPLEMENTED(). if I disable that test, it runs various others but then crashes somewhere else later on. in order to bring this suite up, somebody will need to disable the tests that aren't relevant to android :) https://codereview.chromium.org/307943009/diff/1/ui/gfx/gfx_tests.gyp File ui/gfx/gfx_tests.gyp (right): https://codereview.chromium.org/307943009/diff/1/ui/gfx/gfx_tests.gyp#newcode133 ui/gfx/gfx_tests.gyp:133: 'gfx_unittests', On 2014/05/30 14:56:22, tfarina wrote: > my sort command keeps this last. please, have it below. Done. https://codereview.chromium.org/307943009/diff/1/ui/gfx/test/run_all_unittest... File ui/gfx/test/run_all_unittests.cc (right): https://codereview.chromium.org/307943009/diff/1/ui/gfx/test/run_all_unittest... ui/gfx/test/run_all_unittests.cc:49: gfx::android::RegisterJni(base::android::AttachCurrentThread()); On 2014/05/30 14:56:22, tfarina wrote: > please, have this in GfxTestSuite::Initialize() and after > base::TestSuite::Initialize() call. Done.
+Mike for gfx approval and advice on Font issue on Android. I'd like to understand how come this was not happening when this test was in ui_unittests. Or were it happening? Could we exclude it from android as a short term solution, so the test suite runs? I see ui_unittests excludes render_text_unittest.cc, but just that. I'm also curious as how come this has not been an issue on the bots. Are these test suites not being run there? This lgtm as a necessary band-aid to keep things moving, and we can iterate later if we need to land this CL as is.
ahn, I see! thanks for the clarification. I thought this was a brand new test suite. yeah, the tests were disabled in the "ui_unittests_disabled" file, and they weren't moved to "gfx_unittests_disabled". just did that on the latest patchset, everything is running fine now! :) note though, that afaict the gfx_unittests are NOT running in any bot.. we should probably check with infra, and add it: https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli...
https://codereview.chromium.org/307943009/diff/40001/build/android/pylib/gtes... File build/android/pylib/gtest/filter/ui_unittests_disabled (right): https://codereview.chromium.org/307943009/diff/40001/build/android/pylib/gtes... build/android/pylib/gtest/filter/ui_unittests_disabled:2: # This file was automatically generated by build/android/run_tests.py Is it comment still true? I don't see it in https://chromium.googlesource.com/chromium/chromium/+/trunk/build/android/. https://codereview.chromium.org/307943009/diff/40001/build/android/pylib/gtes... build/android/pylib/gtest/filter/ui_unittests_disabled:5: ClipboardTest.RTFTest This test is not more in this test suite. It was moved into interactive_ui_tests because it can't be parallelized. It is here now - https://chromium.googlesource.com/chromium/chromium/+/trunk/chrome/chrome_tes...
thanks! all addressed, another look please? https://codereview.chromium.org/307943009/diff/40001/build/android/pylib/gtes... File build/android/pylib/gtest/filter/ui_unittests_disabled (right): https://codereview.chromium.org/307943009/diff/40001/build/android/pylib/gtes... build/android/pylib/gtest/filter/ui_unittests_disabled:2: # This file was automatically generated by build/android/run_tests.py On 2014/05/30 18:55:16, tfarina wrote: > Is it comment still true? I don't see it in > https://chromium.googlesource.com/chromium/chromium/+/trunk/build/android/. oh, yeah, this file was autogenerated ages ago.. for a long time is being manually maintained. removed the comment. https://codereview.chromium.org/307943009/diff/40001/build/android/pylib/gtes... build/android/pylib/gtest/filter/ui_unittests_disabled:5: ClipboardTest.RTFTest On 2014/05/30 18:55:16, tfarina wrote: > This test is not more in this test suite. It was moved into interactive_ui_tests > because it can't be parallelized. > > It is here now - > https://chromium.googlesource.com/chromium/chromium/+/trunk/chrome/chrome_tes... Done.
SLGTM. Thanks.
I'd like to see an owner/reviewer highly knowledgable of Android development (ie. not myself) review this CL, if possible. https://codereview.chromium.org/307943009/diff/60001/build/android/pylib/gtes... File build/android/pylib/gtest/filter/gfx_unittests_disabled (left): https://codereview.chromium.org/307943009/diff/60001/build/android/pylib/gtes... build/android/pylib/gtest/filter/gfx_unittests_disabled:1: # List of suppressions Why do we have these files at all? There are two other more common ways of excluding tests from certain builds: (1) disabling individual tests and (2) excluding specific test files from the build. Would either of those be valid replacements for this odd third method that seems so disjoint from the actual test file contents? (ex. the ClipboardTest was removed from this suite, and that wasn't updated here...) Do the rest of these tests still exist and is this the complete set needed to be disabled? https://codereview.chromium.org/307943009/diff/60001/ui/gfx/gfx_tests.gyp File ui/gfx/gfx_tests.gyp (right): https://codereview.chromium.org/307943009/diff/60001/ui/gfx/gfx_tests.gyp#new... ui/gfx/gfx_tests.gyp:120: '../../testing/android/native_test.gyp:native_test_native_code', Why is this specified as an explicit dependency in so many gyp files? Shouldn't this be a dependency of base/base.gyp:test_support_base or something similar, to avoid having separate includes for every test target like this?
https://codereview.chromium.org/307943009/diff/60001/build/android/pylib/gtes... File build/android/pylib/gtest/filter/gfx_unittests_disabled (left): https://codereview.chromium.org/307943009/diff/60001/build/android/pylib/gtes... build/android/pylib/gtest/filter/gfx_unittests_disabled:1: # List of suppressions On 2014/06/02 17:36:08, msw wrote: > Why do we have these files at all? There are two other more common ways of > excluding tests from certain builds: (1) disabling individual tests and (2) > excluding specific test files from the build. Would either of those be valid > replacements for this odd third method that seems so disjoint from the actual > test file contents? (ex. the ClipboardTest was removed from this suite, and that > wasn't updated here...) Do the rest of these tests still exist and is this the > complete set needed to be disabled? Mike I think this question is ortogonal to this CL and should not block it. It was discussed here: https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/gtest-... Has a bug filed here: http://crbug.com/339980 IMO, it should be addressed separately, mainly because this patch is unbreaking gfx_unittests and I need it to continue my work.
I think Chris should be able to review this for Android. +cjhopman.
On 2014/06/02 17:56:08, tfarina wrote: > https://codereview.chromium.org/307943009/diff/60001/build/android/pylib/gtes... > File build/android/pylib/gtest/filter/gfx_unittests_disabled (left): > > https://codereview.chromium.org/307943009/diff/60001/build/android/pylib/gtes... > build/android/pylib/gtest/filter/gfx_unittests_disabled:1: # List of > suppressions > On 2014/06/02 17:36:08, msw wrote: > > Why do we have these files at all? There are two other more common ways of > > excluding tests from certain builds: (1) disabling individual tests and (2) > > excluding specific test files from the build. Would either of those be valid > > replacements for this odd third method that seems so disjoint from the actual > > test file contents? (ex. the ClipboardTest was removed from this suite, and > that > > wasn't updated here...) Do the rest of these tests still exist and is this the > > complete set needed to be disabled? > > Mike I think this question is ortogonal to this CL and should not block it. > > It was discussed here: > https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/gtest-... > > Has a bug filed here: > http://crbug.com/339980 > > IMO, it should be addressed separately, mainly because this patch is unbreaking > gfx_unittests and I need it to continue my work. I wouldn't block this CL on either of my comments, but they were worth noting. I'm glad there's a bug to remove build/android/pylib/gtest/filter files. Also, thanks for adding an Android reviewer, I'll wait for their thoughts.
lgtm https://codereview.chromium.org/307943009/diff/60001/ui/gfx/gfx_tests.gyp File ui/gfx/gfx_tests.gyp (right): https://codereview.chromium.org/307943009/diff/60001/ui/gfx/gfx_tests.gyp#new... ui/gfx/gfx_tests.gyp:120: '../../testing/android/native_test.gyp:native_test_native_code', On 2014/06/02 17:36:08, msw wrote: > Why is this specified as an explicit dependency in so many gyp files? Shouldn't > this be a dependency of base/base.gyp:test_support_base or something similar, to > avoid having separate includes for every test target like this? Yeah, I'd like to see us come up with something where we don't have to explicitly depend on this everywhere. I'd say that this stuff is similar to base/test/launcher/* and, so, including it in base_test_support should be fine. No need to block this change on that, though.
lgtm https://codereview.chromium.org/307943009/diff/60001/ui/gfx/gfx_tests.gyp File ui/gfx/gfx_tests.gyp (right): https://codereview.chromium.org/307943009/diff/60001/ui/gfx/gfx_tests.gyp#new... ui/gfx/gfx_tests.gyp:120: '../../testing/android/native_test.gyp:native_test_native_code', On 2014/06/02 21:27:10, cjhopman wrote: > On 2014/06/02 17:36:08, msw wrote: > > Why is this specified as an explicit dependency in so many gyp files? > Shouldn't > > this be a dependency of base/base.gyp:test_support_base or something similar, > to > > avoid having separate includes for every test target like this? > > Yeah, I'd like to see us come up with something where we don't have to > explicitly depend on this everywhere. I'd say that this stuff is similar to > base/test/launcher/* and, so, including it in base_test_support should be fine. > > No need to block this change on that, though. > cool, please do follow up or file a bug about this.
The CQ bit was checked by tfarina@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bulach@chromium.org/307943009/60001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
Message was sent while issue was closed.
Change committed as 274483 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
