Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1913593002:
Enable emma code coverage for JUnit testcases. (Closed)
|
Created:
4 years, 8 months ago by BigBossZhiling Modified:
4 years, 8 months ago CC:
chromium-reviews, jbudorick+watch_chromium.org, mikecase+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnable emma code coverage for JUnit testcases.
After setting emma_coverage as true, and then run JUnit testcases, code
coverage information will be stored in coverage.ec. In order to read
coverage.ec file, you will need to find the corresponding <name>.em file
in out-gn/Debug and then merge the .em and .ec file into a txt or html.
BUG=604900
Committed: https://crrev.com/071f7c486154a89e15c8b796a8bd0f64573a7096
Cr-Commit-Position: refs/heads/master@{#389902}
Patch Set 1 #
Total comments: 8
Patch Set 2 : fixes #
Total comments: 4
Patch Set 3 : changed it into boolean #Patch Set 4 : change description #
Total comments: 2
Patch Set 5 : changed from root_out_dir to root_build_dir #Messages
Total messages: 36 (13 generated)
Description was changed from ========== Enable emma code coverage for JUnit testcases. After setting emma_coverage as true, and then run JUnit testcases, code coverage information will be stored in coverage.ec. In order to read coverage.ec file, you will need to run 'java -cp <emma.jar directory> emma instr -d <output directory> -ip out-gn/Debug/lib.java/clank_java.jar'. This is going to create a coverage.em file. Then we need to convert them into readable txt file, by calling this command, 'java -cp <emma.jar directory> emma report -r txt -in coverage.em -in coverage.ec. BUG=604900 ========== to ========== Enable emma code coverage for JUnit testcases. After setting emma_coverage as true, and then run JUnit testcases, code coverage information will be stored in coverage.ec. In order to read coverage.ec file, you will need to run 'java -cp <emma.jar directory> emma instr -d <output directory> -ip out-gn/Debug/lib.java/clank_java.jar'. This is going to create a coverage.em file. Then we need to convert them into readable txt file, by calling this command, 'java -cp <emma.jar directory> emma report -r txt -in coverage.em -in coverage.ec. BUG=604900 ==========
hzl@google.com changed reviewers: + jbudorick@chromium.org
Please take a look John.
https://codereview.chromium.org/1913593002/diff/1/build/android/gyp/create_ja... File build/android/gyp/create_java_binary_script.py (right): https://codereview.chromium.org/1913593002/diff/1/build/android/gyp/create_ja... build/android/gyp/create_java_binary_script.py:40: java_cmd.append("-noverify") Otherwise emma will run into a werid error. https://codereview.chromium.org/1913593002/diff/1/build/android/gyp/create_ja... build/android/gyp/create_java_binary_script.py:68: ['../../third_party/android_tools_internal/sdk/tools/lib/emma.jar'] After talking to Andrew, he said that since I don't need it during compilation, I might be able to just hard-code this here.
https://codereview.chromium.org/1913593002/diff/1/build/android/gyp/create_ja... File build/android/gyp/create_java_binary_script.py (right): https://codereview.chromium.org/1913593002/diff/1/build/android/gyp/create_ja... build/android/gyp/create_java_binary_script.py:68: ['../../third_party/android_tools_internal/sdk/tools/lib/emma.jar'] On 2016/04/22 21:37:02, BigBossZhiling wrote: > After talking to Andrew, he said that since I don't need it during compilation, > I might be able to just hard-code this here. I don't think we should be adding this unconditionally to the classpath. Can we do so only if coverage is enabled? (Also, this path should be third_party/android_tools/sdk/tools/lib/emma.jar)
mikecase@chromium.org changed reviewers: + mikecase@chromium.org
https://codereview.chromium.org/1913593002/diff/1/build/android/gyp/create_ja... File build/android/gyp/create_java_binary_script.py (right): https://codereview.chromium.org/1913593002/diff/1/build/android/gyp/create_ja... build/android/gyp/create_java_binary_script.py:68: ['../../third_party/android_tools_internal/sdk/tools/lib/emma.jar'] On 2016/04/22 at 21:37:02, BigBossZhiling wrote: > After talking to Andrew, he said that since I don't need it during compilation, I might be able to just hard-code this here. Do you need this in the classpath when running the test? If you do, you should modify the deps variable in GN when building jars with emma enabled. I didn't think you needed this in the classpath for tests though, I thought it was just to process the files emma generates. This script will process the generated emma files. https://code.google.com/p/chromium/codesearch#chromium/src/build/android/gene...
https://codereview.chromium.org/1913593002/diff/1/build/android/gyp/create_ja... File build/android/gyp/create_java_binary_script.py (right): https://codereview.chromium.org/1913593002/diff/1/build/android/gyp/create_ja... build/android/gyp/create_java_binary_script.py:68: ['../../third_party/android_tools_internal/sdk/tools/lib/emma.jar'] Yes, we need this classpath when we run the test, otherwise it will fail. We don't need this file in the process of compilation, we only need this file when we run the test (specifically this script). And we never build this file, it is always there in the 3rd party directory. I searched for gn files that call this create java binary script, and the owner of the file actually suggested me hard code the classpath here if I don't need the jar file for compilation, but just for this script. However I do agree that this is not the best place to add the classpath, given that this is the only explicit class path in the whole script. lol. So please point me to how to add it. Or I can ask Andrew again and tell him that its ok to bundle the jar file for compilation too so that we will have a more systematic way of adding dependencies. Thoughts? Thanks for sharing the link! On 2016/04/22 23:51:45, mikecase wrote: > On 2016/04/22 at 21:37:02, BigBossZhiling wrote: > > After talking to Andrew, he said that since I don't need it during > compilation, I might be able to just hard-code this here. > > Do you need this in the classpath when running the test? If you do, you should > modify the deps variable in GN when building jars with emma enabled. I didn't > think you needed this in the classpath for tests though, I thought it was just > to process the files emma generates. > > This script will process the generated emma files. > https://code.google.com/p/chromium/codesearch#chromium/src/build/android/gene...
https://codereview.chromium.org/1913593002/diff/1/build/android/gyp/create_ja... File build/android/gyp/create_java_binary_script.py (right): https://codereview.chromium.org/1913593002/diff/1/build/android/gyp/create_ja... build/android/gyp/create_java_binary_script.py:76: classpath = [os.path.relpath(p, run_dir) for p in classpath] I dont think hard coding the path will work. It makes the path relative to the output script here, and like I dont think something like... os.path.relpath('../../third_party/android_tools_internal/sdk/tools/lib/emma.jar', '/usr/local/..../src/out/Debug/bin') will be what you want. And since GN and GYP run differently, I don't think you can get it to work on both by hardcoding it. Have you tested it on both. I could be wrong, but be sure to test it on both and look at the generated script to check taht the path is correct on both.
On 2016/04/25 18:36:40, mikecase wrote: > https://codereview.chromium.org/1913593002/diff/1/build/android/gyp/create_ja... > File build/android/gyp/create_java_binary_script.py (right): > > https://codereview.chromium.org/1913593002/diff/1/build/android/gyp/create_ja... > build/android/gyp/create_java_binary_script.py:76: classpath = > [os.path.relpath(p, run_dir) for p in classpath] > I dont think hard coding the path will work. It makes the path relative to the > output script here, and like I dont think something like... > > os.path.relpath('../../third_party/android_tools_internal/sdk/tools/lib/emma.jar', > '/usr/local/..../src/out/Debug/bin') > > will be what you want. And since GN and GYP run differently, I don't think you > can get it to work on both by hardcoding it. Have you tested it on both. I could > be wrong, but be sure to test it on both and look at the generated script to > check taht the path is correct on both. You are absolutely right. I only tested on GN, not on GYP, and on GYP it doesn't seem to work. > <
hzl@google.com changed reviewers: + agrieve@chromium.org
https://codereview.chromium.org/1913593002/diff/1/build/android/gyp/create_ja... File build/android/gyp/create_java_binary_script.py (right): https://codereview.chromium.org/1913593002/diff/1/build/android/gyp/create_ja... build/android/gyp/create_java_binary_script.py:68: ['../../third_party/android_tools_internal/sdk/tools/lib/emma.jar'] On 2016/04/22 23:51:45, mikecase wrote: > On 2016/04/22 at 21:37:02, BigBossZhiling wrote: > > After talking to Andrew, he said that since I don't need it during > compilation, I might be able to just hard-code this here. > > Do you need this in the classpath when running the test? If you do, you should > modify the deps variable in GN when building jars with emma enabled. I didn't > think you needed this in the classpath for tests though, I thought it was just > to process the files emma generates. > > This script will process the generated emma files. > https://code.google.com/p/chromium/codesearch#chromium/src/build/android/gene... I took away my change and tried to run junit tests with gyp, but it still fails. Is it true that junit tests just don't work with gyp?
On 2016/04/26 00:01:46, BigBossZhiling wrote: > https://codereview.chromium.org/1913593002/diff/1/build/android/gyp/create_ja... > File build/android/gyp/create_java_binary_script.py (right): > > https://codereview.chromium.org/1913593002/diff/1/build/android/gyp/create_ja... > build/android/gyp/create_java_binary_script.py:68: > ['../../third_party/android_tools_internal/sdk/tools/lib/emma.jar'] > On 2016/04/22 23:51:45, mikecase wrote: > > On 2016/04/22 at 21:37:02, BigBossZhiling wrote: > > > After talking to Andrew, he said that since I don't need it during > > compilation, I might be able to just hard-code this here. > > > > Do you need this in the classpath when running the test? If you do, you should > > modify the deps variable in GN when building jars with emma enabled. I didn't > > think you needed this in the classpath for tests though, I thought it was just > > to process the files emma generates. > > > > This script will process the generated emma files. > > > https://code.google.com/p/chromium/codesearch#chromium/src/build/android/gene... > > I took away my change and tried to run junit tests with gyp, but it still fails. > Is it true that junit tests just don't work with gyp? no, the tests themselves should still work with gyp.
https://codereview.chromium.org/1913593002/diff/1/build/android/gyp/create_ja... File build/android/gyp/create_java_binary_script.py (right): https://codereview.chromium.org/1913593002/diff/1/build/android/gyp/create_ja... build/android/gyp/create_java_binary_script.py:68: ['../../third_party/android_tools_internal/sdk/tools/lib/emma.jar'] On 2016/04/26 00:01:46, BigBossZhiling wrote: > On 2016/04/22 23:51:45, mikecase wrote: > > On 2016/04/22 at 21:37:02, BigBossZhiling wrote: > > > After talking to Andrew, he said that since I don't need it during > > compilation, I might be able to just hard-code this here. > > > > Do you need this in the classpath when running the test? If you do, you should > > modify the deps variable in GN when building jars with emma enabled. I didn't > > think you needed this in the classpath for tests though, I thought it was just > > to process the files emma generates. > > > > This script will process the generated emma files. > > > https://code.google.com/p/chromium/codesearch#chromium/src/build/android/gene... > > I took away my change and tried to run junit tests with gyp, but it still fails. > Is it true that junit tests just don't work with gyp? I meant to hardcode it in the .gni file here: https://code.google.com/p/chromium/codesearch#chromium/src/build/config/andro... E.g. if (emma_coverage) { args += ["--classpath", rebase_path(emma_path, root_out_dir)] } Probably also want to create a separate flag for -noverify and pass that only when emma_coverage is set. I wouldn't bother getting this to work for GYP. GYP will hopefully by gone by the end of the week.
hello guys, please take a look!
https://codereview.chromium.org/1913593002/diff/20001/build/android/gyp/creat... File build/android/gyp/create_java_binary_script.py (right): https://codereview.chromium.org/1913593002/diff/20001/build/android/gyp/creat... build/android/gyp/create_java_binary_script.py:62: parser.add_option('--noverify', help='JVM flag: noverify.') nit: Change this to a boolean flag (action='store_true'). This will mean on the .gni side you'll need to not pass any value for false, and pass just "--noverify" for the true case. https://codereview.chromium.org/1913593002/diff/20001/build/config/android/in... File build/config/android/internal_rules.gni (right): https://codereview.chromium.org/1913593002/diff/20001/build/config/android/in... build/config/android/internal_rules.gni:247: "../../third_party/android_tools_internal/sdk/tools/lib/emma.jar", Use // syntax here: //third_party/android_tools_internal/sdk/tools/lib/emma.jar
changed it into a boolean. didn't know such a thing exists. https://codereview.chromium.org/1913593002/diff/20001/build/android/gyp/creat... File build/android/gyp/create_java_binary_script.py (right): https://codereview.chromium.org/1913593002/diff/20001/build/android/gyp/creat... build/android/gyp/create_java_binary_script.py:62: parser.add_option('--noverify', help='JVM flag: noverify.') On 2016/04/26 14:51:10, agrieve wrote: > nit: Change this to a boolean flag (action='store_true'). This will mean on the > .gni side you'll need to not pass any value for false, and pass just > "--noverify" for the true case. Done. https://codereview.chromium.org/1913593002/diff/20001/build/config/android/in... File build/config/android/internal_rules.gni (right): https://codereview.chromium.org/1913593002/diff/20001/build/config/android/in... build/config/android/internal_rules.gni:247: "../../third_party/android_tools_internal/sdk/tools/lib/emma.jar", On 2016/04/26 14:51:10, agrieve wrote: > Use // syntax here: //third_party/android_tools_internal/sdk/tools/lib/emma.jar Done.
On 2016/04/26 17:35:58, BigBossZhiling wrote: > changed it into a boolean. didn't know such a thing exists. > > https://codereview.chromium.org/1913593002/diff/20001/build/android/gyp/creat... > File build/android/gyp/create_java_binary_script.py (right): > > https://codereview.chromium.org/1913593002/diff/20001/build/android/gyp/creat... > build/android/gyp/create_java_binary_script.py:62: > parser.add_option('--noverify', help='JVM flag: noverify.') > On 2016/04/26 14:51:10, agrieve wrote: > > nit: Change this to a boolean flag (action='store_true'). This will mean on > the > > .gni side you'll need to not pass any value for false, and pass just > > "--noverify" for the true case. > > Done. > > https://codereview.chromium.org/1913593002/diff/20001/build/config/android/in... > File build/config/android/internal_rules.gni (right): > > https://codereview.chromium.org/1913593002/diff/20001/build/config/android/in... > build/config/android/internal_rules.gni:247: > "../../third_party/android_tools_internal/sdk/tools/lib/emma.jar", > On 2016/04/26 14:51:10, agrieve wrote: > > Use // syntax here: > //third_party/android_tools_internal/sdk/tools/lib/emma.jar > > Done. lgtm
Description was changed from ========== Enable emma code coverage for JUnit testcases. After setting emma_coverage as true, and then run JUnit testcases, code coverage information will be stored in coverage.ec. In order to read coverage.ec file, you will need to run 'java -cp <emma.jar directory> emma instr -d <output directory> -ip out-gn/Debug/lib.java/clank_java.jar'. This is going to create a coverage.em file. Then we need to convert them into readable txt file, by calling this command, 'java -cp <emma.jar directory> emma report -r txt -in coverage.em -in coverage.ec. BUG=604900 ========== to ========== Enable emma code coverage for JUnit testcases. After setting emma_coverage as true, and then run JUnit testcases, code coverage information will be stored in coverage.ec. In order to read coverage.ec file, you will need to find the corresponding <name>.em file in out-gn/Debug and then merge the .em and .ec file into a txt or html. BUG=604900 ==========
The CQ bit was checked by hzl@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from agrieve@chromium.org Link to the patchset: https://codereview.chromium.org/1913593002/#ps60001 (title: "change description")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1913593002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1913593002/60001
https://codereview.chromium.org/1913593002/diff/60001/build/config/android/in... File build/config/android/internal_rules.gni (right): https://codereview.chromium.org/1913593002/diff/60001/build/config/android/in... build/config/android/internal_rules.gni:248: root_out_dir), Not 100% sure, but I think you want root_build_dir instead of root_out_dir. Im not really clear on the difference between them, but there is some info here... https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/referen...
The CQ bit was unchecked by hzl@google.com
https://codereview.chromium.org/1913593002/diff/60001/build/config/android/in... File build/config/android/internal_rules.gni (right): https://codereview.chromium.org/1913593002/diff/60001/build/config/android/in... build/config/android/internal_rules.gni:248: root_out_dir), On 2016/04/26 17:59:36, mikecase wrote: > Not 100% sure, but I think you want root_build_dir instead of root_out_dir. Im > not really clear on the difference between them, but there is some info here... > https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/referen... Ah, yeah, that's true. The two values are different only when executing in the non-default toolchain (e.g. host_toolchain). root_build_dir is what scripts always run in.
The CQ bit was checked by hzl@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from agrieve@chromium.org Link to the patchset: https://codereview.chromium.org/1913593002/#ps80001 (title: "changed from root_out_dir to root_build_dir")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1913593002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1913593002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by jbudorick@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1913593002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1913593002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Enable emma code coverage for JUnit testcases. After setting emma_coverage as true, and then run JUnit testcases, code coverage information will be stored in coverage.ec. In order to read coverage.ec file, you will need to find the corresponding <name>.em file in out-gn/Debug and then merge the .em and .ec file into a txt or html. BUG=604900 ========== to ========== Enable emma code coverage for JUnit testcases. After setting emma_coverage as true, and then run JUnit testcases, code coverage information will be stored in coverage.ec. In order to read coverage.ec file, you will need to find the corresponding <name>.em file in out-gn/Debug and then merge the .em and .ec file into a txt or html. BUG=604900 Committed: https://crrev.com/071f7c486154a89e15c8b796a8bd0f64573a7096 Cr-Commit-Position: refs/heads/master@{#389902} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/071f7c486154a89e15c8b796a8bd0f64573a7096 Cr-Commit-Position: refs/heads/master@{#389902} |
