Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1716863002:
Raise exception when all devices fail to list tests. (Closed)
|
Created:
4 years, 10 months ago by BigBossZhiling Modified:
4 years, 9 months ago Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRaise exception when all devices fail to list tests.
When devices fail to list tests, instead of treating the result of list tests, which is None as an empty list, we should raise an exception.
BUG=585585
Committed: https://crrev.com/84802d80ccf691838d7e3d8fcbb3a87ae5e6eca9
Cr-Commit-Position: refs/heads/master@{#379925}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Fixed presubmit warnings. #Patch Set 3 : simplify and changed exception to be thrown #
Total comments: 5
Patch Set 4 : fixes #
Total comments: 2
Patch Set 5 : fixes #Messages
Total messages: 17 (6 generated)
Description was changed from ========== Raise exception when all devices fail to list tests. When devices fail to list tests, instead of treating the result of list tests, which is None as an empty list, we should raise an exception. BUG=585585 ========== to ========== Raise exception when all devices fail to list tests. When devices fail to list tests, instead of treating the result of list tests, which is None as an empty list, we should raise an exception. BUG=585585 ==========
hzl@google.com changed reviewers: + mikecase@chromium.org
hzl@google.com changed reviewers: + jbudorick@chromium.org
https://codereview.chromium.org/1716863002/diff/1/build/android/pylib/local/d... File build/android/pylib/local/device/local_device_gtest_run.py (right): https://codereview.chromium.org/1716863002/diff/1/build/android/pylib/local/d... build/android/pylib/local/device/local_device_gtest_run.py:322: if reduce(check_both_devices_failed, map(check_one_device_failed, test_lists)): You could just do "if all(map(check_one_device_failed, test_lists))" instead of using reduce. Also, you could simplify if further and just do something like... if all([tl is None for tl in test_lists]): raise Exception() ^ I think that should work https://codereview.chromium.org/1716863002/diff/1/build/android/pylib/local/d... build/android/pylib/local/device/local_device_gtest_run.py:324: raise Exception('Failed to list tests on all devices') Look around in devil/android/device_errors.py for a more appropriate error. My guess would do something like raise a device_errors.CommandFailedError https://codereview.chromium.org/1716863002/diff/1/build/android/pylib/local/d... build/android/pylib/local/device/local_device_gtest_run.py:327: # all devices fail. nit: Remove this TODO.
https://codereview.chromium.org/1716863002/diff/1/build/android/pylib/local/d... File build/android/pylib/local/device/local_device_gtest_run.py (right): https://codereview.chromium.org/1716863002/diff/1/build/android/pylib/local/d... build/android/pylib/local/device/local_device_gtest_run.py:322: if reduce(check_both_devices_failed, map(check_one_device_failed, test_lists)): On 2016/02/19 19:03:42, mikecase wrote: > You could just do "if all(map(check_one_device_failed, test_lists))" instead of > using reduce. > > Also, you could simplify if further and just do something like... > > if all([tl is None for tl in test_lists]): > raise Exception() > > ^ I think that should work Done. https://codereview.chromium.org/1716863002/diff/1/build/android/pylib/local/d... build/android/pylib/local/device/local_device_gtest_run.py:324: raise Exception('Failed to list tests on all devices') On 2016/02/19 19:03:42, mikecase wrote: > Look around in devil/android/device_errors.py for a more appropriate error. > > My guess would do something like raise a device_errors.CommandFailedError Done.
Gave you some nits. Should be good after that. Thanks for adding this :D https://codereview.chromium.org/1716863002/diff/40001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_gtest_run.py (right): https://codereview.chromium.org/1716863002/diff/40001/build/android/pylib/loc... build/android/pylib/local/device/local_device_gtest_run.py:319: # If all devices failed to list_tests, raise an exception. super nit: s/list_tests/list tests https://codereview.chromium.org/1716863002/diff/40001/build/android/pylib/loc... build/android/pylib/local/device/local_device_gtest_run.py:320: if all([tests == None for tests in test_lists]): super nit: To match the variable naming below... s/tests/tl Also super nit: "tests is None" is probably more pythonic than "==" None https://codereview.chromium.org/1716863002/diff/40001/build/android/pylib/loc... build/android/pylib/local/device/local_device_gtest_run.py:321: # Now we know that all devices failed to do list_tests. super nit: Comment is probably unnecessary given you have a comment above explaining what you are doing. https://codereview.chromium.org/1716863002/diff/40001/build/android/pylib/loc... build/android/pylib/local/device/local_device_gtest_run.py:323: "Failed to list_tests on all devices") super nit: s/list_tests/list tests super nit: single quotes (style is single quotes in python unless you have a single quote in the string) Also, while I'm being annoying with all my nits. Should probably read (to make it more clear)... 'Failed to list tests on any device.' OR 'All devices failed to list tests.'
https://codereview.chromium.org/1716863002/diff/40001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_gtest_run.py (right): https://codereview.chromium.org/1716863002/diff/40001/build/android/pylib/loc... build/android/pylib/local/device/local_device_gtest_run.py:319: # If all devices failed to list_tests, raise an exception. On 2016/02/19 23:41:53, mikecase wrote: > super nit: s/list_tests/list tests Done.
1 last comment. Just be sure to test this locally before you commit. https://codereview.chromium.org/1716863002/diff/60001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_gtest_run.py (right): https://codereview.chromium.org/1716863002/diff/60001/build/android/pylib/loc... build/android/pylib/local/device/local_device_gtest_run.py:319: # If all devices failed to s/list_tests/list tests, raise an exception. In my previous comment (I probably wrote it incorrectly) but I meant replace "list_lists" with "list tests". So now my comment is replace "s/list_tests/list tests" with "list tests".
https://codereview.chromium.org/1716863002/diff/60001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_gtest_run.py (right): https://codereview.chromium.org/1716863002/diff/60001/build/android/pylib/loc... build/android/pylib/local/device/local_device_gtest_run.py:319: # If all devices failed to s/list_tests/list tests, raise an exception. On 2016/02/25 16:43:44, mikecase wrote: > In my previous comment (I probably wrote it incorrectly) but I meant > replace "list_lists" with "list tests". > > So now my comment is replace "s/list_tests/list tests" with "list tests". Done.
lgtm
The CQ bit was checked by hzl@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1716863002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1716863002/80001
Message was sent while issue was closed.
Description was changed from ========== Raise exception when all devices fail to list tests. When devices fail to list tests, instead of treating the result of list tests, which is None as an empty list, we should raise an exception. BUG=585585 ========== to ========== Raise exception when all devices fail to list tests. When devices fail to list tests, instead of treating the result of list tests, which is None as an empty list, we should raise an exception. BUG=585585 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Raise exception when all devices fail to list tests. When devices fail to list tests, instead of treating the result of list tests, which is None as an empty list, we should raise an exception. BUG=585585 ========== to ========== Raise exception when all devices fail to list tests. When devices fail to list tests, instead of treating the result of list tests, which is None as an empty list, we should raise an exception. BUG=585585 Committed: https://crrev.com/84802d80ccf691838d7e3d8fcbb3a87ae5e6eca9 Cr-Commit-Position: refs/heads/master@{#379925} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/84802d80ccf691838d7e3d8fcbb3a87ae5e6eca9 Cr-Commit-Position: refs/heads/master@{#379925} |
