Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1827903002:
[Findit] Modify handlers_util to prepare for the new UI change. (Closed)
Description[Findit] Modify handlers_util to prepare for the new UI change.
Basically new changes will group swarming task data and try job data by step_name first, then the culprits.
BUG=598507
Committed: https://chromium.googlesource.com/infra/infra/+/7169a9d4560901db9d18df4037f9996f5ad2c03e
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : Add flaky info to try job result. #
Total comments: 14
Patch Set 4 : Address comments. #Patch Set 5 : Always include tests in try job information. #Patch Set 6 : . #Patch Set 7 : Modify data format for compile failure #
Total comments: 28
Patch Set 8 : Address comments. #Patch Set 9 : Add module for additional result status. #Patch Set 10 : . #
Total comments: 6
Patch Set 11 : Fix bug when display builds that don't have failure-result-map #
Total comments: 14
Patch Set 12 : address comments. #
Total comments: 2
Patch Set 13 : address comments #
Total comments: 3
Patch Set 14 : remove possible flaky tests. #
Messages
Total messages: 44 (12 generated)
chanli@chromium.org changed reviewers: + lijeffrey@chromium.org, stgao@chromium.org
PTAL
On 2016/03/23 17:25:17, chanli wrote: > PTAL I think I need to re-organize a little bit on the code, please hold the review.
PTAL
Posted some comments as of now, will review more closely later today. Do you mind uploading the other CL that use the code in this CL? https://codereview.chromium.org/1827903002/diff/40001/appengine/findit/handle... File appengine/findit/handlers/handlers_util.py (right): https://codereview.chromium.org/1827903002/diff/40001/appengine/findit/handle... appengine/findit/handlers/handlers_util.py:19: NO_SWARMING_TASK_FOUND = 'No swarming rerun found.' When will we have this case? How about adding some comments to explain the different scenarios? https://codereview.chromium.org/1827903002/diff/40001/appengine/findit/handle... appengine/findit/handlers/handlers_util.py:33: SWARMING_STATUS_TO_DESCRIPTION[wf_analysis_status.ERROR])), Should we do this conversion on the UI side instead of the underlying data or code? https://codereview.chromium.org/1827903002/diff/40001/appengine/findit/handle... appengine/findit/handlers/handlers_util.py:87: 'status': 'No swarming rerun found' Alternative: for underlying data, use enum to indicate statuses. And on UI, convert it to message strings. https://codereview.chromium.org/1827903002/diff/40001/appengine/findit/handle... appengine/findit/handlers/handlers_util.py:125: step_name.split()[0] mind a comment why this split? This could make the code more readable for new comers. https://codereview.chromium.org/1827903002/diff/40001/appengine/findit/handle... appengine/findit/handlers/handlers_util.py:132: waterfall_config.GetSwarmingSettings()['server_host'], Will this cause too many reads against datastore or memcache? https://codereview.chromium.org/1827903002/diff/40001/appengine/findit/handle... appengine/findit/handlers/handlers_util.py:134: if task.classified_tests: When we don't have such info? And how should we deal with that case? It might not be a bad idea to add some comment to explain different cases and how they are handled properly.
Yes, that's my plan. On Thu, Mar 24, 2016 at 10:59 AM, <stgao@chromium.org> wrote: > Posted some comments as of now, will review more closely later today. > > Do you mind uploading the other CL that use the code in this CL? > > > > https://codereview.chromium.org/1827903002/diff/40001/appengine/findit/handle... > File appengine/findit/handlers/handlers_util.py (right): > > > https://codereview.chromium.org/1827903002/diff/40001/appengine/findit/handle... > appengine/findit/handlers/handlers_util.py:19: NO_SWARMING_TASK_FOUND = > 'No swarming rerun found.' > When will we have this case? > > How about adding some comments to explain the different scenarios? > > > https://codereview.chromium.org/1827903002/diff/40001/appengine/findit/handle... > appengine/findit/handlers/handlers_util.py:33: > SWARMING_STATUS_TO_DESCRIPTION[wf_analysis_status.ERROR])), > Should we do this conversion on the UI side instead of the underlying > data or code? > > > https://codereview.chromium.org/1827903002/diff/40001/appengine/findit/handle... > appengine/findit/handlers/handlers_util.py:87: 'status': 'No swarming > rerun found' > Alternative: for underlying data, use enum to indicate statuses. And on > UI, convert it to message strings. > > > https://codereview.chromium.org/1827903002/diff/40001/appengine/findit/handle... > appengine/findit/handlers/handlers_util.py:125: step_name.split()[0] > mind a comment why this split? This could make the code more readable > for new comers. > > > https://codereview.chromium.org/1827903002/diff/40001/appengine/findit/handle... > appengine/findit/handlers/handlers_util.py:132: > waterfall_config.GetSwarmingSettings()['server_host'], > Will this cause too many reads against datastore or memcache? > > > https://codereview.chromium.org/1827903002/diff/40001/appengine/findit/handle... > appengine/findit/handlers/handlers_util.py:134: if > task.classified_tests: > When we don't have such info? And how should we deal with that case? > > It might not be a bad idea to add some comment to explain different > cases and how they are handled properly. > > https://codereview.chromium.org/1827903002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/03/24 18:00:30, chromium-reviews wrote: > Yes, that's my plan. > > On Thu, Mar 24, 2016 at 10:59 AM, <mailto:stgao@chromium.org> wrote: > > > Posted some comments as of now, will review more closely later today. > > > > Do you mind uploading the other CL that use the code in this CL? The reason I asked is to understand how the code is used so that I could give better comments.
Comments addressed. I am still working on the part that will use this change. I will upload another CL later today. https://codereview.chromium.org/1827903002/diff/40001/appengine/findit/handle... File appengine/findit/handlers/handlers_util.py (right): https://codereview.chromium.org/1827903002/diff/40001/appengine/findit/handle... appengine/findit/handlers/handlers_util.py:19: NO_SWARMING_TASK_FOUND = 'No swarming rerun found.' On 2016/03/24 17:59:48, stgao wrote: > When will we have this case? > > How about adding some comments to explain the different scenarios? This would happen for example we delete that task in data store for whatever reason. https://codereview.chromium.org/1827903002/diff/40001/appengine/findit/handle... appengine/findit/handlers/handlers_util.py:33: SWARMING_STATUS_TO_DESCRIPTION[wf_analysis_status.ERROR])), On 2016/03/24 17:59:48, stgao wrote: > Should we do this conversion on the UI side instead of the underlying data or > code? Done. https://codereview.chromium.org/1827903002/diff/40001/appengine/findit/handle... appengine/findit/handlers/handlers_util.py:87: 'status': 'No swarming rerun found' On 2016/03/24 17:59:48, stgao wrote: > Alternative: for underlying data, use enum to indicate statuses. And on UI, > convert it to message strings. Done. https://codereview.chromium.org/1827903002/diff/40001/appengine/findit/handle... appengine/findit/handlers/handlers_util.py:125: step_name.split()[0] On 2016/03/24 17:59:48, stgao wrote: > mind a comment why this split? This could make the code more readable for new > comers. Done. https://codereview.chromium.org/1827903002/diff/40001/appengine/findit/handle... appengine/findit/handlers/handlers_util.py:132: waterfall_config.GetSwarmingSettings()['server_host'], On 2016/03/24 17:59:48, stgao wrote: > Will this cause too many reads against datastore or memcache? Done. https://codereview.chromium.org/1827903002/diff/40001/appengine/findit/handle... appengine/findit/handlers/handlers_util.py:134: if task.classified_tests: On 2016/03/24 17:59:48, stgao wrote: > When we don't have such info? And how should we deal with that case? > > It might not be a bad idea to add some comment to explain different cases and > how they are handled properly. Basically if the task is not completed yet, we will not have such info. Comments added.
https://codereview.chromium.org/1827903002/diff/120001/appengine/findit/handl... File appengine/findit/handlers/handlers_util.py (right): https://codereview.chromium.org/1827903002/diff/120001/appengine/findit/handl... appengine/findit/handlers/handlers_util.py:16: FLAKY = 200 How about creating a message module for these? FLAKY_MESSAGE = 200 def getMessage(id): messages = { FLAKY_MESSAGE: 'Flaky', } return messages.get(id, '!!!MESSAGE BUG!!!') https://codereview.chromium.org/1827903002/diff/120001/appengine/findit/handl... appengine/findit/handlers/handlers_util.py:41: def _GetAllTestsForASwarmingTask(task_key, step_failure_result_map): nit: three empty lines above. https://codereview.chromium.org/1827903002/diff/120001/appengine/findit/handl... appengine/findit/handlers/handlers_util.py:91: swarming_server = waterfall_config.GetSwarmingSettings()['server_host'] This datastore read is not needed if we don't process any data. https://codereview.chromium.org/1827903002/diff/120001/appengine/findit/handl... appengine/findit/handlers/handlers_util.py:102: swarming_task_keys.add(first_failure_key) Maybe simplified with: swarming_task_keys = set(failure.values()) https://codereview.chromium.org/1827903002/diff/120001/appengine/findit/handl... appengine/findit/handlers/handlers_util.py:135: if task.classified_tests: # Swarming rerun has result. This is when the Swarming task completed? https://codereview.chromium.org/1827903002/diff/120001/appengine/findit/handl... appengine/findit/handlers/handlers_util.py:154: master_name, builder_name, build_number) nit: indent https://codereview.chromium.org/1827903002/diff/120001/appengine/findit/handl... appengine/findit/handlers/handlers_util.py:223: if try_job_info.get('status'): "if not" instead? https://codereview.chromium.org/1827903002/diff/120001/appengine/findit/handl... appengine/findit/handlers/handlers_util.py:224: # The try job has been updated by swarming task, no try job yet or ever. this comment seems not clear enough. https://codereview.chromium.org/1827903002/diff/120001/appengine/findit/handl... appengine/findit/handlers/handlers_util.py:229: ref_name = try_job_info['ref_name'] nit: not used outside of if below. https://codereview.chromium.org/1827903002/diff/120001/appengine/findit/handl... appengine/findit/handlers/handlers_util.py:249: all_tests = list(set(all_tests) ^ set(failed_tests)) Name of ``all_tests`` seems a little misleading. https://codereview.chromium.org/1827903002/diff/120001/appengine/findit/handl... appengine/findit/handlers/handlers_util.py:251: break Why we stop here? Mind a comment? https://codereview.chromium.org/1827903002/diff/120001/appengine/findit/handl... appengine/findit/handlers/handlers_util.py:308: if swarming_task_key != try_job_key: Why we need this check? A comment to explain is appreciated. https://codereview.chromium.org/1827903002/diff/120001/appengine/findit/handl... appengine/findit/handlers/handlers_util.py:320: elif task.get('flaky_tests'): # pragma: no cover It might be possible that a step has both flaky and reliable test failures. Is that case also handled here? https://codereview.chromium.org/1827903002/diff/120001/appengine/findit/handl... appengine/findit/handlers/handlers_util.py:409: master_name, builder_name, build_number) nit: indent.
https://codereview.chromium.org/1827903002/diff/120001/appengine/findit/handl... File appengine/findit/handlers/handlers_util.py (right): https://codereview.chromium.org/1827903002/diff/120001/appengine/findit/handl... appengine/findit/handlers/handlers_util.py:16: FLAKY = 200 On 2016/03/25 22:50:23, stgao wrote: > How about creating a message module for these? > > FLAKY_MESSAGE = 200 > > def getMessage(id): > messages = { > FLAKY_MESSAGE: 'Flaky', > } > return messages.get(id, '!!!MESSAGE BUG!!!') Per our previous discussion, the status here are all enumeration. The messages will be converted at the UI side. https://codereview.chromium.org/1827903002/diff/120001/appengine/findit/handl... appengine/findit/handlers/handlers_util.py:41: def _GetAllTestsForASwarmingTask(task_key, step_failure_result_map): On 2016/03/25 22:50:24, stgao wrote: > nit: three empty lines above. Done. https://codereview.chromium.org/1827903002/diff/120001/appengine/findit/handl... appengine/findit/handlers/handlers_util.py:91: swarming_server = waterfall_config.GetSwarmingSettings()['server_host'] On 2016/03/25 22:50:23, stgao wrote: > This datastore read is not needed if we don't process any data. Done. https://codereview.chromium.org/1827903002/diff/120001/appengine/findit/handl... appengine/findit/handlers/handlers_util.py:102: swarming_task_keys.add(first_failure_key) On 2016/03/25 22:50:24, stgao wrote: > Maybe simplified with: > > swarming_task_keys = set(failure.values()) Done. https://codereview.chromium.org/1827903002/diff/120001/appengine/findit/handl... appengine/findit/handlers/handlers_util.py:135: if task.classified_tests: # Swarming rerun has result. On 2016/03/25 22:50:23, stgao wrote: > This is when the Swarming task completed? Yes. this is derived from the result of swarming task. https://codereview.chromium.org/1827903002/diff/120001/appengine/findit/handl... appengine/findit/handlers/handlers_util.py:154: master_name, builder_name, build_number) On 2016/03/25 22:50:23, stgao wrote: > nit: indent Done. https://codereview.chromium.org/1827903002/diff/120001/appengine/findit/handl... appengine/findit/handlers/handlers_util.py:223: if try_job_info.get('status'): On 2016/03/25 22:50:24, stgao wrote: > "if not" instead? I use 'if' here to reduce one level of indentation. https://codereview.chromium.org/1827903002/diff/120001/appengine/findit/handl... appengine/findit/handlers/handlers_util.py:224: # The try job has been updated by swarming task, no try job yet or ever. On 2016/03/25 22:50:23, stgao wrote: > this comment seems not clear enough. Done. https://codereview.chromium.org/1827903002/diff/120001/appengine/findit/handl... appengine/findit/handlers/handlers_util.py:229: ref_name = try_job_info['ref_name'] On 2016/03/25 22:50:23, stgao wrote: > nit: not used outside of if below. Done. https://codereview.chromium.org/1827903002/diff/120001/appengine/findit/handl... appengine/findit/handlers/handlers_util.py:249: all_tests = list(set(all_tests) ^ set(failed_tests)) On 2016/03/25 22:50:24, stgao wrote: > Name of ``all_tests`` seems a little misleading. Done. https://codereview.chromium.org/1827903002/diff/120001/appengine/findit/handl... appengine/findit/handlers/handlers_util.py:251: break On 2016/03/25 22:50:24, stgao wrote: > Why we stop here? Mind a comment? Done. https://codereview.chromium.org/1827903002/diff/120001/appengine/findit/handl... appengine/findit/handlers/handlers_util.py:308: if swarming_task_key != try_job_key: On 2016/03/25 22:50:23, stgao wrote: > Why we need this check? A comment to explain is appreciated. Yes... There will only be one task for each step in one build. Changed. https://codereview.chromium.org/1827903002/diff/120001/appengine/findit/handl... appengine/findit/handlers/handlers_util.py:320: elif task.get('flaky_tests'): # pragma: no cover On 2016/03/25 22:50:23, stgao wrote: > It might be possible that a step has both flaky and reliable test failures. > > Is that case also handled here? Yes. That case is handled by ln329 - ln336. https://codereview.chromium.org/1827903002/diff/120001/appengine/findit/handl... appengine/findit/handlers/handlers_util.py:409: master_name, builder_name, build_number) On 2016/03/25 22:50:23, stgao wrote: > nit: indent. Done.
Will give more comments after reviewing the depending CLs. https://codereview.chromium.org/1827903002/diff/180001/appengine/findit/handl... File appengine/findit/handlers/handlers_util.py (right): https://codereview.chromium.org/1827903002/diff/180001/appengine/findit/handl... appengine/findit/handlers/handlers_util.py:117: if task.classified_tests: # Swarming rerun has result. As the swarming task already completed here, how about mentioning that explicitly here for readability? Possibly also mention how we handle the case the Swarming task has started but not yet completed. https://codereview.chromium.org/1827903002/diff/180001/appengine/findit/handl... appengine/findit/handlers/handlers_util.py:206: # The try job has been updated by swarming task: I don't quite understand why try job is updated by Swarming task? You meant to say "try job is triggerred after Swarming task completed"? And if the try-job has a status, why we do not run the code below? https://codereview.chromium.org/1827903002/diff/180001/appengine/findit/handl... File appengine/findit/handlers/test/result_status_test.py (right): https://codereview.chromium.org/1827903002/diff/180001/appengine/findit/handl... appengine/findit/handlers/test/result_status_test.py:1: # Copyright 2016 The Chromium Authors. All rights reserved. As result_status is straight-forward, we could skip this test if you want. https://codereview.chromium.org/1827903002/diff/200001/appengine/findit/handl... File appengine/findit/handlers/result_status.py (right): https://codereview.chromium.org/1827903002/diff/200001/appengine/findit/handl... appengine/findit/handlers/result_status.py:27: status_message_map = { Make this as a constant var instead of a function?
Just some basic comments for now, will review the other CL https://codereview.chromium.org/1827903002/diff/40001/appengine/findit/handle... File appengine/findit/handlers/handlers_util.py (right): https://codereview.chromium.org/1827903002/diff/40001/appengine/findit/handle... appengine/findit/handlers/handlers_util.py:44: nit: too many blank lines https://codereview.chromium.org/1827903002/diff/40001/appengine/findit/handle... appengine/findit/handlers/handlers_util.py:153: master_name, builder_name, build_number) nit: 4 spaces https://codereview.chromium.org/1827903002/diff/200001/appengine/findit/handl... File appengine/findit/handlers/handlers_util.py (right): https://codereview.chromium.org/1827903002/diff/200001/appengine/findit/handl... appengine/findit/handlers/handlers_util.py:112: if task.task_id: # Swarming rerun has started. nit: 2 spaces before # https://codereview.chromium.org/1827903002/diff/200001/appengine/findit/handl... appengine/findit/handlers/handlers_util.py:206: if try_job_info.get('status'): this if can be combined into 1 if i.e. if (try_job_key != try_job_info['try_job_key'] or try_job_info.get('status')): continue https://codereview.chromium.org/1827903002/diff/200001/appengine/findit/handl... appengine/findit/handlers/handlers_util.py:327: try_jobs.extend(additional_flakiness_list) nit: even if additional_flakiness_list is empty and not None, the if is not needed as the extend will be the same https://codereview.chromium.org/1827903002/diff/200001/appengine/findit/handl... appengine/findit/handlers/handlers_util.py:400: for step_name in failure_result_map: I would separate this out into a different function IsStepFailure(failure_result_map) https://codereview.chromium.org/1827903002/diff/200001/appengine/findit/handl... appengine/findit/handlers/handlers_util.py:401: if step_name == 'compile': nit: just to be safe use step_name.lower() https://codereview.chromium.org/1827903002/diff/200001/appengine/findit/handl... File appengine/findit/handlers/test/result_status_test.py (right): https://codereview.chromium.org/1827903002/diff/200001/appengine/findit/handl... appengine/findit/handlers/test/result_status_test.py:13: self.assertEqual( nit: 2 spaces
https://codereview.chromium.org/1827903002/diff/220001/appengine/findit/handl... File appengine/findit/handlers/result_status.py (right): https://codereview.chromium.org/1827903002/diff/220001/appengine/findit/handl... appengine/findit/handlers/result_status.py:31: NO_SWARMING_TASK_FOUND: 'No swarming task found, hence no try job.', Swarming task is not clear here. I think we refer to the Swarming rerun by Findit, right? Same for all below. https://codereview.chromium.org/1827903002/diff/220001/appengine/findit/handl... appengine/findit/handlers/result_status.py:32: NON_SWARMING_NO_RERUN: ('No swarming task nor try job will be triggered' wording nit: non-swarming step, so no swarming rerun and try job.
https://codereview.chromium.org/1827903002/diff/180001/appengine/findit/handl... File appengine/findit/handlers/handlers_util.py (right): https://codereview.chromium.org/1827903002/diff/180001/appengine/findit/handl... appengine/findit/handlers/handlers_util.py:117: if task.classified_tests: # Swarming rerun has result. On 2016/03/28 22:27:22, stgao wrote: > As the swarming task already completed here, how about mentioning that > explicitly here for readability? > Possibly also mention how we handle the case the Swarming task has started but > not yet completed. Done. https://codereview.chromium.org/1827903002/diff/180001/appengine/findit/handl... appengine/findit/handlers/handlers_util.py:206: # The try job has been updated by swarming task: On 2016/03/28 22:27:22, stgao wrote: > I don't quite understand why try job is updated by Swarming task? You meant to > say "try job is triggerred after Swarming task completed"? > And if the try-job has a status, why we do not run the code below? I am actually saying that if try_job_info.get('status'), that means either there is something wrong with swarming task(no task found, no task triggered, task failed, etc) or swarming task has not completed. In either case, the will be no try job. https://codereview.chromium.org/1827903002/diff/180001/appengine/findit/handl... File appengine/findit/handlers/test/result_status_test.py (right): https://codereview.chromium.org/1827903002/diff/180001/appengine/findit/handl... appengine/findit/handlers/test/result_status_test.py:1: # Copyright 2016 The Chromium Authors. All rights reserved. On 2016/03/28 22:27:22, stgao wrote: > As result_status is straight-forward, we could skip this test if you want. Done. https://codereview.chromium.org/1827903002/diff/200001/appengine/findit/handl... File appengine/findit/handlers/handlers_util.py (right): https://codereview.chromium.org/1827903002/diff/200001/appengine/findit/handl... appengine/findit/handlers/handlers_util.py:112: if task.task_id: # Swarming rerun has started. On 2016/03/28 23:35:04, lijeffrey wrote: > nit: 2 spaces before # Done. https://codereview.chromium.org/1827903002/diff/200001/appengine/findit/handl... appengine/findit/handlers/handlers_util.py:206: if try_job_info.get('status'): On 2016/03/28 23:35:04, lijeffrey wrote: > this if can be combined into 1 if > > i.e. > > if (try_job_key != try_job_info['try_job_key'] or > try_job_info.get('status')): > continue Done. https://codereview.chromium.org/1827903002/diff/200001/appengine/findit/handl... appengine/findit/handlers/handlers_util.py:327: try_jobs.extend(additional_flakiness_list) On 2016/03/28 23:35:04, lijeffrey wrote: > nit: even if additional_flakiness_list is empty and not None, the if is not > needed as the extend will be the same Done. https://codereview.chromium.org/1827903002/diff/200001/appengine/findit/handl... appengine/findit/handlers/handlers_util.py:400: for step_name in failure_result_map: On 2016/03/28 23:35:04, lijeffrey wrote: > I would separate this out into a different function > IsStepFailure(failure_result_map) Since this logic is pretty straight forward, I will keep it as is. https://codereview.chromium.org/1827903002/diff/200001/appengine/findit/handl... appengine/findit/handlers/handlers_util.py:401: if step_name == 'compile': On 2016/03/28 23:35:04, lijeffrey wrote: > nit: just to be safe use step_name.lower() Done. https://codereview.chromium.org/1827903002/diff/200001/appengine/findit/handl... File appengine/findit/handlers/result_status.py (right): https://codereview.chromium.org/1827903002/diff/200001/appengine/findit/handl... appengine/findit/handlers/result_status.py:27: status_message_map = { On 2016/03/28 22:27:22, stgao wrote: > Make this as a constant var instead of a function? Done. https://codereview.chromium.org/1827903002/diff/200001/appengine/findit/handl... File appengine/findit/handlers/test/result_status_test.py (right): https://codereview.chromium.org/1827903002/diff/200001/appengine/findit/handl... appengine/findit/handlers/test/result_status_test.py:13: self.assertEqual( On 2016/03/28 23:35:04, lijeffrey wrote: > nit: 2 spaces File deleted
Description was changed from ========== [Findit] Modify handlers_util to prepare for the new UI change. Basically new changes will group swarming task data and try job data by step_name first, then the culprits. BUG=none ========== to ========== [Findit] Modify handlers_util to prepare for the new UI change. Basically new changes will group swarming task data and try job data by step_name first, then the culprits. BUG=598507 ==========
Patchset #13 (id:240001) has been deleted
The CQ bit was checked by stgao@chromium.org
lgtm https://codereview.chromium.org/1827903002/diff/260001/appengine/findit/handl... File appengine/findit/handlers/handlers_util.py (right): https://codereview.chromium.org/1827903002/diff/260001/appengine/findit/handl... appengine/findit/handlers/handlers_util.py:350: try_job_keys.update(step_try_job_keys) Just double check: should this be outside the for loop?
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1827903002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1827903002/260001
https://codereview.chromium.org/1827903002/diff/260001/appengine/findit/handl... File appengine/findit/handlers/handlers_util.py (right): https://codereview.chromium.org/1827903002/diff/260001/appengine/findit/handl... appengine/findit/handlers/handlers_util.py:350: try_job_keys.update(step_try_job_keys) On 2016/03/29 06:22:44, stgao wrote: > Just double check: should this be outside the for loop? Yes, logically it should be outside of this for loop. Although it should not affect the result, please help me fix it.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Infra Mac Tester on tryserver.infra (JOB_FAILED, https://build.chromium.org/p/tryserver.infra/builders/Infra%20Mac%20Tester/bu...)
The CQ bit was checked by stgao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1827903002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1827903002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Infra Mac Tester on tryserver.infra (JOB_FAILED, https://build.chromium.org/p/tryserver.infra/builders/Infra%20Mac%20Tester/bu...)
The CQ bit was checked by chanli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1827903002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1827903002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by chanli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stgao@chromium.org Link to the patchset: https://codereview.chromium.org/1827903002/#ps280001 (title: "remove possible flaky tests.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1827903002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1827903002/280001
Message was sent while issue was closed.
Description was changed from ========== [Findit] Modify handlers_util to prepare for the new UI change. Basically new changes will group swarming task data and try job data by step_name first, then the culprits. BUG=598507 ========== to ========== [Findit] Modify handlers_util to prepare for the new UI change. Basically new changes will group swarming task data and try job data by step_name first, then the culprits. BUG=598507 Committed: https://chromium.googlesource.com/infra/infra/+/7169a9d4560901db9d18df4037f99... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:280001) as https://chromium.googlesource.com/infra/infra/+/7169a9d4560901db9d18df4037f99...
Message was sent while issue was closed.
A side note: we'd better revisit the flaky tests you deleted later when you are back. There might be bugs behind them.
Message was sent while issue was closed.
https://codereview.chromium.org/1827903002/diff/260001/appengine/findit/handl... File appengine/findit/handlers/handlers_util.py (right): https://codereview.chromium.org/1827903002/diff/260001/appengine/findit/handl... appengine/findit/handlers/handlers_util.py:350: try_job_keys.update(step_try_job_keys) On 2016/03/29 06:27:59, chanli wrote: > On 2016/03/29 06:22:44, stgao wrote: > > Just double check: should this be outside the for loop? > > Yes, logically it should be outside of this for loop. Although it should not > affect the result, please help me fix it. Fixed in https://codereview.chromium.org/1866883002. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
