Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2182303002:
Merging under test java into instrumentation test java. (Closed)
|
Created:
4 years, 4 months ago by smaier Modified:
4 years, 4 months ago CC:
chromium-reviews, jbudorick+watch_chromium.org, mikecase+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@runtimelibrary Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMerging under test java into instrumentation test java.
This enables us to run all ProGuard uninhibited by tests, since Proguard will
now consider both the tests and the tested code while performing its
optimizations. This is opposed to running ProGuard on the tested java,
keeping stuff around for the tests, which would then use the tested code as
a library.
This turns on optimizations and removes test code from the shipped apk, saving
us ~550kb in .dex size, and ~70kb memory per process.
BUG=620323, 625704, 626710
Committed: https://crrev.com/d50624e2e5a9d9fe13596d83e73385c9c44069e9
Cr-Commit-Position: refs/heads/master@{#410056}
Patch Set 1 #Patch Set 2 : Rebased #Patch Set 3 : Merging instrumentation test apks and under test apks into single test apk #Patch Set 4 : Merging instrumentation test apks and under test apks into single test apk #Patch Set 5 : Cleaning up for review #
Total comments: 12
Patch Set 6 : Addressing agrieve's comments #
Total comments: 7
Patch Set 7 : updated comment #
Total comments: 2
Patch Set 8 : rebased #Patch Set 9 : reenabling tests that this fixes #Depends on Patchset: Messages
Total messages: 54 (36 generated)
The CQ bit was checked by smaier@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: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by smaier@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by smaier@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by smaier@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...
smaier@chromium.org changed reviewers: + agrieve@chromium.org, yfriedman@chromium.org
agrieve@chromium.org: Once you're happy with this, lets get kerz to review it. yfriedman@chromium.org: Look at this whenever you get a chance.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Merging instrumentation test apks and under test apks into single test apk BUG=620323,625704,626710 ========== to ========== Merging under test java into instrumentation test java. This enables us to run all ProGuard uninhibited by tests, since Proguard will now consider both the tests and the tested code while performing its optimizations. This is opposed to running ProGuard on the tested java, keeping stuff around for the tests, which would then use the tested code as a library. This turns on optimizations and removes test code from the shipped apk, saving us ~550kb in .dex size, and ~70kb memory per process. BUG=620323,625704,626710 ==========
The CQ bit was checked by smaier@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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2182303002/diff/80001/base/android/base_progu... File base/android/base_proguard_config.flags (right): https://codereview.chromium.org/2182303002/diff/80001/base/android/base_progu... base/android/base_proguard_config.flags:5: -keepattributes *Annotation* From what I can tell here: http://proguard.sourceforge.net/manual/attributes.html This will keep things like @VisibleForTesting Unless it's the case that dexing then removes them? Might be worth a mention that this does not cause VisibleForTesting to affect dex size (besides the annotation class itself) https://codereview.chromium.org/2182303002/diff/80001/build/android/gyp/write... File build/android/gyp/write_build_config.py (right): https://codereview.chromium.org/2182303002/diff/80001/build/android/gyp/write... build/android/gyp/write_build_config.py:284: help='Whether to merge the test apk and tested apk into one.') nit: "Whether to add all jars from test-apk-config into this config's full classpath." https://codereview.chromium.org/2182303002/diff/80001/build/android/gyp/write... build/android/gyp/write_build_config.py:533: tested_apk_config = GetConfig(options.tested_apk_config) rather than expose the top-level config, we should just move ['java']['full_classpath'] to appear within deps_info. You'll just need to update the @FileArg spots in .gni that reference it. https://codereview.chromium.org/2182303002/diff/80001/build/config/android/in... File build/config/android/internal_rules.gni (right): https://codereview.chromium.org/2182303002/diff/80001/build/config/android/in... build/config/android/internal_rules.gni:232: args += [ "--merge-tested-apk" ] nit: write_build_config already has both proguard_enabled and --tested_apk passed to it. No need to add this flag. https://codereview.chromium.org/2182303002/diff/80001/build/config/android/ru... File build/config/android/rules.gni (right): https://codereview.chromium.org/2182303002/diff/80001/build/config/android/ru... build/config/android/rules.gni:1857: _proguard_configs += [ "//testing/android/proguard_for_test.flags" ] nit: It might be nicer to add this in via instrumentation_test_apk instrumentation_apk() is probably also a good spot to add a comment to explain that when proguard is enabled, the test apk contains all the java code, but the apk under test contains resources, assets & manifest. as an aside - I remembered what dalvik didn't like about our previous R.class change. It just doesn't like when a class in classloader A resolves a class from classloader B. This change moves everything to classloader B, so there's never a case where classloader A even gets its code run. https://codereview.chromium.org/2182303002/diff/80001/chrome/android/java_sou... File chrome/android/java_sources.gni (left): https://codereview.chromium.org/2182303002/diff/80001/chrome/android/java_sou... chrome/android/java_sources.gni:1107: "javatests/src/org/chromium/chrome/browser/download/DownloadInfoTest.java", nit: should make a separate review for deleting this so that it will be noticed in the commit message.
https://codereview.chromium.org/2182303002/diff/80001/base/android/base_progu... File base/android/base_proguard_config.flags (right): https://codereview.chromium.org/2182303002/diff/80001/base/android/base_progu... base/android/base_proguard_config.flags:5: -keepattributes *Annotation* On 2016/07/28 01:30:57, agrieve wrote: > From what I can tell here: > http://proguard.sourceforge.net/manual/attributes.html > > This will keep things like @VisibleForTesting > > Unless it's the case that dexing then removes them? Might be worth a mention > that this does not cause VisibleForTesting to affect dex size (besides the > annotation class itself) Done. https://codereview.chromium.org/2182303002/diff/80001/build/android/gyp/write... File build/android/gyp/write_build_config.py (right): https://codereview.chromium.org/2182303002/diff/80001/build/android/gyp/write... build/android/gyp/write_build_config.py:284: help='Whether to merge the test apk and tested apk into one.') On 2016/07/28 01:30:57, agrieve wrote: > nit: "Whether to add all jars from test-apk-config into this config's full > classpath." Done. https://codereview.chromium.org/2182303002/diff/80001/build/android/gyp/write... build/android/gyp/write_build_config.py:533: tested_apk_config = GetConfig(options.tested_apk_config) On 2016/07/28 01:30:57, agrieve wrote: > rather than expose the top-level config, we should just move > ['java']['full_classpath'] to appear within deps_info. You'll just need to > update the @FileArg spots in .gni that reference it. Done. https://codereview.chromium.org/2182303002/diff/80001/build/config/android/in... File build/config/android/internal_rules.gni (right): https://codereview.chromium.org/2182303002/diff/80001/build/config/android/in... build/config/android/internal_rules.gni:232: args += [ "--merge-tested-apk" ] On 2016/07/28 01:30:58, agrieve wrote: > nit: write_build_config already has both proguard_enabled and --tested_apk > passed to it. No need to add this flag. Done. https://codereview.chromium.org/2182303002/diff/80001/build/config/android/ru... File build/config/android/rules.gni (right): https://codereview.chromium.org/2182303002/diff/80001/build/config/android/ru... build/config/android/rules.gni:1857: _proguard_configs += [ "//testing/android/proguard_for_test.flags" ] On 2016/07/28 01:30:58, agrieve wrote: > nit: It might be nicer to add this in via instrumentation_test_apk > > instrumentation_apk() is probably also a good spot to add a comment to explain > that when proguard is enabled, the test apk contains all the java code, but the > apk under test contains resources, assets & manifest. > > as an aside - I remembered what dalvik didn't like about our previous R.class > change. It just doesn't like when a class in classloader A resolves a class from > classloader B. This change moves everything to classloader B, so there's never a > case where classloader A even gets its code run. Done. https://codereview.chromium.org/2182303002/diff/80001/chrome/android/java_sou... File chrome/android/java_sources.gni (left): https://codereview.chromium.org/2182303002/diff/80001/chrome/android/java_sou... chrome/android/java_sources.gni:1107: "javatests/src/org/chromium/chrome/browser/download/DownloadInfoTest.java", On 2016/07/28 01:30:58, agrieve wrote: > nit: should make a separate review for deleting this so that it will be noticed > in the commit message. Done.
agrieve@chromium.org changed reviewers: - yfriedman@chromium.org
agrieve@chromium.org changed reviewers: + jbudorick@chromium.org
lgtm. +jbudorick for seeing if he's okay with this approach.
https://codereview.chromium.org/2182303002/diff/100001/build/android/gyp/writ... File build/android/gyp/write_build_config.py (right): https://codereview.chromium.org/2182303002/diff/100001/build/android/gyp/writ... build/android/gyp/write_build_config.py:521: # An instrumentation test apk should include the java that is in the apk under Why should we do this two different ways rather than always merging the APKs?
https://codereview.chromium.org/2182303002/diff/100001/build/android/gyp/writ... File build/android/gyp/write_build_config.py (right): https://codereview.chromium.org/2182303002/diff/100001/build/android/gyp/writ... build/android/gyp/write_build_config.py:521: # An instrumentation test apk should include the java that is in the apk under On 2016/07/28 16:56:00, jbudorick wrote: > Why should we do this two different ways rather than always merging the APKs? ProGuard is the tool which does the merging. Thus, ProGuard on -> merging, ProGuard off -> no merging.
https://codereview.chromium.org/2182303002/diff/100001/build/android/gyp/writ... File build/android/gyp/write_build_config.py (right): https://codereview.chromium.org/2182303002/diff/100001/build/android/gyp/writ... build/android/gyp/write_build_config.py:521: # An instrumentation test apk should include the java that is in the apk under On 2016/07/28 18:09:51, smaier wrote: > On 2016/07/28 16:56:00, jbudorick wrote: > > Why should we do this two different ways rather than always merging the APKs? > > ProGuard is the tool which does the merging. Thus, ProGuard on -> merging, > ProGuard off -> no merging. I seem to have entirely misunderstood what you meant in your doc, then. I was under the impression that you had been intending to change the dex exclusion logic below to stop excluding the code from the apk under test. https://codereview.chromium.org/2182303002/diff/100001/build/android/gyp/writ... build/android/gyp/write_build_config.py:545: # Exclude dex files from the test apk that exist within the apk under test. (I expected a change here.)
On 2016/07/28 18:18:22, jbudorick wrote: > https://codereview.chromium.org/2182303002/diff/100001/build/android/gyp/writ... > File build/android/gyp/write_build_config.py (right): > > https://codereview.chromium.org/2182303002/diff/100001/build/android/gyp/writ... > build/android/gyp/write_build_config.py:521: # An instrumentation test apk > should include the java that is in the apk under > On 2016/07/28 18:09:51, smaier wrote: > > On 2016/07/28 16:56:00, jbudorick wrote: > > > Why should we do this two different ways rather than always merging the > APKs? > > > > ProGuard is the tool which does the merging. Thus, ProGuard on -> merging, > > ProGuard off -> no merging. > > I seem to have entirely misunderstood what you meant in your doc, then. I was > under the impression that you had been intending to change the dex exclusion > logic below to stop excluding the code from the apk under test. > > https://codereview.chromium.org/2182303002/diff/100001/build/android/gyp/writ... > build/android/gyp/write_build_config.py:545: # Exclude dex files from the test > apk that exist within the apk under test. > (I expected a change here.) and to be clear, I strongly disagree with handling this differently between proguard-enabled and proguard-disabled configurations.
https://codereview.chromium.org/2182303002/diff/100001/build/android/gyp/util... File build/android/gyp/util/proguard_util.py (left): https://codereview.chromium.org/2182303002/diff/100001/build/android/gyp/util... build/android/gyp/util/proguard_util.py:112: self._libraries += tested_apk_info['inputs'] @jbudorick Removing lines 108/109 and 112 are what enables ProGuard to combine the java code under test into the tests' java code. Previously, we told ProGuard to treat the java code from the apk-under-test as a library we can't touch - which isn't great when some functions the tests use are unknowingly inlined during the ProGuard run on the apk-under-test. https://codereview.chromium.org/2182303002/diff/100001/build/android/gyp/writ... File build/android/gyp/write_build_config.py (right): https://codereview.chromium.org/2182303002/diff/100001/build/android/gyp/writ... build/android/gyp/write_build_config.py:545: # Exclude dex files from the test apk that exist within the apk under test. On 2016/07/28 18:18:22, jbudorick wrote: > (I expected a change here.) Sadly, this code applies only to the non-proguard case - ProGuard duplicated this logic within proguard_util.py, which is what I changed.
https://codereview.chromium.org/2182303002/diff/100001/build/android/gyp/writ... File build/android/gyp/write_build_config.py (right): https://codereview.chromium.org/2182303002/diff/100001/build/android/gyp/writ... build/android/gyp/write_build_config.py:545: # Exclude dex files from the test apk that exist within the apk under test. On 2016/07/28 19:19:19, smaier wrote: > On 2016/07/28 18:18:22, jbudorick wrote: > > (I expected a change here.) > > Sadly, this code applies only to the non-proguard case - ProGuard duplicated > this logic within proguard_util.py, which is what I changed. So why isn't it changing here as well...?
After speaking offline, lgtm https://codereview.chromium.org/2182303002/diff/120001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadInfoTest.java (left): https://codereview.chromium.org/2182303002/diff/120001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadInfoTest.java:1: // Copyright 2013 The Chromium Authors. All rights reserved. Wasn't this moving to a separate CL?
The CQ bit was checked by smaier@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...
smaier@chromium.org changed reviewers: + bauerb@chromium.org, kerz@chromium.org, torne@chromium.org
torne@chromium.org: Please review changes in base/android/base_proguard_config.flags bauerb@chromium.org: Please review changes in chrome/android/BUILD.gn, cchrome/android/chrome_public_apk_tmpl.gn https://codereview.chromium.org/2182303002/diff/120001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadInfoTest.java (left): https://codereview.chromium.org/2182303002/diff/120001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadInfoTest.java:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2016/07/29 02:48:24, jbudorick wrote: > Wasn't this moving to a separate CL? https://codereview.chromium.org/2186993003/ - I just couldn't depend on it since I already am depending on another, unrelated to removing this test, CL.
Rubberstamp LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
base_proguard_config.flags LGTM
The CQ bit was checked by smaier@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from agrieve@chromium.org, jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/2182303002/#ps140001 (title: "rebased")
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2182133004 Patch 60001). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
The CQ bit was checked by smaier@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, jbudorick@chromium.org, agrieve@chromium.org, torne@chromium.org Link to the patchset: https://codereview.chromium.org/2182303002/#ps160001 (title: "reenabling tests that this fixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Merging under test java into instrumentation test java. This enables us to run all ProGuard uninhibited by tests, since Proguard will now consider both the tests and the tested code while performing its optimizations. This is opposed to running ProGuard on the tested java, keeping stuff around for the tests, which would then use the tested code as a library. This turns on optimizations and removes test code from the shipped apk, saving us ~550kb in .dex size, and ~70kb memory per process. BUG=620323,625704,626710 ========== to ========== Merging under test java into instrumentation test java. This enables us to run all ProGuard uninhibited by tests, since Proguard will now consider both the tests and the tested code while performing its optimizations. This is opposed to running ProGuard on the tested java, keeping stuff around for the tests, which would then use the tested code as a library. This turns on optimizations and removes test code from the shipped apk, saving us ~550kb in .dex size, and ~70kb memory per process. BUG=620323,625704,626710 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Merging under test java into instrumentation test java. This enables us to run all ProGuard uninhibited by tests, since Proguard will now consider both the tests and the tested code while performing its optimizations. This is opposed to running ProGuard on the tested java, keeping stuff around for the tests, which would then use the tested code as a library. This turns on optimizations and removes test code from the shipped apk, saving us ~550kb in .dex size, and ~70kb memory per process. BUG=620323,625704,626710 ========== to ========== Merging under test java into instrumentation test java. This enables us to run all ProGuard uninhibited by tests, since Proguard will now consider both the tests and the tested code while performing its optimizations. This is opposed to running ProGuard on the tested java, keeping stuff around for the tests, which would then use the tested code as a library. This turns on optimizations and removes test code from the shipped apk, saving us ~550kb in .dex size, and ~70kb memory per process. BUG=620323,625704,626710 Committed: https://crrev.com/d50624e2e5a9d9fe13596d83e73385c9c44069e9 Cr-Commit-Position: refs/heads/master@{#410056} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/d50624e2e5a9d9fe13596d83e73385c9c44069e9 Cr-Commit-Position: refs/heads/master@{#410056} |
