Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1964183003:
Fixed coverage dir for instrumentation tests. (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. |
DescriptionFixed coverage dir for instrumentation tests.
Fixed coverage dir option for instrumentation tests, so that we should
be able to pull run time coverage data from devices to bot.
BUG=608072
Committed: https://crrev.com/111b5841249ab4c8e14ed1b0af2328d5e739a140
Cr-Commit-Position: refs/heads/master@{#395419}
Patch Set 1 #
Total comments: 12
Patch Set 2 : fixes #
Total comments: 1
Patch Set 3 : fixes #
Total comments: 3
Messages
Total messages: 23 (6 generated)
hzl@google.com changed reviewers: + jbudorick@chromium.org, mikecase@chromium.org
Looks good. Mostly minor comments. https://codereview.chromium.org/1964183003/diff/1/build/android/pylib/instrum... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/1964183003/diff/1/build/android/pylib/instrum... build/android/pylib/instrumentation/instrumentation_test_instance.py:397: def CoverageDirectory(self): Nit: To keep with the style of much of the other code I would change this to @property def coverage_directory(self): return self._coverage_directory https://codereview.chromium.org/1964183003/diff/1/build/android/pylib/local/d... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/1964183003/diff/1/build/android/pylib/local/d... build/android/pylib/local/device/local_device_instrumentation_test_run.py:192: coverage_basename = '%s.ec' % test['method'] This will crash if test is a list, which I think it sometimes can be. https://codereview.chromium.org/1964183003/diff/1/build/android/pylib/local/d... build/android/pylib/local/device/local_device_instrumentation_test_run.py:196: coverage_basename) This indentation is a bit off. Probably do something like... coverage_device_file = ( '%s/%s/%s' % ( device.GetExternalStoragePath(), 'chrome/test/coverage', coverage_basename)) OR coverage_device_file = '%s/%s/%s' % (device.GetExternalStoragePath(), 'chrome/test/coverage', coverage_basename) Wow, pretty much all indentation for this looks super awkward. Oh well.... https://codereview.chromium.org/1964183003/diff/1/build/android/pylib/local/d... build/android/pylib/local/device/local_device_instrumentation_test_run.py:291: device.RunShellCommand('rm -f %s' % coverage_device_file) Might* be faster to pull all of the files at the end of the test run instead of after running each test.
https://codereview.chromium.org/1964183003/diff/1/build/android/pylib/instrum... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/1964183003/diff/1/build/android/pylib/instrum... build/android/pylib/instrumentation/instrumentation_test_instance.py:397: def CoverageDirectory(self): On 2016/05/11 17:39:58, mikecase wrote: > Nit: To keep with the style of much of the other code I would change this to > > @property > def coverage_directory(self): > return self._coverage_directory this, and also alphabetize it within the other properties https://codereview.chromium.org/1964183003/diff/1/build/android/pylib/local/d... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/1964183003/diff/1/build/android/pylib/local/d... build/android/pylib/local/device/local_device_instrumentation_test_run.py:196: coverage_basename) On 2016/05/11 17:39:58, mikecase wrote: > This indentation is a bit off. Probably do something like... > > coverage_device_file = ( > '%s/%s/%s' % ( > device.GetExternalStoragePath(), > 'chrome/test/coverage', > coverage_basename)) > > OR > > coverage_device_file = '%s/%s/%s' % (device.GetExternalStoragePath(), > 'chrome/test/coverage', > coverage_basename) > > Wow, pretty much all indentation for this looks super awkward. Oh well.... This should instead be: coverage_device_file = posixpath.join( device.GetExternalStoragePath(), 'chrome', 'test', 'coverage', coverage_basename) (not sure if that last line is under 80 characters) https://codereview.chromium.org/1964183003/diff/1/build/android/pylib/local/d... build/android/pylib/local/device/local_device_instrumentation_test_run.py:201: if isinstance(test, list): yes, test can be a list.
https://codereview.chromium.org/1964183003/diff/1/build/android/pylib/instrum... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/1964183003/diff/1/build/android/pylib/instrum... build/android/pylib/instrumentation/instrumentation_test_instance.py:397: def CoverageDirectory(self): On 2016/05/11 17:39:58, mikecase wrote: > Nit: To keep with the style of much of the other code I would change this to > > @property > def coverage_directory(self): > return self._coverage_directory Done. https://codereview.chromium.org/1964183003/diff/1/build/android/pylib/local/d... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/1964183003/diff/1/build/android/pylib/local/d... build/android/pylib/local/device/local_device_instrumentation_test_run.py:192: coverage_basename = '%s.ec' % test['method'] On 2016/05/11 17:39:58, mikecase wrote: > This will crash if test is a list, which I think it sometimes can be. Done. https://codereview.chromium.org/1964183003/diff/1/build/android/pylib/local/d... build/android/pylib/local/device/local_device_instrumentation_test_run.py:196: coverage_basename) On 2016/05/17 00:07:33, jbudorick wrote: > On 2016/05/11 17:39:58, mikecase wrote: > > This indentation is a bit off. Probably do something like... > > > > coverage_device_file = ( > > '%s/%s/%s' % ( > > device.GetExternalStoragePath(), > > 'chrome/test/coverage', > > coverage_basename)) > > > > OR > > > > coverage_device_file = '%s/%s/%s' % (device.GetExternalStoragePath(), > > 'chrome/test/coverage', > > coverage_basename) > > > > Wow, pretty much all indentation for this looks super awkward. Oh well.... > > This should instead be: > > coverage_device_file = posixpath.join( > device.GetExternalStoragePath(), 'chrome', 'test', 'coverage', > coverage_basename) > > (not sure if that last line is under 80 characters) Done. https://codereview.chromium.org/1964183003/diff/1/build/android/pylib/local/d... build/android/pylib/local/device/local_device_instrumentation_test_run.py:201: if isinstance(test, list): On 2016/05/17 00:07:33, jbudorick wrote: > yes, test can be a list. Done. https://codereview.chromium.org/1964183003/diff/1/build/android/pylib/local/d... build/android/pylib/local/device/local_device_instrumentation_test_run.py:291: device.RunShellCommand('rm -f %s' % coverage_device_file) On 2016/05/11 17:39:58, mikecase wrote: > Might* be faster to pull all of the files at the end of the test run instead of > after running each test. I was thinking of collecting ec files after calling runtests, instead of calling runtest, but I find it pretty hard to bubble the 'device' object up a level. https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli... , since it is using 'self._env.parallel_devices'.
https://codereview.chromium.org/1964183003/diff/20001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/1964183003/diff/20001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:191: coverage_basename = '%s.ec' % (test[0]['method'] not a huge fan of just naming this .ec file after the first method in the list. Can we just name this something generic in this case since it gets deleted after the tests are run? Something like... tests_coverage_data.ec
https://codereview.chromium.org/1964183003/diff/40001/build/android/pylib/ins... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/1964183003/diff/40001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:395: self._coverage_directory = args.coverage_dir There doesnt seem like there is a coverage_dir arg for instrumentation tests?
https://codereview.chromium.org/1964183003/diff/40001/build/android/pylib/ins... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/1964183003/diff/40001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:395: self._coverage_directory = args.coverage_dir On 2016/05/20 21:00:41, mikecase wrote: > There doesnt seem like there is a coverage_dir arg for instrumentation tests? args.coverage_dir is the directory. It is not true and false.
mikecase@google.com changed reviewers: + mikecase@google.com
lgtm https://codereview.chromium.org/1964183003/diff/40001/build/android/pylib/ins... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/1964183003/diff/40001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:395: self._coverage_directory = args.coverage_dir On 2016/05/20 at 21:08:05, BigBossZhiling wrote: > On 2016/05/20 21:00:41, mikecase wrote: > > There doesnt seem like there is a coverage_dir arg for instrumentation tests? > > args.coverage_dir is the directory. It is not true and false. oops, I missed that --coverage-dir is defined for instr tests.
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/1964183003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1964183003/40001
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
lgtm
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/1964183003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1964183003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fixed coverage dir for instrumentation tests. Fixed coverage dir option for instrumentation tests, so that we should be able to pull run time coverage data from devices to bot. BUG=608072 ========== to ========== Fixed coverage dir for instrumentation tests. Fixed coverage dir option for instrumentation tests, so that we should be able to pull run time coverage data from devices to bot. BUG=608072 Committed: https://crrev.com/111b5841249ab4c8e14ed1b0af2328d5e739a140 Cr-Commit-Position: refs/heads/master@{#395419} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/111b5841249ab4c8e14ed1b0af2328d5e739a140 Cr-Commit-Position: refs/heads/master@{#395419} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
