Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
776303002:
Migrate DeviceUtils.SetJavaAsserts to adb_wrapper (Closed)
|
Created:
6 years ago by perezju Modified:
5 years, 10 months ago CC:
chromium-reviews, klundberg+watch_chromium.org, yfriedman+watch_chromium.org, jbudorick+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMigrate DeviceUtils.SetJavaAsserts to adb_wrapper
The implementation mirrors that of android_commands, but is hopefully
clearer and more efficient. ReadFile/WriteFile are used to read
persistent properties first, and then GetProp/SetProp are used for
the current runtime properties.
BUG=267773
Committed: https://crrev.com/f4d73eee22208905080ad07022a8a267dff6b85f
Cr-Commit-Position: refs/heads/master@{#314791}
Patch Set 1 #
Total comments: 4
Patch Set 2 : git rebase master #Patch Set 3 : upgrade to new interface of ReadFile #
Total comments: 3
Patch Set 4 : FindProperty -> find_java_property #
Total comments: 4
Patch Set 5 : addressed comments #
Messages
Total messages: 16 (3 generated)
perezju@chromium.org changed reviewers: + jbudorick@chromium.org
I'm going to hold this one until we get ReadFile done. But otherwise it's mostly ready for review. https://codereview.chromium.org/776303002/diff/1/build/android/pylib/device/d... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/776303002/diff/1/build/android/pylib/device/d... build/android/pylib/device/device_utils.py:64: return ''.join(s for line in lines for s in (line, '\n')) This seems quite general. And we might need it as well if we switch the implementation of ReadFile to return a string. Maybe should also sit in some pylib.util.* ? https://codereview.chromium.org/776303002/diff/1/build/android/pylib/device/d... build/android/pylib/device/device_utils.py:982: return None, default Not for this CL but, maybe we would like a Get/SetPersistentProp? https://codereview.chromium.org/776303002/diff/1/build/android/pylib/device/d... build/android/pylib/device/device_utils.py:988: properties = self.ReadFile(constants.DEVICE_LOCAL_PROPERTIES_PATH) we would change this to ReadFile(..).splitlines() if we decide to switch that interface. https://codereview.chromium.org/776303002/diff/1/build/android/pylib/device/d... File build/android/pylib/device/device_utils_test.py (right): https://codereview.chromium.org/776303002/diff/1/build/android/pylib/device/d... build/android/pylib/device/device_utils_test.py:1296: 'dalvik.vm.enableassertions=all\n'), Specially looking a this, yeah, it seems more natural to have both ReadFile/WriteFile deal with the text as a string.
ReadFile landed and stuck, so is this ready for rebase + review?
rebased and ready for review
https://codereview.chromium.org/776303002/diff/40001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/776303002/diff/40001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:1044: def FindPropery(lines, propname, default=None): local functions should be lower_case_with_underscores also, propery -> property https://codereview.chromium.org/776303002/diff/40001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:1059: index, value = FindPropery(properties, self.JAVA_ASSERT_PROPERTY, '') - why is find_property a local function at all? - why does find_property have a default kwarg if you're only calling it once and it's a local function? - why does that default kwarg have a default value that's not used by the single caller?
added a new patchset https://codereview.chromium.org/776303002/diff/40001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/776303002/diff/40001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:1059: index, value = FindPropery(properties, self.JAVA_ASSERT_PROPERTY, '') On 2015/02/03 14:42:55, jbudorick wrote: > - why is find_property a local function at all? > - why does find_property have a default kwarg if you're only calling it once and > it's a local function? > - why does that default kwarg have a default value that's not used by the single > caller? I guess in my mind the function "wanted" to be more than just a local function: it (kind-of) mimics |list.index| and |dict.get|. But yeah, it's probably only useful on a very limited number of situations (the list has key-value entries on a particular format), so I just made it more specific instead.
https://codereview.chromium.org/776303002/diff/60001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/776303002/diff/60001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:1044: def find_java_property(lines): This goes a little further than I intended with my comment (sorry). I think passing in the property name is fine (and would make it easier to extract the local property logic in the future, if we want to do so). https://codereview.chromium.org/776303002/diff/60001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:1068: # if we are here index is not None While I see how this is true, it's a little convoluted -- it'd be nice to have a sanity check here. (an assert is fine, I think)
https://codereview.chromium.org/776303002/diff/60001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/776303002/diff/60001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:1044: def find_java_property(lines): On 2015/02/03 15:56:04, jbudorick wrote: > This goes a little further than I intended with my comment (sorry). I think > passing in the property name is fine (and would make it easier to extract the > local property logic in the future, if we want to do so). Done. https://codereview.chromium.org/776303002/diff/60001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:1068: # if we are here index is not None On 2015/02/03 15:56:04, jbudorick wrote: > While I see how this is true, it's a little convoluted -- it'd be nice to have a > sanity check here. (an assert is fine, I think) Done.
lgtm
The CQ bit was checked by perezju@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/776303002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/f4d73eee22208905080ad07022a8a267dff6b85f Cr-Commit-Position: refs/heads/master@{#314791}
Message was sent while issue was closed.
qinmin@chromium.org changed reviewers: + qinmin@chromium.org
Message was sent while issue was closed.
I somehow unable to run instrumentation test after this change:
File "build/android/test_runner.py", line 1018, in <module>
sys.exit(main())
File "build/android/test_runner.py", line 1014, in main
return RunTestsCommand(args, parser)
File "build/android/test_runner.py", line 910, in RunTestsCommand
return _RunInstrumentationTests(args, devices)
File "build/android/test_runner.py", line 705, in _RunInstrumentationTests
num_retries=args.num_retries)
File
"/usr/local/google/code/clankium-master/src/build/android/pylib/base/test_dispatcher.py",
line 329, in RunTests
runners = _CreateRunners(runner_factory, devices, setup_timeout)
File
"/usr/local/google/code/clankium-master/src/build/android/pylib/base/test_dispatcher.py",
line 242, in _CreateRunners
threads.JoinAll(watchdog_timer.WatchdogTimer(timeout))
File
"/usr/local/google/code/clankium-master/src/build/android/pylib/utils/reraiser_thread.py",
line 143, in JoinAll
self._JoinAll(watcher)
File
"/usr/local/google/code/clankium-master/src/build/android/pylib/utils/reraiser_thread.py",
line 130, in _JoinAll
thread.ReraiseIfException()
File
"/usr/local/google/code/clankium-master/src/build/android/pylib/utils/reraiser_thread.py",
line 76, in run
self._ret = self._func(*self._args, **self._kwargs)
File
"/usr/local/google/code/clankium-master/src/build/android/pylib/base/test_dispatcher.py",
line 151, in _SetUp
runner.SetUp()
File
"/usr/local/google/code/clankium-master/src/build/android/pylib/instrumentation/test_runner.py",
line 99, in SetUp
if self.device.SetJavaAsserts(self.options.set_asserts):
File
"/usr/local/google/code/clankium-master/src/build/android/pylib/device/decorators.py",
line 56, in TimeoutRetryWrapper
return timeout_retry.Run(impl, timeout, retries)
File
"/usr/local/google/code/clankium-master/src/build/android/pylib/utils/timeout_retry.py",
line 161, in Run
thread_group.JoinAll(child_thread.GetWatcher())
File
"/usr/local/google/code/clankium-master/src/build/android/pylib/utils/reraiser_thread.py",
line 143, in JoinAll
self._JoinAll(watcher)
File
"/usr/local/google/code/clankium-master/src/build/android/pylib/utils/reraiser_thread.py",
line 130, in _JoinAll
thread.ReraiseIfException()
File
"/usr/local/google/code/clankium-master/src/build/android/pylib/utils/reraiser_thread.py",
line 76, in run
self._ret = self._func(*self._args, **self._kwargs)
File
"/usr/local/google/code/clankium-master/src/build/android/pylib/utils/timeout_retry.py",
line 150, in RunOnTimeoutThread
ret[0] = func(*args, **kwargs)
File
"/usr/local/google/code/clankium-master/src/build/android/pylib/device/decorators.py",
line 50, in impl
return f(*args, **kwargs)
File
"/usr/local/google/code/clankium-master/src/build/android/pylib/device/device_utils.py",
line 1062, in SetJavaAsserts
index, value = find_property(properties, self.JAVA_ASSERT_PROPERTY)
File
"/usr/local/google/code/clankium-master/src/build/android/pylib/device/device_utils.py",
line 1049, in find_property
key, value = (s.strip() for s in line.split('=', 1))
ValueError: need more than 1 value to unpack
Message was sent while issue was closed.
On 2015/02/09 22:34:54, qinmin wrote:
> I somehow unable to run instrumentation test after this change:
>
> File "build/android/test_runner.py", line 1018, in <module>
> sys.exit(main())
> File "build/android/test_runner.py", line 1014, in main
> return RunTestsCommand(args, parser)
> File "build/android/test_runner.py", line 910, in RunTestsCommand
> return _RunInstrumentationTests(args, devices)
> File "build/android/test_runner.py", line 705, in _RunInstrumentationTests
> num_retries=args.num_retries)
> File
>
"/usr/local/google/code/clankium-master/src/build/android/pylib/base/test_dispatcher.py",
> line 329, in RunTests
> runners = _CreateRunners(runner_factory, devices, setup_timeout)
> File
>
"/usr/local/google/code/clankium-master/src/build/android/pylib/base/test_dispatcher.py",
> line 242, in _CreateRunners
> threads.JoinAll(watchdog_timer.WatchdogTimer(timeout))
> File
>
"/usr/local/google/code/clankium-master/src/build/android/pylib/utils/reraiser_thread.py",
> line 143, in JoinAll
> self._JoinAll(watcher)
> File
>
"/usr/local/google/code/clankium-master/src/build/android/pylib/utils/reraiser_thread.py",
> line 130, in _JoinAll
> thread.ReraiseIfException()
> File
>
"/usr/local/google/code/clankium-master/src/build/android/pylib/utils/reraiser_thread.py",
> line 76, in run
> self._ret = self._func(*self._args, **self._kwargs)
> File
>
"/usr/local/google/code/clankium-master/src/build/android/pylib/base/test_dispatcher.py",
> line 151, in _SetUp
> runner.SetUp()
> File
>
"/usr/local/google/code/clankium-master/src/build/android/pylib/instrumentation/test_runner.py",
> line 99, in SetUp
> if self.device.SetJavaAsserts(self.options.set_asserts):
> File
>
"/usr/local/google/code/clankium-master/src/build/android/pylib/device/decorators.py",
> line 56, in TimeoutRetryWrapper
> return timeout_retry.Run(impl, timeout, retries)
> File
>
"/usr/local/google/code/clankium-master/src/build/android/pylib/utils/timeout_retry.py",
> line 161, in Run
> thread_group.JoinAll(child_thread.GetWatcher())
> File
>
"/usr/local/google/code/clankium-master/src/build/android/pylib/utils/reraiser_thread.py",
> line 143, in JoinAll
> self._JoinAll(watcher)
> File
>
"/usr/local/google/code/clankium-master/src/build/android/pylib/utils/reraiser_thread.py",
> line 130, in _JoinAll
> thread.ReraiseIfException()
> File
>
"/usr/local/google/code/clankium-master/src/build/android/pylib/utils/reraiser_thread.py",
> line 76, in run
> self._ret = self._func(*self._args, **self._kwargs)
> File
>
"/usr/local/google/code/clankium-master/src/build/android/pylib/utils/timeout_retry.py",
> line 150, in RunOnTimeoutThread
> ret[0] = func(*args, **kwargs)
> File
>
"/usr/local/google/code/clankium-master/src/build/android/pylib/device/decorators.py",
> line 50, in impl
> return f(*args, **kwargs)
> File
>
"/usr/local/google/code/clankium-master/src/build/android/pylib/device/device_utils.py",
> line 1062, in SetJavaAsserts
> index, value = find_property(properties, self.JAVA_ASSERT_PROPERTY)
> File
>
"/usr/local/google/code/clankium-master/src/build/android/pylib/device/device_utils.py",
> line 1049, in find_property
> key, value = (s.strip() for s in line.split('=', 1))
> ValueError: need more than 1 value to unpack
I think qsr@ landed a change to fix this in
https://codereview.chromium.org/910793002/
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
