Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1833833002:
[Findit] Modify build_failure to prepare for new UI. (Closed)
Description[Findit] Modify build_failure to prepare for new UI.
BUG=598507
Committed: https://chromium.googlesource.com/infra/infra/+/92786e8dbdfc03b62202457c92866bd9557960ee
Patch Set 1 #Patch Set 2 : . #
Total comments: 6
Patch Set 3 : . #Patch Set 4 : Add comments. #Patch Set 5 : change new filter #Patch Set 6 : . #
Total comments: 25
Patch Set 7 : address comments #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 19 (5 generated)
chanli@chromium.org changed reviewers: + lijeffrey@chromium.org, stgao@chromium.org
PTAL
https://codereview.chromium.org/1833833002/diff/20001/appengine/findit/handle... File appengine/findit/handlers/build_failure.py (right): https://codereview.chromium.org/1833833002/diff/20001/appengine/findit/handle... appengine/findit/handlers/build_failure.py:6: from collections import defaultdict nit: order. https://codereview.chromium.org/1833833002/diff/20001/appengine/findit/handle... appengine/findit/handlers/build_failure.py:132: 'suspected_cls': group['suspected_cls'] Is it possible that two tests failed due to the same revisions, but start failing at different build cycles? How should we handle such a case? https://codereview.chromium.org/1833833002/diff/20001/appengine/findit/handle... appengine/findit/handlers/build_failure.py:139: def _UpdateAnalysisResultWithTryJobInfo( naming nit: Update -> Get?
https://codereview.chromium.org/1833833002/diff/20001/appengine/findit/handle... File appengine/findit/handlers/build_failure.py (right): https://codereview.chromium.org/1833833002/diff/20001/appengine/findit/handle... appengine/findit/handlers/build_failure.py:6: from collections import defaultdict On 2016/03/25 23:10:51, stgao wrote: > nit: order. Done. https://codereview.chromium.org/1833833002/diff/20001/appengine/findit/handle... appengine/findit/handlers/build_failure.py:132: 'suspected_cls': group['suspected_cls'] On 2016/03/25 23:10:51, stgao wrote: > Is it possible that two tests failed due to the same revisions, but start > failing at different build cycles? How should we handle such a case? If they failed due to the same revision, and failed in different cycle, it sounds just like what I described earlier that the revision broke a former flaky test. And they will not be grouped together in my opinion. Is there another possible reason for this scenario? https://codereview.chromium.org/1833833002/diff/20001/appengine/findit/handle... appengine/findit/handlers/build_failure.py:139: def _UpdateAnalysisResultWithTryJobInfo( On 2016/03/25 23:10:51, stgao wrote: > naming nit: Update -> Get? Done.
https://codereview.chromium.org/1833833002/diff/100001/appengine/findit/base_... File appengine/findit/base_handler.py (right): https://codereview.chromium.org/1833833002/diff/100001/appengine/findit/base_... appengine/findit/base_handler.py:14: from handlers import result_status It seems we could revert the change in this CL. https://codereview.chromium.org/1833833002/diff/100001/appengine/findit/handl... File appengine/findit/handlers/build_failure.py (right): https://codereview.chromium.org/1833833002/diff/100001/appengine/findit/handl... appengine/findit/handlers/build_failure.py:57: nit: empty line. https://codereview.chromium.org/1833833002/diff/100001/appengine/findit/handl... appengine/findit/handlers/build_failure.py:63: for failure in analysis_result.get('failures', []): Maybe rename ``failure`` to ``step_failure`` to improve readability or add a comment for this? https://codereview.chromium.org/1833833002/diff/100001/appengine/findit/handl... appengine/findit/handlers/build_failure.py:85: # Groups tests by suspected CLs' revision. Maybe explain what does the key in the dict mean? (It should be index of the test in the test list, right?) https://codereview.chromium.org/1833833002/diff/100001/appengine/findit/handl... appengine/findit/handlers/build_failure.py:113: # Found tests that have the same revision, add current test to group. "same revision" -> "same culprits"? Possibly update other places too. https://codereview.chromium.org/1833833002/diff/100001/appengine/findit/handl... appengine/findit/handlers/build_failure.py:130: for index, group in tests_group.iteritems(): How about a comment for what this logic block is for? https://codereview.chromium.org/1833833002/diff/100001/appengine/findit/handl... appengine/findit/handlers/build_failure.py:209: step_updated_results = update_results[step_name]['results'] To be consistent, should we rename ``update_results`` to ``updated_results``? https://codereview.chromium.org/1833833002/diff/100001/appengine/findit/handl... appengine/findit/handlers/build_failure.py:231: (indices['try_job_index'], indices['heuristic_index'])].append( What if the test has result only from heuristic or only from try-job? https://codereview.chromium.org/1833833002/diff/100001/appengine/findit/handl... appengine/findit/handlers/build_failure.py:234: for indices, tests in indices_test_map.iteritems(): For readability, how about below if it works? for (try_job_index, heuristic_index), tests in ...: https://codereview.chromium.org/1833833002/diff/100001/appengine/findit/handl... appengine/findit/handlers/build_failure.py:243: 'tests': tests if tests != ['non-swarming'] else [], Maybe make 'non-swarming' a singleton python object instead? In this way, there won't be conflict with test names. NON_SWARMING = object() data[NON_SWARMING] = some_value.
https://codereview.chromium.org/1833833002/diff/100001/appengine/findit/handl... File appengine/findit/handlers/build_failure.py (right): https://codereview.chromium.org/1833833002/diff/100001/appengine/findit/handl... appengine/findit/handlers/build_failure.py:70: # Non swraming, just group the whole step together. nit: swarming https://codereview.chromium.org/1833833002/diff/100001/appengine/findit/handl... appengine/findit/handlers/build_failure.py:209: step_updated_results = update_results[step_name]['results'] On 2016/03/28 23:29:26, stgao wrote: > To be consistent, should we rename ``update_results`` to ``updated_results``? +1 https://codereview.chromium.org/1833833002/diff/100001/appengine/findit/handl... appengine/findit/handlers/build_failure.py:220: for index, heuristic_result in enumerate(step_heuristic_results): nit: 1 blank line before this for
https://codereview.chromium.org/1833833002/diff/100001/appengine/findit/base_... File appengine/findit/base_handler.py (right): https://codereview.chromium.org/1833833002/diff/100001/appengine/findit/base_... appengine/findit/base_handler.py:14: from handlers import result_status On 2016/03/28 23:29:26, stgao wrote: > It seems we could revert the change in this CL. Done. https://codereview.chromium.org/1833833002/diff/100001/appengine/findit/handl... File appengine/findit/handlers/build_failure.py (right): https://codereview.chromium.org/1833833002/diff/100001/appengine/findit/handl... appengine/findit/handlers/build_failure.py:57: On 2016/03/28 23:29:26, stgao wrote: > nit: empty line. Done. https://codereview.chromium.org/1833833002/diff/100001/appengine/findit/handl... appengine/findit/handlers/build_failure.py:63: for failure in analysis_result.get('failures', []): On 2016/03/28 23:29:26, stgao wrote: > Maybe rename ``failure`` to ``step_failure`` to improve readability or add a > comment for this? Done. https://codereview.chromium.org/1833833002/diff/100001/appengine/findit/handl... appengine/findit/handlers/build_failure.py:70: # Non swraming, just group the whole step together. On 2016/03/29 00:29:38, lijeffrey wrote: > nit: swarming Done. https://codereview.chromium.org/1833833002/diff/100001/appengine/findit/handl... appengine/findit/handlers/build_failure.py:85: # Groups tests by suspected CLs' revision. On 2016/03/28 23:29:26, stgao wrote: > Maybe explain what does the key in the dict mean? > (It should be index of the test in the test list, right?) Done. https://codereview.chromium.org/1833833002/diff/100001/appengine/findit/handl... appengine/findit/handlers/build_failure.py:113: # Found tests that have the same revision, add current test to group. On 2016/03/28 23:29:26, stgao wrote: > "same revision" -> "same culprits"? Possibly update other places too. Done. https://codereview.chromium.org/1833833002/diff/100001/appengine/findit/handl... appengine/findit/handlers/build_failure.py:130: for index, group in tests_group.iteritems(): On 2016/03/28 23:29:26, stgao wrote: > How about a comment for what this logic block is for? Done. https://codereview.chromium.org/1833833002/diff/100001/appengine/findit/handl... appengine/findit/handlers/build_failure.py:209: step_updated_results = update_results[step_name]['results'] On 2016/03/28 23:29:26, stgao wrote: > To be consistent, should we rename ``update_results`` to ``updated_results``? Done. https://codereview.chromium.org/1833833002/diff/100001/appengine/findit/handl... appengine/findit/handlers/build_failure.py:220: for index, heuristic_result in enumerate(step_heuristic_results): On 2016/03/29 00:29:38, lijeffrey wrote: > nit: 1 blank line before this for Done. https://codereview.chromium.org/1833833002/diff/100001/appengine/findit/handl... appengine/findit/handlers/build_failure.py:231: (indices['try_job_index'], indices['heuristic_index'])].append( On 2016/03/28 23:29:26, stgao wrote: > What if the test has result only from heuristic or only from try-job? There should still be entries for those tests, just there would be no culprit in try job or suspected_cls=[] in heuristic results. https://codereview.chromium.org/1833833002/diff/100001/appengine/findit/handl... appengine/findit/handlers/build_failure.py:234: for indices, tests in indices_test_map.iteritems(): On 2016/03/28 23:29:26, stgao wrote: > For readability, how about below if it works? > > for (try_job_index, heuristic_index), tests in ...: Done. https://codereview.chromium.org/1833833002/diff/100001/appengine/findit/handl... appengine/findit/handlers/build_failure.py:243: 'tests': tests if tests != ['non-swarming'] else [], On 2016/03/28 23:29:26, stgao wrote: > Maybe make 'non-swarming' a singleton python object instead? In this way, there > won't be conflict with test names. > > NON_SWARMING = object() > > data[NON_SWARMING] = some_value. Done.
Description was changed from ========== [Findit] Modify build_failure to prepare for new UI. BUG=none ========== to ========== [Findit] Modify build_failure to prepare for new UI. BUG=598507 ==========
Patchset #7 (id:120001) has been deleted
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/1833833002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1833833002/140001
Message was sent while issue was closed.
Description was changed from ========== [Findit] Modify build_failure to prepare for new UI. BUG=598507 ========== to ========== [Findit] Modify build_failure to prepare for new UI. BUG=598507 Committed: https://chromium.googlesource.com/infra/infra/+/92786e8dbdfc03b62202457c92866... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001) as https://chromium.googlesource.com/infra/infra/+/92786e8dbdfc03b62202457c92866... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
