Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2410613002:
Change SwarmingIsolatedScriptTest to upload json format results (Closed)
DescriptionChange SwarmingIsolatedScriptTest to upload json format results
BUG=649762
Committed: https://chromium.googlesource.com/chromium/tools/build/+/e21ee362bd0357572c633342ccd9dac6c49ff86c
Patch Set 1 #
Total comments: 4
Patch Set 2 : Rebase #Patch Set 3 : Rebase #
Messages
Total messages: 23 (9 generated)
Description was changed from ========== Change SwarmingIsolatedScriptTest to upload json format results BUG=649762 ========== to ========== Change SwarmingIsolatedScriptTest to upload json format results To reuse the logic of uploading json results format, this first requires updating & renaming upload_gtest_test_results.py to upload_test_results.py so that it support both gtest formart & json results format. BUG=649762 ==========
nednguyen@google.com changed reviewers: + dpranke@chromium.org, kbr@chromium.org, sergiyb@chromium.org
Description was changed from ========== Change SwarmingIsolatedScriptTest to upload json format results To reuse the logic of uploading json results format, this first requires updating & renaming upload_gtest_test_results.py to upload_test_results.py so that it support both gtest formart & json results format. BUG=649762 ========== to ========== Change SwarmingIsolatedScriptTest to upload json format results To reuse the logic of uploading json results format, this first requires updating & renaming upload_gtest_test_results.py to upload_test_results.py so that it support both gtest formart & json results format. The main changes are in: scripts/slave/recipe_modules/chromium_tests/steps.py scripts/slave/recipe_modules/test_results/resources/upload_test_results.py BUG=649762 ==========
PTAL on the overall direction. To make this easier for reviewing, I can split this into 2 CLs: 1) Renaming upload_gtest_test_results.py to upload_test_results.py 2) Add logic for uploading full json results directly & call it in SwarmingIsolatedScriptTest Thanks
kbr@chromium.org changed reviewers: + phajdan.jr@chromium.org
Pawel may be a good overall reviewer for this change; adding him to the reviewer list.
Description was changed from ========== Change SwarmingIsolatedScriptTest to upload json format results To reuse the logic of uploading json results format, this first requires updating & renaming upload_gtest_test_results.py to upload_test_results.py so that it support both gtest formart & json results format. The main changes are in: scripts/slave/recipe_modules/chromium_tests/steps.py scripts/slave/recipe_modules/test_results/resources/upload_test_results.py BUG=649762 ========== to ========== Change SwarmingIsolatedScriptTest to upload json format results To reuse the logic of uploading json results format, this first requires updating & renaming upload_gtest_test_results.py to upload_test_results.py so that it supports both gtest format & json results format. The main changes are in: scripts/slave/recipe_modules/chromium_tests/steps.py scripts/slave/recipe_modules/test_results/resources/upload_test_results.py BUG=649762 ==========
This looks like a good direction to me. I agree with your assessment that the renaming of the script should be done separately, to minimize the number of recipe expectation changes, and to make one of the CLs a no-op. Any recipe changes are likely to break the overall system, so it's important that they be separable and easily revertible. It'd be fine with me to add the new functionality to the existing script now, and rename it later. Or vice versa; your choice. https://codereview.chromium.org/2410613002/diff/1/scripts/slave/recipe_module... File scripts/slave/recipe_modules/test_results/resources/upload_test_results_unittest.py (right): https://codereview.chromium.org/2410613002/diff/1/scripts/slave/recipe_module... scripts/slave/recipe_modules/test_results/resources/upload_test_results_unittest.py:14: class UploadTestResultsTest(unittest.TestCase): This unit test should be expanded to cover the new code paths in upload_test_results.py.
On 2016/10/11 00:18:10, Ken Russell wrote: > This looks like a good direction to me. > > I agree with your assessment that the renaming of the script should be done > separately, to minimize the number of recipe expectation changes, and to make > one of the CLs a no-op. Any recipe changes are likely to break the overall > system, so it's important that they be separable and easily revertible. > > It'd be fine with me to add the new functionality to the existing script now, > and rename it later. Or vice versa; your choice. > > https://codereview.chromium.org/2410613002/diff/1/scripts/slave/recipe_module... > File > scripts/slave/recipe_modules/test_results/resources/upload_test_results_unittest.py > (right): > > https://codereview.chromium.org/2410613002/diff/1/scripts/slave/recipe_module... > scripts/slave/recipe_modules/test_results/resources/upload_test_results_unittest.py:14: > class UploadTestResultsTest(unittest.TestCase): > This unit test should be expanded to cover the new code paths in > upload_test_results.py. Sergiy: any opinion?
On 2016/10/11 14:06:56, nednguyen wrote: > On 2016/10/11 00:18:10, Ken Russell wrote: > > This looks like a good direction to me. > > > > I agree with your assessment that the renaming of the script should be done > > separately, to minimize the number of recipe expectation changes, and to make > > one of the CLs a no-op. Any recipe changes are likely to break the overall > > system, so it's important that they be separable and easily revertible. > > > > It'd be fine with me to add the new functionality to the existing script now, > > and rename it later. Or vice versa; your choice. > > > > > https://codereview.chromium.org/2410613002/diff/1/scripts/slave/recipe_module... > > File > > > scripts/slave/recipe_modules/test_results/resources/upload_test_results_unittest.py > > (right): > > > > > https://codereview.chromium.org/2410613002/diff/1/scripts/slave/recipe_module... > > > scripts/slave/recipe_modules/test_results/resources/upload_test_results_unittest.py:14: > > class UploadTestResultsTest(unittest.TestCase): > > This unit test should be expanded to cover the new code paths in > > upload_test_results.py. > > Sergiy: any opinion? +1 to tests.
https://codereview.chromium.org/2410613002/diff/1/scripts/slave/recipe_module... File scripts/slave/recipe_modules/chromium_tests/steps.py (right): https://codereview.chromium.org/2410613002/diff/1/scripts/slave/recipe_module... scripts/slave/recipe_modules/chromium_tests/steps.py:1130: # Only upload json format results. Why is it done in validate_task_results? What if this function is called multiple times?
On 2016/10/11 14:39:30, Sergiy Byelozyorov wrote: > https://codereview.chromium.org/2410613002/diff/1/scripts/slave/recipe_module... > File scripts/slave/recipe_modules/chromium_tests/steps.py (right): > > https://codereview.chromium.org/2410613002/diff/1/scripts/slave/recipe_module... > scripts/slave/recipe_modules/chromium_tests/steps.py:1130: # Only upload json > format results. > Why is it done in validate_task_results? What if this function is called > multiple times? +1 to splitting the CL as well.
On 2016/10/11 14:40:49, Sergiy Byelozyorov wrote: > On 2016/10/11 14:39:30, Sergiy Byelozyorov wrote: > > > https://codereview.chromium.org/2410613002/diff/1/scripts/slave/recipe_module... > > File scripts/slave/recipe_modules/chromium_tests/steps.py (right): > > > > > https://codereview.chromium.org/2410613002/diff/1/scripts/slave/recipe_module... > > scripts/slave/recipe_modules/chromium_tests/steps.py:1130: # Only upload json > > format results. > > Why is it done in validate_task_results? What if this function is called > > multiple times? > > +1 to splitting the CL as well. I split the first part of the CL to https://codereview.chromium.org/2411763002/
Thanks, Ken. Also commented on https://codereview.chromium.org/2411763002 . https://codereview.chromium.org/2410613002/diff/1/scripts/slave/recipe_module... File scripts/slave/recipe_modules/chromium_tests/steps.py (right): https://codereview.chromium.org/2410613002/diff/1/scripts/slave/recipe_module... scripts/slave/recipe_modules/chromium_tests/steps.py:1130: # Only upload json format results. On 2016/10/11 at 14:39:29, Sergiy Byelozyorov wrote: > Why is it done in validate_task_results? What if this function is called multiple times? +1 ; post_run would be better place
https://codereview.chromium.org/2410613002/diff/1/scripts/slave/recipe_module... File scripts/slave/recipe_modules/chromium_tests/steps.py (right): https://codereview.chromium.org/2410613002/diff/1/scripts/slave/recipe_module... scripts/slave/recipe_modules/chromium_tests/steps.py:1130: # Only upload json format results. On 2016/10/11 18:04:11, Paweł Hajdan Jr. wrote: > On 2016/10/11 at 14:39:29, Sergiy Byelozyorov wrote: > > Why is it done in validate_task_results? What if this function is called > multiple times? > > +1 ; post_run would be better place Swarming.post_run(..) calls validate_task_results: https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/chromium_... I override post_run to still call the parent class's post_run, but add some finally step to upload the result
LGTM
Description was changed from ========== Change SwarmingIsolatedScriptTest to upload json format results To reuse the logic of uploading json results format, this first requires updating & renaming upload_gtest_test_results.py to upload_test_results.py so that it supports both gtest format & json results format. The main changes are in: scripts/slave/recipe_modules/chromium_tests/steps.py scripts/slave/recipe_modules/test_results/resources/upload_test_results.py BUG=649762 ========== to ========== Change SwarmingIsolatedScriptTest to upload json format results BUG=649762 ==========
On 2016/10/13 19:21:34, Paweł Hajdan Jr. wrote: > LGTM After https://chromium.googlesource.com/chromium/tools/build.git/+/bb373a58224d53e3..., this is now ready to land.
The CQ bit was checked by nednguyen@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from phajdan.jr@chromium.org Link to the patchset: https://codereview.chromium.org/2410613002/#ps40001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Change SwarmingIsolatedScriptTest to upload json format results BUG=649762 ========== to ========== Change SwarmingIsolatedScriptTest to upload json format results BUG=649762 Committed: https://chromium.googlesource.com/chromium/tools/build/+/e21ee362bd0357572c63... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/tools/build/+/e21ee362bd0357572c63... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
