Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2735373002:
Fix writing GPU test runner arguments to results file (Closed)
|
Created:
3 years, 9 months ago by oetuaho-nv Modified:
3 years, 9 months ago Reviewers:
Ken Russell (switch to Gerrit) CC:
chromium-reviews, jam, darin-cc_chromium.org, piman+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix writing GPU test runner arguments to results file
The command line arguments for the script changed when it was ported
to use typ. Clean up code that was written for the old runner and fix
writing test run arguments to the results json file. An extra argument
is now required to toggle writing the test run arguments.
BUG=690535
TEST=run_gpu_integration_test
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Review-Url: https://codereview.chromium.org/2735373002
Cr-Commit-Position: refs/heads/master@{#457320}
Committed: https://chromium.googlesource.com/chromium/src/+/8db424fb7191b2be7b58b89427b9b258d057d97b
Patch Set 1 #
Total comments: 1
Patch Set 2 : Added an extra argument to toggle writing arguments #
Total comments: 4
Patch Set 3 : Clean up argument filtering #Messages
Total messages: 19 (10 generated)
Description was changed from ========== Fix writing GPU test runner arguments to results file The command line arguments for the script changed when it was ported to use typ. Clean up code that was written for the old runner and fix writing test run arguments to the results json file. BUG=690535 TEST=run_gpu_integration_test ========== to ========== Fix writing GPU test runner arguments to results file The command line arguments for the script changed when it was ported to use typ. Clean up code that was written for the old runner and fix writing test run arguments to the results json file. BUG=690535 TEST=run_gpu_integration_test CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
oetuaho@nvidia.com changed reviewers: + kbr@chromium.org
New version of my earlier patch. PTAL!
Thanks Olli for putting this together. LGTM if it doesn't break anything. Let's see what the commit queue says. Triggering a dry run.
The CQ bit was checked by kbr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2735373002/diff/1/content/test/gpu/run_gpu_in... File content/test/gpu/run_gpu_integration_test.py (right): https://codereview.chromium.org/2735373002/diff/1/content/test/gpu/run_gpu_in... content/test/gpu/run_gpu_integration_test.py:24: test_result['run_test_args'] = run_test_args Sorry, I should have guessed that this would not work. Here's the stack trace: Traceback (most recent call last): File "/b/rr/tmp6Su5Uh/rw/checkout/scripts/slave/recipe_modules/swarming/resources/standard_isolated_script_merge.py", line 44, in <module> sys.exit(main()) File "/b/rr/tmp6Su5Uh/rw/checkout/scripts/slave/recipe_modules/swarming/resources/standard_isolated_script_merge.py", line 40, in main return StandardIsolatedScriptMerge(args.output_json, args.jsons_to_merge) File "/b/rr/tmp6Su5Uh/rw/checkout/scripts/slave/recipe_modules/swarming/resources/standard_isolated_script_merge.py", line 25, in StandardIsolatedScriptMerge merged_results = results_merger.merge_test_results(shard_results_list) File "/b/rr/tmp6Su5Uh/rw/checkout/scripts/slave/recipe_modules/swarming/resources/results_merger.py", line 65, in merge_test_results return _merge_json_test_result_format(shard_results_list) File "/b/rr/tmp6Su5Uh/rw/checkout/scripts/slave/recipe_modules/swarming/resources/results_merger.py", line 178, in _merge_json_test_result_format 'Unmergable values %s' % result_json.keys()) results_merger.MergeException: Unmergable values [u'run_test_args'] I think that you should add a new command line argument that you specify only on your machines internally. The test results format is tightly specified at https://www.chromium.org/developers/the-json-test-results-format and I don't think Chromium's bots can easily be changed to add this field.
Description was changed from ========== Fix writing GPU test runner arguments to results file The command line arguments for the script changed when it was ported to use typ. Clean up code that was written for the old runner and fix writing test run arguments to the results json file. BUG=690535 TEST=run_gpu_integration_test CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Fix writing GPU test runner arguments to results file The command line arguments for the script changed when it was ported to use typ. Clean up code that was written for the old runner and fix writing test run arguments to the results json file. An extra argument is now required to toggle writing the test run arguments. BUG=690535 TEST=run_gpu_integration_test CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Patch set #2 should take care of the issue.
https://codereview.chromium.org/2735373002/diff/20001/content/test/gpu/run_gp... File content/test/gpu/run_gpu_integration_test.py (right): https://codereview.chromium.org/2735373002/diff/20001/content/test/gpu/run_gp... content/test/gpu/run_gpu_integration_test.py:45: rest_args_filtered = rest_args This is redundant. Just assign: option, rest_args_filtered = parser.parse_known_args(rest_args) https://codereview.chromium.org/2735373002/diff/20001/content/test/gpu/run_gp... content/test/gpu/run_gpu_integration_test.py:63: option, _ = parser.parse_known_args(rest_args) I really wish it weren't necessary to do this second argument parse because it makes the code more confusing but I can't think of a better way to do it right now.
Addressed review comments. https://codereview.chromium.org/2735373002/diff/20001/content/test/gpu/run_gp... File content/test/gpu/run_gpu_integration_test.py (right): https://codereview.chromium.org/2735373002/diff/20001/content/test/gpu/run_gp... content/test/gpu/run_gpu_integration_test.py:45: rest_args_filtered = rest_args On 2017/03/15 04:25:29, Ken Russell wrote: > This is redundant. Just assign: > option, rest_args_filtered = parser.parse_known_args(rest_args) Done. https://codereview.chromium.org/2735373002/diff/20001/content/test/gpu/run_gp... content/test/gpu/run_gpu_integration_test.py:63: option, _ = parser.parse_known_args(rest_args) On 2017/03/15 04:25:29, Ken Russell wrote: > I really wish it weren't necessary to do this second argument parse because it > makes the code more confusing but I can't think of a better way to do it right > now. Yep, it's ugly but I couldn't think of a better way to do it either.
LGTM again. Let me CQ this for you.
The CQ bit was checked by kbr@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": 40001, "attempt_start_ts": 1489626479991300,
"parent_rev": "17f4b3fb7897a423a8fb90068b37a7c92f6f624f", "commit_rev":
"8db424fb7191b2be7b58b89427b9b258d057d97b"}
Message was sent while issue was closed.
Description was changed from ========== Fix writing GPU test runner arguments to results file The command line arguments for the script changed when it was ported to use typ. Clean up code that was written for the old runner and fix writing test run arguments to the results json file. An extra argument is now required to toggle writing the test run arguments. BUG=690535 TEST=run_gpu_integration_test CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Fix writing GPU test runner arguments to results file The command line arguments for the script changed when it was ported to use typ. Clean up code that was written for the old runner and fix writing test run arguments to the results json file. An extra argument is now required to toggle writing the test run arguments. BUG=690535 TEST=run_gpu_integration_test CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2735373002 Cr-Commit-Position: refs/heads/master@{#457320} Committed: https://chromium.googlesource.com/chromium/src/+/8db424fb7191b2be7b58b89427b9... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/8db424fb7191b2be7b58b89427b9... |
