Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(783)

Issue 2375663003: Add json test results format support for SwarmingIsolatedScriptTest (Closed)

Created:
4 years, 2 months ago by nednguyen
Modified:
4 years, 2 months ago
CC:
chromium-reviews, infra-reviews+build_chromium.org, kjellander-cc_chromium.org, Dirk Pranke, martiniss
Target Ref:
refs/heads/master
Project:
build
Visibility:
Public.

Description

Add json test results format support for SwarmingIsolatedScriptTest BUG=649762 Committed: https://chromium.googlesource.com/chromium/tools/build/+/7baad31fc4293485a824d4ef6ef7361adc2979a1

Patch Set 1 #

Total comments: 22

Patch Set 2 : Move merging logic to a separate file for unittesting #

Patch Set 3 : Address review comments #

Total comments: 1

Patch Set 4 : Fix typos #

Total comments: 9

Patch Set 5 : Add expectation test coverage #

Patch Set 6 : Undo collect_gtest_task_test change #

Patch Set 7 : Update SwarmingIsolatedScriptTest #

Total comments: 2

Patch Set 8 : Add missing comments #

Patch Set 9 : Rebase #

Total comments: 24

Patch Set 10 : Address Sergiy's comments #

Patch Set 11 : Rebase #

Patch Set 12 : Address Dirk's comment #

Total comments: 2

Patch Set 13 : Address Sergiy comments #

Total comments: 8

Patch Set 14 : Update failed test generation #

Patch Set 15 : Use repr(e) to make the invalid_results_exc clearer #

Patch Set 16 : Move results_merger out of resources/ & remove resources/__init__.py #

Patch Set 17 : Add no cover #

