Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1965013008:
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/e16476a8c069b9646370f75d71cad6cdc9c491e8
Cr-Commit-Position: refs/heads/master@{#394470}
Patch Set 1 #Patch Set 2 : fixed incremental builds #Patch Set 3 : used another way to fix it. #
Total comments: 8
Patch Set 4 : fixed nits #
Total comments: 1
Patch Set 5 : fixes #
Messages
Total messages: 22 (8 generated)
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 ==========
hzl@google.com changed reviewers: + jbudorick@chromium.org, mikecase@chromium.org
Fixed incremental builds.
Fixed incremental builds.
Please take a look at the fix. It would be great if you guys would be able to look at patch 2. The fix in patch 2 is giving me very weird errors. I suspect is that python's subprocess.popen method doesn't like args like ['xxx', 'xxx', 'xxx', '"YYYY"']. I guess, its only my guess that '"YYY"' is making popen fail. One other interesting thing is that, in the failure message of patch 2, it says "failure to execute 'ZZZZ' command", but if I try the command in command line, it works perfectly.
https://codereview.chromium.org/1965013008/diff/40001/build/android/gyp/creat... File build/android/gyp/create_java_binary_script.py (right): https://codereview.chromium.org/1965013008/diff/40001/build/android/gyp/creat... build/android/gyp/create_java_binary_script.py:52: except: What gets caught by this? Would this be resolved by ArgumentParser.parse_known_args: https://docs.python.org/3/library/argparse.html#argparse.ArgumentParser.parse... ? https://codereview.chromium.org/1965013008/diff/40001/build/android/pylib/jun... File build/android/pylib/junit/test_runner.py (right): https://codereview.chromium.org/1965013008/diff/40001/build/android/pylib/jun... build/android/pylib/junit/test_runner.py:22: self._coverage_dir = args.coverage_dir nit: alphabetize https://codereview.chromium.org/1965013008/diff/40001/build/android/pylib/jun... build/android/pylib/junit/test_runner.py:35: args = ['-test-jars', self._test_suite + '.jar', nit: call this jar_args https://codereview.chromium.org/1965013008/diff/40001/build/android/pylib/jun... build/android/pylib/junit/test_runner.py:48: args = [] nit: call this jvm_args
https://codereview.chromium.org/1965013008/diff/40001/build/android/gyp/creat... File build/android/gyp/create_java_binary_script.py (right): https://codereview.chromium.org/1965013008/diff/40001/build/android/gyp/creat... build/android/gyp/create_java_binary_script.py:52: except: On 2016/05/17 00:03:50, jbudorick wrote: > What gets caught by this? Would this be resolved by > ArgumentParser.parse_known_args: > https://docs.python.org/3/library/argparse.html#argparse.ArgumentParser.parse... > ? Done. https://codereview.chromium.org/1965013008/diff/40001/build/android/pylib/jun... File build/android/pylib/junit/test_runner.py (right): https://codereview.chromium.org/1965013008/diff/40001/build/android/pylib/jun... build/android/pylib/junit/test_runner.py:22: self._coverage_dir = args.coverage_dir On 2016/05/17 00:03:50, jbudorick wrote: > nit: alphabetize Done. https://codereview.chromium.org/1965013008/diff/40001/build/android/pylib/jun... build/android/pylib/junit/test_runner.py:35: args = ['-test-jars', self._test_suite + '.jar', On 2016/05/17 00:03:50, jbudorick wrote: > nit: call this jar_args Done. https://codereview.chromium.org/1965013008/diff/40001/build/android/pylib/jun... build/android/pylib/junit/test_runner.py:48: args = [] On 2016/05/17 00:03:50, jbudorick wrote: > nit: call this jvm_args Done.
lgtm with 1 comment https://codereview.chromium.org/1965013008/diff/60001/build/android/gyp/creat... File build/android/gyp/create_java_binary_script.py (right): https://codereview.chromium.org/1965013008/diff/60001/build/android/gyp/creat... build/android/gyp/create_java_binary_script.py:52: else: If jar_arguments is specified and there are unknown_args, then the unknown_args will just be ignored. This probably shouldn't ever happen, but maybe have the script raise an exception in this case so people don't screw this up.
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/1965013008/#ps80001 (title: "fixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1965013008/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1965013008/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by hzl@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1965013008/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1965013008/80001
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 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
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/e16476a8c069b9646370f75d71cad6cdc9c491e8 Cr-Commit-Position: refs/heads/master@{#394470} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/e16476a8c069b9646370f75d71cad6cdc9c491e8 Cr-Commit-Position: refs/heads/master@{#394470}
Message was sent while issue was closed.
On 2016/05/18 at 18:11:41, commit-bot wrote: > Patchset 5 (id:??) landed as https://crrev.com/e16476a8c069b9646370f75d71cad6cdc9c491e8 > Cr-Commit-Position: refs/heads/master@{#394470} Just after this was committed, there was a failure in a test in cc_unittests on the Android Tests bot: https://build.chromium.org/p/chromium.linux/builders/Android%20Tests/builds/2... Not yet sure whether that's a consistent failure, or whether this is related.
Message was sent while issue was closed.
On 2016/05/18 20:19:19, qyearsley wrote: > On 2016/05/18 at 18:11:41, commit-bot wrote: > > Patchset 5 (id:??) landed as > https://crrev.com/e16476a8c069b9646370f75d71cad6cdc9c491e8 > > Cr-Commit-Position: refs/heads/master@{#394470} > > Just after this was committed, there was a failure in a test in cc_unittests on > the Android Tests bot: > https://build.chromium.org/p/chromium.linux/builders/Android%20Tests/builds/2... > > Not yet sure whether that's a consistent failure, or whether this is related. Not related.
Message was sent while issue was closed.
On 2016/05/18 at 20:32:32, jbudorick wrote: > On 2016/05/18 20:19:19, qyearsley wrote: > > On 2016/05/18 at 18:11:41, commit-bot wrote: > > > Patchset 5 (id:??) landed as > > https://crrev.com/e16476a8c069b9646370f75d71cad6cdc9c491e8 > > > Cr-Commit-Position: refs/heads/master@{#394470} > > > > Just after this was committed, there was a failure in a test in cc_unittests on > > the Android Tests bot: > > https://build.chromium.org/p/chromium.linux/builders/Android%20Tests/builds/2... > > > > Not yet sure whether that's a consistent failure, or whether this is related. > > Not related. Thanks -- also, that apparently wasn't a consistent failure. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
