Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1778153002:
[Findit] Strip platform from step_name before triggering try job. (Closed)
Description[Findit] Strip platform from step_name before triggering try job.
BUG=593528
Committed: https://chromium.googlesource.com/infra/infra/+/b21f57d3906379654fdb9adacb7eb46df1d74a11
Patch Set 1 #
Total comments: 6
Patch Set 2 : address comments #Patch Set 3 : address comments #
Total comments: 16
Patch Set 4 : . #
Total comments: 1
Patch Set 5 : . #
Messages
Total messages: 20 (3 generated)
chanli@chromium.org changed reviewers: + lijeffrey@chromium.org, stgao@chromium.org
PTAL.
https://codereview.chromium.org/1778153002/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/try_job_util.py (right): https://codereview.chromium.org/1778153002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/try_job_util.py:88: updated_tests[step_name.strip()[0]] = tests How about using the value of tag "name" in the Swarming tasks instead? IIRC, we already pull information from Swarming Server for the whole build to determine which steps are run on Swarming. For those not running on Swarming, just default to the step name from build json data. https://apis-explorer.appspot.com/apis-explorer/?base=https://chromium-swarm....
https://codereview.chromium.org/1778153002/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/try_job_util.py (right): https://codereview.chromium.org/1778153002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/try_job_util.py:82: def _StripPlatform(targeted_tests): nit: just for clarity maybe rename this '_GetTargetedTestsWithoutPlatform' or something similar and add a doc string https://codereview.chromium.org/1778153002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/try_job_util.py:88: updated_tests[step_name.strip()[0]] = tests are you sure you want step_name.strip()[0]? I think that only gives you the first letter?
PTAL https://codereview.chromium.org/1778153002/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/try_job_util.py (right): https://codereview.chromium.org/1778153002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/try_job_util.py:82: def _StripPlatform(targeted_tests): On 2016/03/10 00:43:56, lijeffrey wrote: > nit: just for clarity maybe rename this '_GetTargetedTestsWithoutPlatform' or > something similar and add a doc string Function deleted. https://codereview.chromium.org/1778153002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/try_job_util.py:88: updated_tests[step_name.strip()[0]] = tests On 2016/03/10 00:43:56, lijeffrey wrote: > are you sure you want step_name.strip()[0]? I think that only gives you the > first letter? Right... I wanted to use split... Done https://codereview.chromium.org/1778153002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/try_job_util.py:88: updated_tests[step_name.strip()[0]] = tests On 2016/03/10 00:39:30, stgao wrote: > How about using the value of tag "name" in the Swarming tasks instead? > > IIRC, we already pull information from Swarming Server for the whole build to > determine which steps are run on Swarming. > For those not running on Swarming, just default to the step name from build json > data. > > https://apis-explorer.appspot.com/apis-explorer/?base=https://chromium-swarm.... You're right, and actually this patch won't work because it just modified original target_tests but not the updated_one.
https://codereview.chromium.org/1778153002/diff/40001/appengine/findit/waterf... File appengine/findit/waterfall/process_swarming_task_result_pipeline.py (right): https://codereview.chromium.org/1778153002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/process_swarming_task_result_pipeline.py:88: step_name_no_platform = step_name Should not default to step name for steps running on Swarming. https://codereview.chromium.org/1778153002/diff/40001/appengine/findit/waterf... File appengine/findit/waterfall/swarming_tasks_to_try_job_pipeline.py (right): https://codereview.chromium.org/1778153002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/swarming_tasks_to_try_job_pipeline.py:36: task_id) Could be reverted? https://codereview.chromium.org/1778153002/diff/40001/appengine/findit/waterf... File appengine/findit/waterfall/trigger_swarming_task_pipeline.py (right): https://codereview.chromium.org/1778153002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/trigger_swarming_task_pipeline.py:57: ref_name = swarming_util.GetTagValue(new_request.tags, 'name') or step_name Should get from the `ref_request` instead, right? https://codereview.chromium.org/1778153002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/trigger_swarming_task_pipeline.py:57: ref_name = swarming_util.GetTagValue(new_request.tags, 'name') or step_name And should not default to the step_name. https://codereview.chromium.org/1778153002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/trigger_swarming_task_pipeline.py:57: ref_name = swarming_util.GetTagValue(new_request.tags, 'name') or step_name I'm wondering why don't we get the name from the check_first_failure stage/pipeline. Didn't we query all tasks for all Swarming steps earlier before triggering any Swarming task? Is there a reason we do it here instead of an earlier stage?
https://codereview.chromium.org/1778153002/diff/40001/appengine/findit/waterf... File appengine/findit/waterfall/process_swarming_task_result_pipeline.py (right): https://codereview.chromium.org/1778153002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/process_swarming_task_result_pipeline.py:88: step_name_no_platform = step_name On 2016/03/10 23:50:28, stgao wrote: > Should not default to step name for steps running on Swarming. I think we need to fall back to step name just in case. https://codereview.chromium.org/1778153002/diff/40001/appengine/findit/waterf... File appengine/findit/waterfall/swarming_tasks_to_try_job_pipeline.py (right): https://codereview.chromium.org/1778153002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/swarming_tasks_to_try_job_pipeline.py:36: task_id) On 2016/03/10 23:50:28, stgao wrote: > Could be reverted? Missed... Done https://codereview.chromium.org/1778153002/diff/40001/appengine/findit/waterf... File appengine/findit/waterfall/trigger_swarming_task_pipeline.py (right): https://codereview.chromium.org/1778153002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/trigger_swarming_task_pipeline.py:57: ref_name = swarming_util.GetTagValue(new_request.tags, 'name') or step_name On 2016/03/10 23:50:28, stgao wrote: > Should get from the `ref_request` instead, right? No, this is the referred task, the tag is 'name', 'ref_name' is the tag I added to the findit tasks. https://codereview.chromium.org/1778153002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/trigger_swarming_task_pipeline.py:57: ref_name = swarming_util.GetTagValue(new_request.tags, 'name') or step_name On 2016/03/10 23:50:28, stgao wrote: > And should not default to the step_name. Done. https://codereview.chromium.org/1778153002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/trigger_swarming_task_pipeline.py:57: ref_name = swarming_util.GetTagValue(new_request.tags, 'name') or step_name On 2016/03/10 23:50:28, stgao wrote: > I'm wondering why don't we get the name from the check_first_failure > stage/pipeline. Didn't we query all tasks for all Swarming steps earlier before > triggering any Swarming task? > > Is there a reason we do it here instead of an earlier stage? If we get the name when we list swarming tasks in detect_first_failure_pipeline, we need to either carry it along the pipeline or save it somewhere and retrieve it later... While we are already parsing the referred request here, I think the name can be treated as a part of request (in tags) anyway.
https://codereview.chromium.org/1778153002/diff/40001/appengine/findit/waterf... File appengine/findit/waterfall/process_swarming_task_result_pipeline.py (right): https://codereview.chromium.org/1778153002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/process_swarming_task_result_pipeline.py:88: step_name_no_platform = step_name On 2016/03/11 00:49:41, chanli wrote: > On 2016/03/10 23:50:28, stgao wrote: > > Should not default to step name for steps running on Swarming. > > I think we need to fall back to step name just in case. In which scenario do you think we need to fall back to step name? https://codereview.chromium.org/1778153002/diff/40001/appengine/findit/waterf... File appengine/findit/waterfall/trigger_swarming_task_pipeline.py (right): https://codereview.chromium.org/1778153002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/trigger_swarming_task_pipeline.py:57: ref_name = swarming_util.GetTagValue(new_request.tags, 'name') or step_name On 2016/03/11 00:49:41, chanli wrote: > On 2016/03/10 23:50:28, stgao wrote: > > Should get from the `ref_request` instead, right? > > No, this is the referred task, the tag is 'name', 'ref_name' is the tag I added > to the findit tasks. My thought is that ref_request is the original data (source of truth), while new_request is not the source of truth. And we clear new_request.tags after this line.
https://codereview.chromium.org/1778153002/diff/40001/appengine/findit/waterf... File appengine/findit/waterfall/process_swarming_task_result_pipeline.py (right): https://codereview.chromium.org/1778153002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/process_swarming_task_result_pipeline.py:88: step_name_no_platform = step_name On 2016/03/11 01:03:20, stgao wrote: > On 2016/03/11 00:49:41, chanli wrote: > > On 2016/03/10 23:50:28, stgao wrote: > > > Should not default to step name for steps running on Swarming. > > > > I think we need to fall back to step name just in case. > > In which scenario do you think we need to fall back to step name? Just for precaution. https://codereview.chromium.org/1778153002/diff/40001/appengine/findit/waterf... File appengine/findit/waterfall/trigger_swarming_task_pipeline.py (right): https://codereview.chromium.org/1778153002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/trigger_swarming_task_pipeline.py:57: ref_name = swarming_util.GetTagValue(new_request.tags, 'name') or step_name On 2016/03/11 01:03:20, stgao wrote: > On 2016/03/11 00:49:41, chanli wrote: > > On 2016/03/10 23:50:28, stgao wrote: > > > Should get from the `ref_request` instead, right? > > > > No, this is the referred task, the tag is 'name', 'ref_name' is the tag I > added > > to the findit tasks. > > My thought is that ref_request is the original data (source of truth), while > new_request is not the source of truth. And we clear new_request.tags after this > line. But before you clear new_request.tags, the tags in it are deepcopied from ref_request. I don't see any problems here. If you still have doubt, I can change it to ref_name = swarming_util.GetTagValue(new_request.tags, 'name') or step_name Although they are the same to me.
https://codereview.chromium.org/1778153002/diff/40001/appengine/findit/waterf... File appengine/findit/waterfall/process_swarming_task_result_pipeline.py (right): https://codereview.chromium.org/1778153002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/process_swarming_task_result_pipeline.py:88: step_name_no_platform = step_name On 2016/03/11 03:47:00, chanli wrote: > On 2016/03/11 01:03:20, stgao wrote: > > On 2016/03/11 00:49:41, chanli wrote: > > > On 2016/03/10 23:50:28, stgao wrote: > > > > Should not default to step name for steps running on Swarming. > > > > > > I think we need to fall back to step name just in case. > > > > In which scenario do you think we need to fall back to step name? > > Just for precaution. But the step names always includes the platform name now. That won't work. https://codereview.chromium.org/1778153002/diff/40001/appengine/findit/waterf... File appengine/findit/waterfall/trigger_swarming_task_pipeline.py (right): https://codereview.chromium.org/1778153002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/trigger_swarming_task_pipeline.py:57: ref_name = swarming_util.GetTagValue(new_request.tags, 'name') or step_name On 2016/03/11 03:47:00, chanli wrote: > On 2016/03/11 01:03:20, stgao wrote: > > On 2016/03/11 00:49:41, chanli wrote: > > > On 2016/03/10 23:50:28, stgao wrote: > > > > Should get from the `ref_request` instead, right? > > > > > > No, this is the referred task, the tag is 'name', 'ref_name' is the tag I > > added > > > to the findit tasks. > > > > My thought is that ref_request is the original data (source of truth), while > > new_request is not the source of truth. And we clear new_request.tags after > this > > line. > > But before you clear new_request.tags, the tags in it are deepcopied from > ref_request. I don't see any problems here. If you still have doubt, I can > change it to > > ref_name = swarming_util.GetTagValue(new_request.tags, 'name') or step_name > > Although they are the same to me. Using either new_request.tags or ref_request.tags has the same effect from the perspective of program. However, from the perspective of coding style, I believe using ref_request is the right way to go. https://codereview.chromium.org/1778153002/diff/60001/appengine/findit/waterf... File appengine/findit/waterfall/trigger_swarming_task_pipeline.py (right): https://codereview.chromium.org/1778153002/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/trigger_swarming_task_pipeline.py:57: ref_name = swarming_util.GetTagValue(new_request.tags, 'name') or step_name We'd better not default to step name here, since step name always includes the platform name now.
lgtm
lgtm
The CQ bit was checked by chanli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1778153002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1778153002/80001
Message was sent while issue was closed.
Description was changed from ========== [Findit] Strip platform from step_name before triggering try job. BUG=593528 ========== to ========== [Findit] Strip platform from step_name before triggering try job. BUG=593528 Committed: https://chromium.googlesource.com/infra/infra/+/b21f57d3906379654fdb9adacb7eb... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/infra/infra/+/b21f57d3906379654fdb9adacb7eb...
Message was sent while issue was closed.
On 2016/03/11 20:00:54, commit-bot: I haz the power wrote: > Committed patchset #5 (id:80001) as > https://chromium.googlesource.com/infra/infra/+/b21f57d3906379654fdb9adacb7eb... I think this CL is causing test failures (e.g., https://build.chromium.org/p/tryserver.infra/builders/Infra%20Linux%20Trusty%...): ====================================================================== ERROR: handlers.test.build_failure_test.BuildFailureTest.testUpdateAnalysisResultWithTryJob (/b/build/slave/infra/build/infra/appengine/findit/handlers/test/build_failure_test.expected/BuildFailureTest.testUpdateAnalysisResultWithTryJob.json) ---------------------------------------------------------------------- Traceback (most recent call last): File "/b/build/slave/infra/build/infra/ENV/local/lib/python2.7/site-packages/expect_tests/pipeline.py", line 397, in process_test subresult = subtest.run() File "/b/build/slave/infra/build/infra/ENV/local/lib/python2.7/site-packages/expect_tests/type_definitions.py", line 240, in run return self.func_call(context=context) File "/b/build/slave/infra/build/infra/ENV/local/lib/python2.7/site-packages/expect_tests/type_definitions.py", line 155, in __call__ return f.func(*f.args, **f.kwargs) File "/b/build/slave/infra/build/infra/ENV/local/lib/python2.7/site-packages/expect_tests/unittest_helper.py", line 30, in _RunTestCaseSingle return Result(getattr(test_instance, test_name)()) File "/b/build/slave/infra/build/infra/appengine/findit/handlers/test/build_failure_test.py", line 737, in testUpdateAnalysisResultWithTryJob self.assertEqual(result, expected_updated_result) File "/usr/lib/python2.7/unittest/case.py", line 515, in assertEqual assertion_func(first, second, msg=msg) File "/usr/lib/python2.7/unittest/case.py", line 837, in assertDictEqual self.fail(self._formatMessage(msg, standardMsg)) File "/usr/lib/python2.7/unittest/case.py", line 412, in fail raise self.failureException(msg) AssertionError: {'failures': [{'tests': [{'suspected_cls': [{'commit_position': 123, 'revision': [truncated]... != {'failures': [{'tests': [{'suspected_cls': [{'result_source': ['heuristic analys [truncated]...
Message was sent while issue was closed.
On 2016/03/11 22:38:30, dnj wrote: > On 2016/03/11 20:00:54, commit-bot: I haz the power wrote: > > Committed patchset #5 (id:80001) as > > > https://chromium.googlesource.com/infra/infra/+/b21f57d3906379654fdb9adacb7eb... > > I think this CL is causing test failures (e.g., > https://build.chromium.org/p/tryserver.infra/builders/Infra%20Linux%20Trusty%...): > > > ====================================================================== > ERROR: > handlers.test.build_failure_test.BuildFailureTest.testUpdateAnalysisResultWithTryJob > (/b/build/slave/infra/build/infra/appengine/findit/handlers/test/build_failure_test.expected/BuildFailureTest.testUpdateAnalysisResultWithTryJob.json) > ---------------------------------------------------------------------- > Traceback (most recent call last): > File > "/b/build/slave/infra/build/infra/ENV/local/lib/python2.7/site-packages/expect_tests/pipeline.py", > line 397, in process_test > subresult = subtest.run() > File > "/b/build/slave/infra/build/infra/ENV/local/lib/python2.7/site-packages/expect_tests/type_definitions.py", > line 240, in run > return self.func_call(context=context) > File > "/b/build/slave/infra/build/infra/ENV/local/lib/python2.7/site-packages/expect_tests/type_definitions.py", > line 155, in __call__ > return f.func(*f.args, **f.kwargs) > File > "/b/build/slave/infra/build/infra/ENV/local/lib/python2.7/site-packages/expect_tests/unittest_helper.py", > line 30, in _RunTestCaseSingle > return Result(getattr(test_instance, test_name)()) > File > "/b/build/slave/infra/build/infra/appengine/findit/handlers/test/build_failure_test.py", > line 737, in testUpdateAnalysisResultWithTryJob > self.assertEqual(result, expected_updated_result) > File "/usr/lib/python2.7/unittest/case.py", line 515, in assertEqual > assertion_func(first, second, msg=msg) > File "/usr/lib/python2.7/unittest/case.py", line 837, in assertDictEqual > self.fail(self._formatMessage(msg, standardMsg)) > File "/usr/lib/python2.7/unittest/case.py", line 412, in fail > raise self.failureException(msg) > AssertionError: {'failures': [{'tests': [{'suspected_cls': [{'commit_position': > 123, 'revision': [truncated]... != {'failures': [{'tests': [{'suspected_cls': > [{'result_source': ['heuristic analys [truncated]... I'll check it. Thanks.
Message was sent while issue was closed.
On 2016/03/11 22:38:30, dnj wrote: > On 2016/03/11 20:00:54, commit-bot: I haz the power wrote: > > Committed patchset #5 (id:80001) as > > > https://chromium.googlesource.com/infra/infra/+/b21f57d3906379654fdb9adacb7eb... > > I think this CL is causing test failures (e.g., > https://build.chromium.org/p/tryserver.infra/builders/Infra%20Linux%20Trusty%...): > > > ====================================================================== > ERROR: > handlers.test.build_failure_test.BuildFailureTest.testUpdateAnalysisResultWithTryJob > (/b/build/slave/infra/build/infra/appengine/findit/handlers/test/build_failure_test.expected/BuildFailureTest.testUpdateAnalysisResultWithTryJob.json) > ---------------------------------------------------------------------- > Traceback (most recent call last): > File > "/b/build/slave/infra/build/infra/ENV/local/lib/python2.7/site-packages/expect_tests/pipeline.py", > line 397, in process_test > subresult = subtest.run() > File > "/b/build/slave/infra/build/infra/ENV/local/lib/python2.7/site-packages/expect_tests/type_definitions.py", > line 240, in run > return self.func_call(context=context) > File > "/b/build/slave/infra/build/infra/ENV/local/lib/python2.7/site-packages/expect_tests/type_definitions.py", > line 155, in __call__ > return f.func(*f.args, **f.kwargs) > File > "/b/build/slave/infra/build/infra/ENV/local/lib/python2.7/site-packages/expect_tests/unittest_helper.py", > line 30, in _RunTestCaseSingle > return Result(getattr(test_instance, test_name)()) > File > "/b/build/slave/infra/build/infra/appengine/findit/handlers/test/build_failure_test.py", > line 737, in testUpdateAnalysisResultWithTryJob > self.assertEqual(result, expected_updated_result) > File "/usr/lib/python2.7/unittest/case.py", line 515, in assertEqual > assertion_func(first, second, msg=msg) > File "/usr/lib/python2.7/unittest/case.py", line 837, in assertDictEqual > self.fail(self._formatMessage(msg, standardMsg)) > File "/usr/lib/python2.7/unittest/case.py", line 412, in fail > raise self.failureException(msg) > AssertionError: {'failures': [{'tests': [{'suspected_cls': [{'commit_position': > 123, 'revision': [truncated]... != {'failures': [{'tests': [{'suspected_cls': > [{'result_source': ['heuristic analys [truncated]... It turned out there was a flaky test in another CL of mine(https://codereview.chromium.org/1761783005). I have reverted it. Thanks. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
