Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2544603002:
[Android] Add '--gtest_also_run_disabled_tests' logic in gtest runner (Closed)
|
Created:
4 years ago by shenghuazhang Modified:
4 years ago Reviewers:
jbudorick Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Android] Add '--gtest_also_run_disabled_tests' logic in gtest runner
Command line option '--gtest_also_run_disabled_tests' already exists in
test_runner.py, should build the logic in local_device_gtest_run.py in
order to run disabled tests.
This CL also unify '--gtest_repeat' for instrumentation test options.
BUG=669632
Committed: https://crrev.com/b58f48f40ef901780540975d4b5f4121e229277c
Cr-Commit-Position: refs/heads/master@{#435524}
Patch Set 1 #
Total comments: 8
Patch Set 2 : disabled_prefixes #
Total comments: 9
Patch Set 3 : remove run_disabled argument #Patch Set 4 : refactor _GenerateDisabledFilterString #
Total comments: 3
Patch Set 5 : add condition for flags #
Messages
Total messages: 17 (4 generated)
shenghuazhang@chromium.org changed reviewers: + jbudorick@chromium.org
https://codereview.chromium.org/2544603002/diff/1/build/android/pylib/gtest/g... File build/android/pylib/gtest/gtest_test_instance.py (right): https://codereview.chromium.org/2544603002/diff/1/build/android/pylib/gtest/g... build/android/pylib/gtest/gtest_test_instance.py:291: if args.run_disabled: This can just be self._run_disabled = args.run_disabled https://codereview.chromium.org/2544603002/diff/1/build/android/pylib/gtest/g... build/android/pylib/gtest/gtest_test_instance.py:412: def FilterTests(self, test_list, disabled_prefixes=None, run_disabled=False): Does this need to be exposed? Can this function just use self.gtest_also_run_disabled_tests? https://codereview.chromium.org/2544603002/diff/1/build/android/pylib/gtest/g... build/android/pylib/gtest/gtest_test_instance.py:442: if run_disabled: nit: disabled_prefixes = ['FAILS_', 'PRE_', 'MANUAL_'] if not run_disabled: disabled_prefixes += ['DISABLED_', 'FLAKY_'] ? https://codereview.chromium.org/2544603002/diff/1/build/android/pylib/local/d... File build/android/pylib/local/device/local_device_gtest_run.py (right): https://codereview.chromium.org/2544603002/diff/1/build/android/pylib/local/d... build/android/pylib/local/device/local_device_gtest_run.py:387: self._test_instance.gtest_also_run_disabled_tests) This shouldn't need a value, you should just be able to pass --gtest_also_run_disabled_tests
https://codereview.chromium.org/2544603002/diff/1/build/android/pylib/gtest/g... File build/android/pylib/gtest/gtest_test_instance.py (right): https://codereview.chromium.org/2544603002/diff/1/build/android/pylib/gtest/g... build/android/pylib/gtest/gtest_test_instance.py:291: if args.run_disabled: On 2016/11/30 21:05:56, jbudorick wrote: > This can just be > > self._run_disabled = args.run_disabled Done. https://codereview.chromium.org/2544603002/diff/1/build/android/pylib/gtest/g... build/android/pylib/gtest/gtest_test_instance.py:412: def FilterTests(self, test_list, disabled_prefixes=None, run_disabled=False): On 2016/11/30 21:05:56, jbudorick wrote: > Does this need to be exposed? Can this function just use > self.gtest_also_run_disabled_tests? Done. https://codereview.chromium.org/2544603002/diff/1/build/android/pylib/gtest/g... build/android/pylib/gtest/gtest_test_instance.py:442: if run_disabled: On 2016/11/30 21:05:56, jbudorick wrote: > nit: > > disabled_prefixes = ['FAILS_', 'PRE_', 'MANUAL_'] > if not run_disabled: > disabled_prefixes += ['DISABLED_', 'FLAKY_'] > > ? Done. https://codereview.chromium.org/2544603002/diff/1/build/android/pylib/local/d... File build/android/pylib/local/device/local_device_gtest_run.py (right): https://codereview.chromium.org/2544603002/diff/1/build/android/pylib/local/d... build/android/pylib/local/device/local_device_gtest_run.py:387: self._test_instance.gtest_also_run_disabled_tests) On 2016/11/30 21:05:56, jbudorick wrote: > This shouldn't need a value, you should just be able to pass > --gtest_also_run_disabled_tests Done. https://codereview.chromium.org/2544603002/diff/20001/build/android/pylib/gte... File build/android/pylib/gtest/gtest_test_instance.py (right): https://codereview.chromium.org/2544603002/diff/20001/build/android/pylib/gte... build/android/pylib/gtest/gtest_test_instance.py:440: if run_disabled: When 'disabled_prefixes' is an non-empty input, could delete 'DISABLED_' and 'FLAKY_' as well.
https://codereview.chromium.org/2544603002/diff/20001/build/android/pylib/gte... File build/android/pylib/gtest/gtest_test_instance.py (right): https://codereview.chromium.org/2544603002/diff/20001/build/android/pylib/gte... build/android/pylib/gtest/gtest_test_instance.py:435: run_disabled=False): Sorry, I should've made my comment about passing in run_disabled here. Does this need to be passed in, or can we use it directly? https://codereview.chromium.org/2544603002/diff/20001/build/android/pylib/gte... build/android/pylib/gtest/gtest_test_instance.py:440: if run_disabled: On 2016/11/30 21:58:01, shenghuazhang wrote: > When 'disabled_prefixes' is an non-empty input, could delete 'DISABLED_' and > 'FLAKY_' as well. This is a bit more complex than just appending them to the end of the list if run_disabled isn't set. https://codereview.chromium.org/2544603002/diff/20001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_gtest_run.py (right): https://codereview.chromium.org/2544603002/diff/20001/build/android/pylib/loc... build/android/pylib/local/device/local_device_gtest_run.py:385: flags += ' --gtest_also_run_disabled_tests' I don't think we should always be adding this.
I think that _GenerateDisabledFilterString logic needs to cover removing the inputted disabled_prefixes? https://codereview.chromium.org/2544603002/diff/20001/build/android/pylib/gte... File build/android/pylib/gtest/gtest_test_instance.py (right): https://codereview.chromium.org/2544603002/diff/20001/build/android/pylib/gte... build/android/pylib/gtest/gtest_test_instance.py:435: run_disabled=False): On 2016/11/30 23:17:11, jbudorick wrote: > Sorry, I should've made my comment about passing in run_disabled here. Does this > need to be passed in, or can we use it directly? Oh no we don't need to pass this... Will use the arg directly. https://codereview.chromium.org/2544603002/diff/20001/build/android/pylib/gte... build/android/pylib/gtest/gtest_test_instance.py:440: if run_disabled: On 2016/11/30 23:17:11, jbudorick wrote: > On 2016/11/30 21:58:01, shenghuazhang wrote: > > When 'disabled_prefixes' is an non-empty input, could delete 'DISABLED_' and > > 'FLAKY_' as well. > > This is a bit more complex than just appending them to the end of the list if > run_disabled isn't set. But it didn't consider to remove the inputted disabled_prefixes I think. When the 'disabled_prefixes' argument is passed in as e.g. disabled_prefixes = ['DISABLED_', 'FLAKY_', ...], and when run_disabled is set to true, the prefixes 'DISABLED_' and 'FLAKY_' won't be removed by that way? https://codereview.chromium.org/2544603002/diff/20001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_gtest_run.py (right): https://codereview.chromium.org/2544603002/diff/20001/build/android/pylib/loc... build/android/pylib/local/device/local_device_gtest_run.py:385: flags += ' --gtest_also_run_disabled_tests' On 2016/11/30 23:17:11, jbudorick wrote: > I don't think we should always be adding this. Done.
https://codereview.chromium.org/2544603002/diff/20001/build/android/pylib/gte... File build/android/pylib/gtest/gtest_test_instance.py (right): https://codereview.chromium.org/2544603002/diff/20001/build/android/pylib/gte... build/android/pylib/gtest/gtest_test_instance.py:440: if run_disabled: On 2016/11/30 23:52:44, shenghuazhang wrote: > On 2016/11/30 23:17:11, jbudorick wrote: > > On 2016/11/30 21:58:01, shenghuazhang wrote: > > > When 'disabled_prefixes' is an non-empty input, could delete 'DISABLED_' and > > > 'FLAKY_' as well. > > > > This is a bit more complex than just appending them to the end of the list if > > run_disabled isn't set. > > But it didn't consider to remove the inputted disabled_prefixes I think. When > the 'disabled_prefixes' argument is passed in as e.g. disabled_prefixes = > ['DISABLED_', 'FLAKY_', ...], and when run_disabled is set to true, the prefixes > 'DISABLED_' and 'FLAKY_' won't be removed by that way? That's a fair point, and I hadn't considered it. That option isn't used (any more?), though, so I think we can ignore that case.
Thanks for the analysis! Also, I think the '--gtest_also_run_disabled_tests' flag needs to be added in _RunTest somehow. Seems like it's also a device side command flag when determine whether running disabled tests. https://codereview.chromium.org/2544603002/diff/20001/build/android/pylib/gte... File build/android/pylib/gtest/gtest_test_instance.py (right): https://codereview.chromium.org/2544603002/diff/20001/build/android/pylib/gte... build/android/pylib/gtest/gtest_test_instance.py:440: if run_disabled: On 2016/11/30 23:54:14, jbudorick wrote: > On 2016/11/30 23:52:44, shenghuazhang wrote: > > On 2016/11/30 23:17:11, jbudorick wrote: > > > On 2016/11/30 21:58:01, shenghuazhang wrote: > > > > When 'disabled_prefixes' is an non-empty input, could delete 'DISABLED_' > and > > > > 'FLAKY_' as well. > > > > > > This is a bit more complex than just appending them to the end of the list > if > > > run_disabled isn't set. > > > > But it didn't consider to remove the inputted disabled_prefixes I think. When > > the 'disabled_prefixes' argument is passed in as e.g. disabled_prefixes = > > ['DISABLED_', 'FLAKY_', ...], and when run_disabled is set to true, the > prefixes > > 'DISABLED_' and 'FLAKY_' won't be removed by that way? > > That's a fair point, and I hadn't considered it. That option isn't used (any > more?), though, so I think we can ignore that case. OK that make sense. Will refactor it with appending way in next patch. https://codereview.chromium.org/2544603002/diff/60001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_gtest_run.py (right): https://codereview.chromium.org/2544603002/diff/60001/build/android/pylib/loc... build/android/pylib/local/device/local_device_gtest_run.py:385: flags += ' --gtest_also_run_disabled_tests' I think this line is somehow necessary. When run gtest with '--gtest_also_run_disabled_tests', all the 'DISABLED_' tests would come out as FAILED if without this line. But those tests would pass if this line exists. E.g. $build/android/test_runner.py gtest -s ipc_tests --gtest_also_run_disabled_tests Result: W 0.024s Main No data dependencies will be pushed. C 19.335s Main ******************************************************************************** C 19.335s Main Detailed Logs C 19.335s Main ******************************************************************************** C 19.336s Main ******************************************************************************** C 19.336s Main Summary C 19.336s Main ******************************************************************************** C 19.337s Main [==========] 81 tests ran. C 19.337s Main [ PASSED ] 72 tests. C 19.337s Main [ FAILED ] 9 tests, listed below: C 19.337s Main [ FAILED ] IPCMessageIntegrity.DISABLED_ReadVectorTooLarge1 (UNKNOWN) C 19.337s Main [ FAILED ] IPCMessageParameterTest.DISABLED_OneIntegerWithParam (UNKNOWN) C 19.337s Main [ FAILED ] IPCSyncChannelTest.DISABLED_ChannelDeleteDuringSend (UNKNOWN) C 19.337s Main [ FAILED ] IPCSyncChannelTest.DISABLED_ChattyServer (UNKNOWN) C 19.337s Main [ FAILED ] IPCSyncChannelTest.DISABLED_ChattyServerPumpDuringSend (UNKNOWN) C 19.337s Main [ FAILED ] IPCSyncChannelTest.DISABLED_DoneEventRace (UNKNOWN) C 19.337s Main [ FAILED ] IPCSyncChannelTest.DISABLED_RestrictedDispatch4WayDeadlock (UNKNOWN) C 19.337s Main [ FAILED ] IPCSyncChannelTest.DISABLED_Simple (UNKNOWN) C 19.337s Main [ FAILED ] MessageAttachmentSet.DISABLED_DontClose (UNKNOWN) C 19.338s Main C 19.338s Main 9 FAILED TESTS C 19.338s Main ********************************************************************************
https://codereview.chromium.org/2544603002/diff/60001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_gtest_run.py (right): https://codereview.chromium.org/2544603002/diff/60001/build/android/pylib/loc... build/android/pylib/local/device/local_device_gtest_run.py:385: flags += ' --gtest_also_run_disabled_tests' On 2016/12/01 00:39:56, shenghuazhang wrote: > I think this line is somehow necessary. When run gtest with > '--gtest_also_run_disabled_tests', all the 'DISABLED_' tests would come out as > FAILED if without this line. But those tests would pass if this line exists. > > E.g. $build/android/test_runner.py gtest -s ipc_tests > --gtest_also_run_disabled_tests > > Result: > > W 0.024s Main No data dependencies will be pushed. > C 19.335s Main > ******************************************************************************** > C 19.335s Main Detailed Logs > C 19.335s Main > ******************************************************************************** > C 19.336s Main > ******************************************************************************** > C 19.336s Main Summary > C 19.336s Main > ******************************************************************************** > C 19.337s Main [==========] 81 tests ran. > C 19.337s Main [ PASSED ] 72 tests. > C 19.337s Main [ FAILED ] 9 tests, listed below: > C 19.337s Main [ FAILED ] IPCMessageIntegrity.DISABLED_ReadVectorTooLarge1 > (UNKNOWN) > C 19.337s Main [ FAILED ] > IPCMessageParameterTest.DISABLED_OneIntegerWithParam (UNKNOWN) > C 19.337s Main [ FAILED ] > IPCSyncChannelTest.DISABLED_ChannelDeleteDuringSend (UNKNOWN) > C 19.337s Main [ FAILED ] IPCSyncChannelTest.DISABLED_ChattyServer > (UNKNOWN) > C 19.337s Main [ FAILED ] > IPCSyncChannelTest.DISABLED_ChattyServerPumpDuringSend (UNKNOWN) > C 19.337s Main [ FAILED ] IPCSyncChannelTest.DISABLED_DoneEventRace > (UNKNOWN) > C 19.337s Main [ FAILED ] > IPCSyncChannelTest.DISABLED_RestrictedDispatch4WayDeadlock (UNKNOWN) > C 19.337s Main [ FAILED ] IPCSyncChannelTest.DISABLED_Simple (UNKNOWN) > C 19.337s Main [ FAILED ] MessageAttachmentSet.DISABLED_DontClose (UNKNOWN) > C 19.338s Main > C 19.338s Main 9 FAILED TESTS > C 19.338s Main > ******************************************************************************** My point was just that we shouldn't add it all the time -- we ought to only add it if gtest_also_run_disabled_tests is set.
Fixed the condition for disable flag. https://codereview.chromium.org/2544603002/diff/60001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_gtest_run.py (right): https://codereview.chromium.org/2544603002/diff/60001/build/android/pylib/loc... build/android/pylib/local/device/local_device_gtest_run.py:385: flags += ' --gtest_also_run_disabled_tests' On 2016/12/01 00:44:48, jbudorick wrote: > On 2016/12/01 00:39:56, shenghuazhang wrote: > > I think this line is somehow necessary. When run gtest with > > '--gtest_also_run_disabled_tests', all the 'DISABLED_' tests would come out as > > FAILED if without this line. But those tests would pass if this line exists. > > > > E.g. $build/android/test_runner.py gtest -s ipc_tests > > --gtest_also_run_disabled_tests > > > > Result: > > > > W 0.024s Main No data dependencies will be pushed. > > C 19.335s Main > > > ******************************************************************************** > > C 19.335s Main Detailed Logs > > C 19.335s Main > > > ******************************************************************************** > > C 19.336s Main > > > ******************************************************************************** > > C 19.336s Main Summary > > C 19.336s Main > > > ******************************************************************************** > > C 19.337s Main [==========] 81 tests ran. > > C 19.337s Main [ PASSED ] 72 tests. > > C 19.337s Main [ FAILED ] 9 tests, listed below: > > C 19.337s Main [ FAILED ] > IPCMessageIntegrity.DISABLED_ReadVectorTooLarge1 > > (UNKNOWN) > > C 19.337s Main [ FAILED ] > > IPCMessageParameterTest.DISABLED_OneIntegerWithParam (UNKNOWN) > > C 19.337s Main [ FAILED ] > > IPCSyncChannelTest.DISABLED_ChannelDeleteDuringSend (UNKNOWN) > > C 19.337s Main [ FAILED ] IPCSyncChannelTest.DISABLED_ChattyServer > > (UNKNOWN) > > C 19.337s Main [ FAILED ] > > IPCSyncChannelTest.DISABLED_ChattyServerPumpDuringSend (UNKNOWN) > > C 19.337s Main [ FAILED ] IPCSyncChannelTest.DISABLED_DoneEventRace > > (UNKNOWN) > > C 19.337s Main [ FAILED ] > > IPCSyncChannelTest.DISABLED_RestrictedDispatch4WayDeadlock (UNKNOWN) > > C 19.337s Main [ FAILED ] IPCSyncChannelTest.DISABLED_Simple (UNKNOWN) > > C 19.337s Main [ FAILED ] MessageAttachmentSet.DISABLED_DontClose > (UNKNOWN) > > C 19.338s Main > > C 19.338s Main 9 FAILED TESTS > > C 19.338s Main > > > ******************************************************************************** > > My point was just that we shouldn't add it all the time -- we ought to only add > it if gtest_also_run_disabled_tests is set. KK I see :p
lgtm
The CQ bit was checked by shenghuazhang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1480553988333110,
"parent_rev": "a69ee8af0e8e6e65153259657d61899f85c2c623", "commit_rev":
"e1e2e582d3fce9faa84b8debacf173b2f63085ba"}
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [Android] Add '--gtest_also_run_disabled_tests' logic in gtest runner Command line option '--gtest_also_run_disabled_tests' already exists in test_runner.py, should build the logic in local_device_gtest_run.py in order to run disabled tests. This CL also unify '--gtest_repeat' for instrumentation test options. BUG=669632 ========== to ========== [Android] Add '--gtest_also_run_disabled_tests' logic in gtest runner Command line option '--gtest_also_run_disabled_tests' already exists in test_runner.py, should build the logic in local_device_gtest_run.py in order to run disabled tests. This CL also unify '--gtest_repeat' for instrumentation test options. BUG=669632 Committed: https://crrev.com/b58f48f40ef901780540975d4b5f4121e229277c Cr-Commit-Position: refs/heads/master@{#435524} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/b58f48f40ef901780540975d4b5f4121e229277c Cr-Commit-Position: refs/heads/master@{#435524} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