Unified diffs Side-by-side diffs Delta from patch set Stats (+523 lines, -78 lines) Patch
M scripts/slave/recipe_modules/chromium_tests/steps.py View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +22 lines, -8 lines 0 comments Download
M scripts/slave/recipe_modules/swarming/api.py View 1 2 3 4 10 11 12 13 14 15 4 chunks +4 lines, -28 lines 0 comments Download
A scripts/slave/recipe_modules/swarming/results_merger.py View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +111 lines, -0 lines 0 comments Download
A scripts/slave/recipe_modules/swarming/tests/results_merger_unittest.py View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +234 lines, -0 lines 0 comments Download
M scripts/slave/recipe_modules/test_utils/api.py View 1 2 3 4 5 6 1 chunk +3 lines, -0 lines 0 comments Download
M scripts/slave/recipe_modules/test_utils/test_api.py View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +81 lines, -11 lines 0 comments Download
M scripts/slave/recipes/chromium.py View 1 2 3 4 5 6 7 8 9 11 12 2 chunks +37 lines, -3 lines 0 comments Download
M scripts/slave/recipes/chromium.expected/dynamic_swarmed_passed_isolated_script_test_with_swarming_failure.json View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +1 line, -1 line 0 comments Download
M scripts/slave/recipes/chromium.expected/dynamic_swarmed_sharded_failed_isolated_script_test.json View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +3 lines, -3 lines 0 comments Download
A + scripts/slave/recipes/chromium.expected/dynamic_swarmed_sharded_invalid_format_isolated_script_test.json View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 5 chunks +21 lines, -19 lines 0 comments Download
M scripts/slave/recipes/chromium.expected/dynamic_swarmed_sharded_isolated_chartjson_test_harness_failure.json View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +1 line, -1 line 0 comments Download
M scripts/slave/recipes/chromium.expected/dynamic_swarmed_sharded_isolated_script_test_harness_failure.json View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +1 line, -1 line 0 comments Download
M scripts/slave/recipes/chromium.expected/dynamic_swarmed_sharded_isolated_script_test_missing_shard.json View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +1 line, -1 line 0 comments Download
M scripts/slave/recipes/chromium.expected/dynamic_swarmed_sharded_passed_isolated_script_test.json View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +2 lines, -2 lines 0 comments Download
M scripts/slave/unittests/recipe_lint_test.py View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 81 (36 generated)
nednguyen
This is not ready for full review yet, but I just want to have some ...
4 years, 2 months ago (2016-09-29 14:18:42 UTC) #13
eyaich1
You are also going to have to update this contract in other places: 1) chromium_tests/steps.py ...
4 years, 2 months ago (2016-09-29 14:55:23 UTC) #15
estaab
On 2016/09/29 at 14:18:42, nednguyen wrote: > This is not ready for full review yet, ...
4 years, 2 months ago (2016-09-29 17:36:15 UTC) #16
Sergiy Byelozyorov
Yay! Thank you for this CL! Left a few comments. https://codereview.chromium.org/2375663003/diff/40001/scripts/slave/recipe_modules/swarming/api.py File scripts/slave/recipe_modules/swarming/api.py (right): https://codereview.chromium.org/2375663003/diff/40001/scripts/slave/recipe_modules/swarming/api.py#newcode809 ...
4 years, 2 months ago (2016-09-29 21:36:22 UTC) #18
Ken Russell (switch to Gerrit)
Thanks for generalizing this code. A couple of comments so far. https://codereview.chromium.org/2375663003/diff/40001/scripts/slave/recipe_modules/swarming/api.py File scripts/slave/recipe_modules/swarming/api.py (right): ...
4 years, 2 months ago (2016-09-30 23:37:53 UTC) #19
nednguyen
https://codereview.chromium.org/2375663003/diff/40001/scripts/slave/recipe_modules/swarming/api.py File scripts/slave/recipe_modules/swarming/api.py (right): https://codereview.chromium.org/2375663003/diff/40001/scripts/slave/recipe_modules/swarming/api.py#newcode809 scripts/slave/recipe_modules/swarming/api.py:809: raise recipe_api.InfraFailure('Invalid json test results') On 2016/09/29 21:36:22, Sergiy ...
4 years, 2 months ago (2016-10-04 00:10:11 UTC) #20
nednguyen
https://codereview.chromium.org/2375663003/diff/80001/scripts/slave/recipe_modules/swarming/__init__.py File scripts/slave/recipe_modules/swarming/__init__.py (right): https://codereview.chromium.org/2375663003/diff/80001/scripts/slave/recipe_modules/swarming/__init__.py#newcode18 scripts/slave/recipe_modules/swarming/__init__.py:18: try: Reviewers: I need to guard this so that ...
4 years, 2 months ago (2016-10-04 00:14:59 UTC) #21
Ken Russell (switch to Gerrit)
CC'ing Vadim in particular regarding the unittest and the try/catch.
4 years, 2 months ago (2016-10-04 00:49:29 UTC) #23
Vadim Sh.
+ also Robbie, who's working on improving recipes unit tests in general https://codereview.chromium.org/2375663003/diff/100001/scripts/slave/recipe_modules/swarming/api.py File scripts/slave/recipe_modules/swarming/api.py ...
4 years, 2 months ago (2016-10-04 01:05:29 UTC) #25
Vadim Sh.
https://codereview.chromium.org/2375663003/diff/100001/scripts/slave/recipe_modules/swarming/api.py File scripts/slave/recipe_modules/swarming/api.py (right): https://codereview.chromium.org/2375663003/diff/100001/scripts/slave/recipe_modules/swarming/api.py#newcode787 scripts/slave/recipe_modules/swarming/api.py:787: return results_merger.merge_test_results(shard_results_list) On 2016/10/04 01:05:29, Vadim Sh. wrote: > ...
4 years, 2 months ago (2016-10-04 01:11:32 UTC) #26
iannucci
Can use leak_to feature of json.output to avoid round trip. On Mon, Oct 3, 2016, ...
4 years, 2 months ago (2016-10-04 01:13:23 UTC) #27
Ken Russell (switch to Gerrit)
It should certainly be made possible to unit test the merger functions, but I'm not ...
4 years, 2 months ago (2016-10-04 01:14:29 UTC) #28
iannucci
Yes and post process assertions should land this week. I'll take a look at the ...
4 years, 2 months ago (2016-10-04 01:20:43 UTC) #29
nednguyen
On 2016/10/04 01:14:29, Ken Russell wrote: > It should certainly be made possible to unit ...
4 years, 2 months ago (2016-10-04 01:23:27 UTC) #30
Ken Russell (switch to Gerrit)
On 2016/10/04 01:23:27, nednguyen wrote: > On 2016/10/04 01:14:29, Ken Russell wrote: > > It ...
4 years, 2 months ago (2016-10-04 01:43:36 UTC) #31
Ken Russell (switch to Gerrit)
https://codereview.chromium.org/2375663003/diff/100001/scripts/slave/recipe_modules/swarming/__init__.py File scripts/slave/recipe_modules/swarming/__init__.py (right): https://codereview.chromium.org/2375663003/diff/100001/scripts/slave/recipe_modules/swarming/__init__.py#newcode18 scripts/slave/recipe_modules/swarming/__init__.py:18: try: Add a comment that this try/catch enables the ...
4 years, 2 months ago (2016-10-04 01:43:44 UTC) #32
nednguyen
https://codereview.chromium.org/2375663003/diff/100001/scripts/slave/recipe_modules/swarming/__init__.py File scripts/slave/recipe_modules/swarming/__init__.py (right): https://codereview.chromium.org/2375663003/diff/100001/scripts/slave/recipe_modules/swarming/__init__.py#newcode18 scripts/slave/recipe_modules/swarming/__init__.py:18: try: On 2016/10/04 01:43:43, Ken Russell wrote: > Add ...
4 years, 2 months ago (2016-10-04 01:49:52 UTC) #33
Sergiy Byelozyorov
https://codereview.chromium.org/2375663003/diff/40001/scripts/slave/recipe_modules/swarming/api.py File scripts/slave/recipe_modules/swarming/api.py (right): https://codereview.chromium.org/2375663003/diff/40001/scripts/slave/recipe_modules/swarming/api.py#newcode809 scripts/slave/recipe_modules/swarming/api.py:809: raise recipe_api.InfraFailure('Invalid json test results') On 2016/10/04 00:10:11, nednguyen ...
4 years, 2 months ago (2016-10-04 08:54:52 UTC) #34
nednguyen
https://codereview.chromium.org/2375663003/diff/40001/scripts/slave/recipe_modules/swarming/api.py File scripts/slave/recipe_modules/swarming/api.py (right): https://codereview.chromium.org/2375663003/diff/40001/scripts/slave/recipe_modules/swarming/api.py#newcode839 scripts/slave/recipe_modules/swarming/api.py:839: results_json['seconds_since_epoch']) On 2016/10/04 08:54:52, Sergiy Byelozyorov wrote: > On ...
4 years, 2 months ago (2016-10-04 13:30:29 UTC) #35
nednguyen
On 2016/10/04 13:30:29, nednguyen wrote: > https://codereview.chromium.org/2375663003/diff/40001/scripts/slave/recipe_modules/swarming/api.py > File scripts/slave/recipe_modules/swarming/api.py (right): > > https://codereview.chromium.org/2375663003/diff/40001/scripts/slave/recipe_modules/swarming/api.py#newcode839 > ...
4 years, 2 months ago (2016-10-04 13:34:14 UTC) #36
nednguyen
On 2016/10/04 13:34:14, nednguyen wrote: > On 2016/10/04 13:30:29, nednguyen wrote: > > > https://codereview.chromium.org/2375663003/diff/40001/scripts/slave/recipe_modules/swarming/api.py ...
4 years, 2 months ago (2016-10-04 17:46:17 UTC) #37
nednguyen
On 2016/10/04 17:46:17, nednguyen wrote: > On 2016/10/04 13:34:14, nednguyen wrote: > > On 2016/10/04 ...
4 years, 2 months ago (2016-10-04 18:32:46 UTC) #46
Dirk Pranke
https://codereview.chromium.org/2375663003/diff/40001/scripts/slave/recipe_modules/swarming/api.py File scripts/slave/recipe_modules/swarming/api.py (right): https://codereview.chromium.org/2375663003/diff/40001/scripts/slave/recipe_modules/swarming/api.py#newcode809 scripts/slave/recipe_modules/swarming/api.py:809: raise recipe_api.InfraFailure('Invalid json test results') On 2016/10/04 08:54:52, Sergiy ...
4 years, 2 months ago (2016-10-04 20:04:03 UTC) #48
nednguyen
On 2016/10/04 20:04:03, Dirk Pranke (slow) wrote: > https://codereview.chromium.org/2375663003/diff/40001/scripts/slave/recipe_modules/swarming/api.py > File scripts/slave/recipe_modules/swarming/api.py (right): > > ...
4 years, 2 months ago (2016-10-04 21:08:46 UTC) #49
Ken Russell (switch to Gerrit)
Looks excellent to me. LGTM https://codereview.chromium.org/2375663003/diff/160001/scripts/slave/recipe_modules/swarming/results_merger.py File scripts/slave/recipe_modules/swarming/results_merger.py (right): https://codereview.chromium.org/2375663003/diff/160001/scripts/slave/recipe_modules/swarming/results_merger.py#newcode18 scripts/slave/recipe_modules/swarming/results_merger.py:18: a Please finish this ...
4 years, 2 months ago (2016-10-05 00:56:28 UTC) #51
nednguyen
https://codereview.chromium.org/2375663003/diff/160001/scripts/slave/recipe_modules/swarming/results_merger.py File scripts/slave/recipe_modules/swarming/results_merger.py (right): https://codereview.chromium.org/2375663003/diff/160001/scripts/slave/recipe_modules/swarming/results_merger.py#newcode18 scripts/slave/recipe_modules/swarming/results_merger.py:18: a On 2016/10/05 00:56:28, Ken Russell wrote: > Please ...
4 years, 2 months ago (2016-10-05 13:14:37 UTC) #52
nednguyen
On 2016/10/05 13:14:37, nednguyen wrote: > https://codereview.chromium.org/2375663003/diff/160001/scripts/slave/recipe_modules/swarming/results_merger.py > File scripts/slave/recipe_modules/swarming/results_merger.py (right): > > https://codereview.chromium.org/2375663003/diff/160001/scripts/slave/recipe_modules/swarming/results_merger.py#newcode18 > ...
4 years, 2 months ago (2016-10-05 14:55:40 UTC) #53
Sergiy Byelozyorov
https://codereview.chromium.org/2375663003/diff/200001/scripts/slave/recipe_modules/chromium_tests/steps.py File scripts/slave/recipe_modules/chromium_tests/steps.py (right): https://codereview.chromium.org/2375663003/diff/200001/scripts/slave/recipe_modules/chromium_tests/steps.py#newcode1085 scripts/slave/recipe_modules/chromium_tests/steps.py:1085: valid = results['valid'] Please remove two lines above... they ...
4 years, 2 months ago (2016-10-05 15:36:07 UTC) #54
nednguyen
https://codereview.chromium.org/2375663003/diff/200001/scripts/slave/recipe_modules/chromium_tests/steps.py File scripts/slave/recipe_modules/chromium_tests/steps.py (right): https://codereview.chromium.org/2375663003/diff/200001/scripts/slave/recipe_modules/chromium_tests/steps.py#newcode1085 scripts/slave/recipe_modules/chromium_tests/steps.py:1085: valid = results['valid'] On 2016/10/05 15:36:06, Sergiy Byelozyorov wrote: ...
4 years, 2 months ago (2016-10-05 18:50:53 UTC) #55
Dirk Pranke
https://codereview.chromium.org/2375663003/diff/200001/scripts/slave/recipe_modules/chromium_tests/steps.py File scripts/slave/recipe_modules/chromium_tests/steps.py (right): https://codereview.chromium.org/2375663003/diff/200001/scripts/slave/recipe_modules/chromium_tests/steps.py#newcode1093 scripts/slave/recipe_modules/chromium_tests/steps.py:1093: valid = results['num_failures_by_type'].get('FAIL', 0) == len(failures) On 2016/10/05 18:50:53, ...
4 years, 2 months ago (2016-10-05 19:39:46 UTC) #56
nednguyen
https://codereview.chromium.org/2375663003/diff/200001/scripts/slave/recipe_modules/chromium_tests/steps.py File scripts/slave/recipe_modules/chromium_tests/steps.py (right): https://codereview.chromium.org/2375663003/diff/200001/scripts/slave/recipe_modules/chromium_tests/steps.py#newcode1093 scripts/slave/recipe_modules/chromium_tests/steps.py:1093: valid = results['num_failures_by_type'].get('FAIL', 0) == len(failures) On 2016/10/05 19:39:46, ...
4 years, 2 months ago (2016-10-05 20:14:29 UTC) #57
nednguyen
On 2016/10/05 20:14:29, nednguyen wrote: > https://codereview.chromium.org/2375663003/diff/200001/scripts/slave/recipe_modules/chromium_tests/steps.py > File scripts/slave/recipe_modules/chromium_tests/steps.py (right): > > https://codereview.chromium.org/2375663003/diff/200001/scripts/slave/recipe_modules/chromium_tests/steps.py#newcode1093 > ...
4 years, 2 months ago (2016-10-06 22:41:29 UTC) #58
Sergiy Byelozyorov
lgtm w/ comments https://codereview.chromium.org/2375663003/diff/200001/scripts/slave/recipe_modules/chromium_tests/steps.py File scripts/slave/recipe_modules/chromium_tests/steps.py (right): https://codereview.chromium.org/2375663003/diff/200001/scripts/slave/recipe_modules/chromium_tests/steps.py#newcode1092 scripts/slave/recipe_modules/chromium_tests/steps.py:1092: tests[t]['expected'] != tests[t]['actual']) On 2016/10/05 18:50:53, ...
4 years, 2 months ago (2016-10-09 14:43:10 UTC) #59
nednguyen
https://codereview.chromium.org/2375663003/diff/200001/scripts/slave/recipe_modules/chromium_tests/steps.py File scripts/slave/recipe_modules/chromium_tests/steps.py (right): https://codereview.chromium.org/2375663003/diff/200001/scripts/slave/recipe_modules/chromium_tests/steps.py#newcode1092 scripts/slave/recipe_modules/chromium_tests/steps.py:1092: tests[t]['expected'] != tests[t]['actual']) On 2016/10/09 14:43:10, Sergiy Byelozyorov wrote: ...
4 years, 2 months ago (2016-10-10 13:24:52 UTC) #60
eyaich1
https://codereview.chromium.org/2375663003/diff/280001/scripts/slave/recipe_modules/swarming/api.py File scripts/slave/recipe_modules/swarming/api.py (right): https://codereview.chromium.org/2375663003/diff/280001/scripts/slave/recipe_modules/swarming/api.py#newcode11 scripts/slave/recipe_modules/swarming/api.py:11: from resources import results_merger Are you sure this is ...
4 years, 2 months ago (2016-10-10 13:51:28 UTC) #61
nednguyen
https://codereview.chromium.org/2375663003/diff/280001/scripts/slave/recipe_modules/swarming/api.py File scripts/slave/recipe_modules/swarming/api.py (right): https://codereview.chromium.org/2375663003/diff/280001/scripts/slave/recipe_modules/swarming/api.py#newcode11 scripts/slave/recipe_modules/swarming/api.py:11: from resources import results_merger On 2016/10/10 13:51:28, eyaich1 wrote: ...
4 years, 2 months ago (2016-10-10 14:26:26 UTC) #62
nednguyen
PTAL again Emily https://codereview.chromium.org/2375663003/diff/280001/scripts/slave/recipes/chromium.expected/dynamic_swarmed_sharded_invalid_format_isolated_script_test.json File scripts/slave/recipes/chromium.expected/dynamic_swarmed_sharded_invalid_format_isolated_script_test.json (right): https://codereview.chromium.org/2375663003/diff/280001/scripts/slave/recipes/chromium.expected/dynamic_swarmed_sharded_invalid_format_isolated_script_test.json#newcode452 scripts/slave/recipes/chromium.expected/dynamic_swarmed_sharded_invalid_format_isolated_script_test.json:452: "status_code": 0 On 2016/10/10 13:51:28, eyaich1 ...
4 years, 2 months ago (2016-10-10 14:42:29 UTC) #63
eyaich1
On 2016/10/10 14:42:29, nednguyen wrote: > PTAL again Emily > > https://codereview.chromium.org/2375663003/diff/280001/scripts/slave/recipes/chromium.expected/dynamic_swarmed_sharded_invalid_format_isolated_script_test.json > File > ...
4 years, 2 months ago (2016-10-10 14:43:34 UTC) #64
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2375663003/320001
4 years, 2 months ago (2016-10-10 14:44:35 UTC) #67
commit-bot: I haz the power
Try jobs failed on following builders: Build Presubmit on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/31c7e5c41b570610)
4 years, 2 months ago (2016-10-10 14:56:04 UTC) #69
nednguyen
On 2016/10/10 14:56:04, commit-bot: I haz the power wrote: > Try jobs failed on following ...
4 years, 2 months ago (2016-10-10 15:14:51 UTC) #70
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2375663003/340001
4 years, 2 months ago (2016-10-10 15:15:01 UTC) #73
commit-bot: I haz the power
Try jobs failed on following builders: Build Presubmit on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/31c7fafd6da82f10)
4 years, 2 months ago (2016-10-10 15:28:57 UTC) #75
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2375663003/360001
4 years, 2 months ago (2016-10-10 15:35:01 UTC) #78
commit-bot: I haz the power
4 years, 2 months ago (2016-10-10 15:42:42 UTC) #80
Message was sent while issue was closed.
Committed patchset #17 (id:360001) as
https://chromium.googlesource.com/chromium/tools/build/+/7baad31fc4293485a824...

Powered by Google App Engine
This is Rietveld 408576698