Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1957023002:
Add directory option for JUnit coverage files. (Closed)
|
Created:
4 years, 7 months ago by BigBossZhiling Modified:
4 years, 7 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. |
DescriptionAdd directory option for JUnit coverage files.
Added runtime option --coverage-dir to let the users decide where to store
coverage.ec.
BUG=608072
Committed: https://crrev.com/de08f6f711ebd74fe493584e0694bcce51693318
Cr-Commit-Position: refs/heads/master@{#392727}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Added prefix of JVM to be more clear. #
Total comments: 6
Patch Set 3 : changed to use argparse #Patch Set 4 : doesn't work version #
Total comments: 4
Patch Set 5 : #
Total comments: 6
Patch Set 6 : #
Total comments: 4
Patch Set 7 : #Patch Set 8 : fixed incremental build #
Messages
Total messages: 34 (6 generated)
hzl@google.com changed reviewers: + jbudorick@chromium.org
JUnit coverage-dir can be set now. For example, we can set it in the following way. out-gn/Debug/bin/run_clank_junit_tests --coverage-dir /tmp/this_is_kidding_coverage.ec
https://codereview.chromium.org/1957023002/diff/1/build/android/gyp/create_ja... File build/android/gyp/create_java_binary_script.py (right): https://codereview.chromium.org/1957023002/diff/1/build/android/gyp/create_ja... build/android/gyp/create_java_binary_script.py:40: jvm_arguments = [ x for x in sys.argv[1:] if x.startswith('-D')] '-D' I think is the conventional prefix of jvm arguments. So I am here first separating between jvm arguments and jar arguments. https://codereview.chromium.org/1957023002/diff/1/build/android/test_runner.py File build/android/test_runner.py (right): https://codereview.chromium.org/1957023002/diff/1/build/android/test_runner.p... build/android/test_runner.py:462: group.add_argument( Without the extra argument, coverage.ec will be stored at its default location.
https://codereview.chromium.org/1957023002/diff/1/build/android/gyp/create_ja... File build/android/gyp/create_java_binary_script.py (right): https://codereview.chromium.org/1957023002/diff/1/build/android/gyp/create_ja... build/android/gyp/create_java_binary_script.py:42: java_cmd.extend(jvm_arguments) Why is this split necessary? https://codereview.chromium.org/1957023002/diff/1/build/android/test_runner.py File build/android/test_runner.py (right): https://codereview.chromium.org/1957023002/diff/1/build/android/test_runner.p... build/android/test_runner.py:462: group.add_argument( On 2016/05/07 01:12:01, BigBossZhiling wrote: > Without the extra argument, coverage.ec will be stored at its default location. I think that's ok.
mikecase@chromium.org changed reviewers: + mikecase@chromium.org
https://codereview.chromium.org/1957023002/diff/20001/build/android/gyp/creat... File build/android/gyp/create_java_binary_script.py (right): https://codereview.chromium.org/1957023002/diff/20001/build/android/gyp/creat... build/android/gyp/create_java_binary_script.py:41: jvm_arguments = [ x[3:] for x in sys.argv[1:] if x.startswith('JVM')] What does this 3 mean? https://codereview.chromium.org/1957023002/diff/20001/build/android/pylib/jun... File build/android/pylib/junit/test_runner.py (right): https://codereview.chromium.org/1957023002/diff/20001/build/android/pylib/jun... build/android/pylib/junit/test_runner.py:45: command.extend(['JVM-Demma.coverage.out.file=' + self._coverage_dir]) Where are the .ec files stored by default?
https://codereview.chromium.org/1957023002/diff/20001/build/android/gyp/creat... File build/android/gyp/create_java_binary_script.py (right): https://codereview.chromium.org/1957023002/diff/20001/build/android/gyp/creat... build/android/gyp/create_java_binary_script.py:40: # JVM argumments have prefix JVM. Let's try something a little different here: - use argparse - make callers pass in arguments via either --jvm-arg or --jar-arg (repeated) - use that to split instead of prefixes https://codereview.chromium.org/1957023002/diff/20001/build/android/pylib/jun... File build/android/pylib/junit/test_runner.py (right): https://codereview.chromium.org/1957023002/diff/20001/build/android/pylib/jun... build/android/pylib/junit/test_runner.py:45: command.extend(['JVM-Demma.coverage.out.file=' + self._coverage_dir]) On 2016/05/09 21:11:46, mikecase wrote: > Where are the .ec files stored by default? checkout root directory
https://codereview.chromium.org/1957023002/diff/20001/build/android/gyp/creat... File build/android/gyp/create_java_binary_script.py (right): https://codereview.chromium.org/1957023002/diff/20001/build/android/gyp/creat... build/android/gyp/create_java_binary_script.py:41: jvm_arguments = [ x[3:] for x in sys.argv[1:] if x.startswith('JVM')] On 2016/05/09 21:11:46, mikecase wrote: > What does this 3 mean? JVM arguments start with JVM to differentiate between JVM arguments and Jar arugments. For example, JVM arguments are 'JVM-DXXXX=YYYYYY', but when we actually pass it to JVM, we take away the JVM tag. https://codereview.chromium.org/1957023002/diff/20001/build/android/pylib/jun... File build/android/pylib/junit/test_runner.py (right): https://codereview.chromium.org/1957023002/diff/20001/build/android/pylib/jun... build/android/pylib/junit/test_runner.py:45: command.extend(['JVM-Demma.coverage.out.file=' + self._coverage_dir]) On 2016/05/09 21:11:46, mikecase wrote: > Where are the .ec files stored by default? By default it is saved in src directory.
clank_junit_tests: error: unrecognized arguments: -test-jars clank_junit_tests.jar -json-results-file /tmp/tmpwCcc_8 -Demma.coverage.out.file=/tmp/this_is_kidding_coverage.ec argparse thinks that "-test-jars clank_junit_tests.jar -json-results-file /tmp/tmpwCcc_8 -Demma.coverage.out.file=/tmp/this_is_kidding_coverage.ec" is an option, but actually "-test-jars ...." is an argument of -jar-args.
https://codereview.chromium.org/1957023002/diff/60001/build/android/gyp/creat... File build/android/gyp/create_java_binary_script.py (right): https://codereview.chromium.org/1957023002/diff/60001/build/android/gyp/creat... build/android/gyp/create_java_binary_script.py:43: parser.add_argument('-jar-args', nargs='*') This should be --jar-args and the nargs parameter shouldn't be here at all. https://codereview.chromium.org/1957023002/diff/60001/build/android/gyp/creat... build/android/gyp/create_java_binary_script.py:44: parser.add_argument('-jvm-args', nargs='*') Similarly, this should be --jvm-args and the nargs parameter shouldn't be here. https://codereview.chromium.org/1957023002/diff/60001/build/android/pylib/jun... File build/android/pylib/junit/test_runner.py (right): https://codereview.chromium.org/1957023002/diff/60001/build/android/pylib/jun... build/android/pylib/junit/test_runner.py:35: command.append('-jar-args') This should be: args = ['-test-jars', self._test_suite + '.jar', '-json-results-file', json_file.name] if self._test_filter: args.extend(['-gtest-filter', self._test_filter]) # etc command.extend([ '--jar-args', cmd_helper.SingleQuote(' '.join(args)) ]) if self._coverage_dir: command.extend([ '--jvm-args', cmd_helper.SingleQuote('-Demma.coverage.out.file=' + self._coverage_dir) ])
https://codereview.chromium.org/1957023002/diff/60001/build/android/pylib/jun... File build/android/pylib/junit/test_runner.py (right): https://codereview.chromium.org/1957023002/diff/60001/build/android/pylib/jun... build/android/pylib/junit/test_runner.py:35: command.append('-jar-args') On 2016/05/09 23:51:49, jbudorick wrote: > This should be: > > args = ['-test-jars', self._test_suite + '.jar', '-json-results-file', > json_file.name] > if self._test_filter: > args.extend(['-gtest-filter', self._test_filter]) > # etc > > command.extend([ > '--jar-args', > cmd_helper.SingleQuote(' '.join(args)) > ]) > > if self._coverage_dir: > command.extend([ > '--jvm-args', > cmd_helper.SingleQuote('-Demma.coverage.out.file=' + self._coverage_dir) > ]) instead of cmd_helper.SingleQuote, just do '"%s"' % ('-Demma.coverage.out.file=' + self._coverage_dir)
Please take a look. Thx!
https://codereview.chromium.org/1957023002/diff/80001/build/android/gyp/creat... File build/android/gyp/create_java_binary_script.py (right): https://codereview.chromium.org/1957023002/diff/80001/build/android/gyp/creat... build/android/gyp/create_java_binary_script.py:47: jvm_arguments = args.jvm_args[1:-1].split() Are you stripping off the quotes here? If so, I think doing args.jvm_args.strip('"') would be much clearer. https://codereview.chromium.org/1957023002/diff/80001/build/android/gyp/creat... build/android/gyp/create_java_binary_script.py:55: jar_arguments = args.jar_args[1:-1].split() same here. https://codereview.chromium.org/1957023002/diff/80001/build/android/pylib/jun... File build/android/pylib/junit/test_runner.py (right): https://codereview.chromium.org/1957023002/diff/80001/build/android/pylib/jun... build/android/pylib/junit/test_runner.py:36: '-json-results-file', json_file.name] nit: Line up the indents. args = ['-test-jars', self._test_suite + '.jar', '-json-results-file', json_file.name]
https://codereview.chromium.org/1957023002/diff/80001/build/android/gyp/creat... File build/android/gyp/create_java_binary_script.py (right): https://codereview.chromium.org/1957023002/diff/80001/build/android/gyp/creat... build/android/gyp/create_java_binary_script.py:47: jvm_arguments = args.jvm_args[1:-1].split() On 2016/05/10 19:24:46, mikecase wrote: > Are you stripping off the quotes here? > > If so, I think doing args.jvm_args.strip('"') would be much clearer. Done. https://codereview.chromium.org/1957023002/diff/80001/build/android/gyp/creat... build/android/gyp/create_java_binary_script.py:55: jar_arguments = args.jar_args[1:-1].split() On 2016/05/10 19:24:46, mikecase wrote: > same here. Done. https://codereview.chromium.org/1957023002/diff/80001/build/android/pylib/jun... File build/android/pylib/junit/test_runner.py (right): https://codereview.chromium.org/1957023002/diff/80001/build/android/pylib/jun... build/android/pylib/junit/test_runner.py:36: '-json-results-file', json_file.name] On 2016/05/10 19:24:46, mikecase wrote: > nit: Line up the indents. > > > args = ['-test-jars', self._test_suite + '.jar', > '-json-results-file', json_file.name] Done.
One final nit, lgtm https://codereview.chromium.org/1957023002/diff/100001/build/android/pylib/ju... File build/android/pylib/junit/test_runner.py (right): https://codereview.chromium.org/1957023002/diff/100001/build/android/pylib/ju... build/android/pylib/junit/test_runner.py:50: args.append('-Demma.coverage.out.file=' + self._coverage_dir) super picky nit: I think most places use the %s notation I would change this to '-Demma.coverage.out.file=%s' % self._coverage_dir
https://codereview.chromium.org/1957023002/diff/100001/build/android/gyp/crea... File build/android/gyp/create_java_binary_script.py (right): https://codereview.chromium.org/1957023002/diff/100001/build/android/gyp/crea... build/android/gyp/create_java_binary_script.py:47: jvm_arguments = args.jvm_args.strip('"').split() Was stripping necessary?
https://codereview.chromium.org/1957023002/diff/100001/build/android/gyp/crea... File build/android/gyp/create_java_binary_script.py (right): https://codereview.chromium.org/1957023002/diff/100001/build/android/gyp/crea... build/android/gyp/create_java_binary_script.py:47: jvm_arguments = args.jvm_args.strip('"').split() On 2016/05/10 19:42:10, jbudorick wrote: > Was stripping necessary? I tried. It seems that it is necessary to strip away the ". argparse doesn't take it away. https://codereview.chromium.org/1957023002/diff/100001/build/android/pylib/ju... File build/android/pylib/junit/test_runner.py (right): https://codereview.chromium.org/1957023002/diff/100001/build/android/pylib/ju... build/android/pylib/junit/test_runner.py:50: args.append('-Demma.coverage.out.file=' + self._coverage_dir) On 2016/05/10 19:41:14, mikecase wrote: > super picky nit: I think most places use the %s notation > > I would change this to > > '-Demma.coverage.out.file=%s' % self._coverage_dir Done.
The CQ bit was checked by hzl@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mikecase@chromium.org Link to the patchset: https://codereview.chromium.org/1957023002/#ps120001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1957023002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1957023002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Add directory option for JUnit coverage files. Added runtime option --coverage-dir to let the users decide where to store coverage.ec. BUG=608072 ========== to ========== Add directory option for JUnit coverage files. Added runtime option --coverage-dir to let the users decide where to store coverage.ec. BUG=608072 Committed: https://crrev.com/de08f6f711ebd74fe493584e0694bcce51693318 Cr-Commit-Position: refs/heads/master@{#392727} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/de08f6f711ebd74fe493584e0694bcce51693318 Cr-Commit-Position: refs/heads/master@{#392727}
Message was sent while issue was closed.
This CL breaks the build for me, with messages like:
FAILED: gen/third_party/gif_player/gif_player_java__compile_java__javac.d
gen/third_party/gif_player/gif_player_java__compile_java.javac.jar
gen/third_party/gif_player/gif_player_java__compile_java.javac.jar.md5.stamp
gen/third_party/gif_player/gif_player_java__compile_java.javac.jar.pdb
python ../../build/android/gyp/javac.py
--depfile=gen/third_party/gif_player/gif_player_java__compile_java__javac.d
--classpath=@FileArg\(gen/third_party/gif_player/gif_player_java.build_config:javac:interface_classpath\)
--jar-path=gen/third_party/gif_player/gif_player_java__compile_java.javac.jar
--java-srcjars=\[\]
--java-srcjars=@FileArg\(gen/third_party/gif_player/gif_player_java.build_config:javac:srcjars\)
--incremental --bootclasspath=lib.java/android.interface.jar --chromium-code=1
../../third_party/gif_player/src/jp/tomorrowkey/android/gifplayer/BaseGifDrawable.java
../../third_party/gif_player/src/jp/tomorrowkey/android/gifplayer/BaseGifImage.java
Traceback (most recent call last):
File "../../build/android/gyp/javac.py", line 393, in <module>
sys.exit(main(sys.argv[1:]))
File "../../build/android/gyp/javac.py", line 389, in main
pass_changes=True)
File "/media/treib/work/clankium/src/build/android/gyp/util/build_utils.py",
line 526, in CallAndWriteDepfileIfStale
pass_changes=True)
File "/media/treib/work/clankium/src/build/android/gyp/util/md5_check.py",
line 87, in CallAndRecordIfStale
function(*args)
File "/media/treib/work/clankium/src/build/android/gyp/util/build_utils.py",
line 510, in on_stale_md5
function(*args)
File "../../build/android/gyp/javac.py", line 383, in <lambda>
classpath_inputs),
File "../../build/android/gyp/javac.py", line 212, in _OnStaleMd5
attempt_build()
File "../../build/android/gyp/javac.py", line 210, in <lambda>
stderr_filter=ColorJavacOutput)
File "/media/treib/work/clankium/src/build/android/gyp/util/build_utils.py",
line 174, in CheckOutput
raise CalledProcessError(cwd, args, stdout + stderr)
util.build_utils.CalledProcessError: Command failed: ( cd
/media/treib/work/clankium/src/out/Release; bin/jmake -pdb
gen/third_party/gif_player/gif_player_java__compile_java.javac.jar.pdb -C-g
-C-encoding -CUTF-8 -classpath '' -C-sourcepath -C -bootclasspath
lib.java/android.interface.jar -C-source -C1.7 -C-target -C1.7
-C-Xlint:unchecked -C-Xlint:deprecation -d /tmp/tmpGfiiMw/classes
../../third_party/gif_player/src/jp/tomorrowkey/android/gifplayer/BaseGifDrawable.java
../../third_party/gif_player/src/jp/tomorrowkey/android/gifplayer/BaseGifImage.java
)
usage: jmake [-h] [--jar-args JAR_ARGS] [--jvm-args JVM_ARGS]
jmake: error: unrecognized arguments: -pdb
gen/third_party/gif_player/gif_player_java__compile_java.javac.jar.pdb -C-g
-C-encoding -CUTF-8 -classpath -C-sourcepath -C -bootclasspath
lib.java/android.interface.jar -C-source -C1.7 -C-target -C1.7
-C-Xlint:unchecked -C-Xlint:deprecation -d /tmp/tmpGfiiMw/classes
../../third_party/gif_player/src/jp/tomorrowkey/android/gifplayer/BaseGifDrawable.java
../../third_party/gif_player/src/jp/tomorrowkey/android/gifplayer/BaseGifImage.java
A "-pdb ..." parameter is passed to the (generated) jmake script, which is
rejected by the new argument parsing in create_java_binary_script.py.
Has anybody else seen this? Am I doing something wrong?
Message was sent while issue was closed.
On 2016/05/11 11:50:42, Marc Treib wrote: > This CL breaks the build for me, with messages like: > > FAILED: gen/third_party/gif_player/gif_player_java__compile_java__javac.d > gen/third_party/gif_player/gif_player_java__compile_java.javac.jar > gen/third_party/gif_player/gif_player_java__compile_java.javac.jar.md5.stamp > gen/third_party/gif_player/gif_player_java__compile_java.javac.jar.pdb > python ../../build/android/gyp/javac.py > --depfile=gen/third_party/gif_player/gif_player_java__compile_java__javac.d > --classpath=@FileArg\(gen/third_party/gif_player/gif_player_java.build_config:javac:interface_classpath\) > --jar-path=gen/third_party/gif_player/gif_player_java__compile_java.javac.jar > --java-srcjars=\[\] > --java-srcjars=@FileArg\(gen/third_party/gif_player/gif_player_java.build_config:javac:srcjars\) > --incremental --bootclasspath=lib.java/android.interface.jar --chromium-code=1 > ../../third_party/gif_player/src/jp/tomorrowkey/android/gifplayer/BaseGifDrawable.java > ../../third_party/gif_player/src/jp/tomorrowkey/android/gifplayer/BaseGifImage.java > Traceback (most recent call last): > File "../../build/android/gyp/javac.py", line 393, in <module> > sys.exit(main(sys.argv[1:])) > File "../../build/android/gyp/javac.py", line 389, in main > pass_changes=True) > File "/media/treib/work/clankium/src/build/android/gyp/util/build_utils.py", > line 526, in CallAndWriteDepfileIfStale > pass_changes=True) > File "/media/treib/work/clankium/src/build/android/gyp/util/md5_check.py", > line 87, in CallAndRecordIfStale > function(*args) > File "/media/treib/work/clankium/src/build/android/gyp/util/build_utils.py", > line 510, in on_stale_md5 > function(*args) > File "../../build/android/gyp/javac.py", line 383, in <lambda> > classpath_inputs), > File "../../build/android/gyp/javac.py", line 212, in _OnStaleMd5 > attempt_build() > File "../../build/android/gyp/javac.py", line 210, in <lambda> > stderr_filter=ColorJavacOutput) > File "/media/treib/work/clankium/src/build/android/gyp/util/build_utils.py", > line 174, in CheckOutput > raise CalledProcessError(cwd, args, stdout + stderr) > util.build_utils.CalledProcessError: Command failed: ( cd > /media/treib/work/clankium/src/out/Release; bin/jmake -pdb > gen/third_party/gif_player/gif_player_java__compile_java.javac.jar.pdb -C-g > -C-encoding -CUTF-8 -classpath '' -C-sourcepath -C -bootclasspath > lib.java/android.interface.jar -C-source -C1.7 -C-target -C1.7 > -C-Xlint:unchecked -C-Xlint:deprecation -d /tmp/tmpGfiiMw/classes > ../../third_party/gif_player/src/jp/tomorrowkey/android/gifplayer/BaseGifDrawable.java > ../../third_party/gif_player/src/jp/tomorrowkey/android/gifplayer/BaseGifImage.java > ) > usage: jmake [-h] [--jar-args JAR_ARGS] [--jvm-args JVM_ARGS] > jmake: error: unrecognized arguments: -pdb > gen/third_party/gif_player/gif_player_java__compile_java.javac.jar.pdb -C-g > -C-encoding -CUTF-8 -classpath -C-sourcepath -C -bootclasspath > lib.java/android.interface.jar -C-source -C1.7 -C-target -C1.7 > -C-Xlint:unchecked -C-Xlint:deprecation -d /tmp/tmpGfiiMw/classes > ../../third_party/gif_player/src/jp/tomorrowkey/android/gifplayer/BaseGifDrawable.java > ../../third_party/gif_player/src/jp/tomorrowkey/android/gifplayer/BaseGifImage.java > > > A "-pdb ..." parameter is passed to the (generated) jmake script, which is > rejected by the new argument parsing in create_java_binary_script.py. > > Has anybody else seen this? Am I doing something wrong? Guessing that's something specific to the incremental build. Reverting to allow time to investigate.
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/1973503002/ by jbudorick@chromium.org. The reason for reverting is: Seemingly breaks incremental builds..
Message was sent while issue was closed.
Description was changed from ========== Add directory option for JUnit coverage files. Added runtime option --coverage-dir to let the users decide where to store coverage.ec. BUG=608072 Committed: https://crrev.com/de08f6f711ebd74fe493584e0694bcce51693318 Cr-Commit-Position: refs/heads/master@{#392727} ========== to ========== Add directory option for JUnit coverage files. Added runtime option --coverage-dir to let the users decide where to store coverage.ec. BUG=608072 Committed: https://crrev.com/de08f6f711ebd74fe493584e0694bcce51693318 Cr-Commit-Position: refs/heads/master@{#392727} ==========
Fixed incremental build.
On 2016/05/11 22:12:56, BigBossZhiling wrote: > Fixed incremental build. upload it as a new CL with what originally landed as patchset 1 & the revised version as patchset 2 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
