Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2179283009:
[Findit] Compare lists to lists instead of lists to tuples, and change tests (Closed)
DescriptionCompare lists to lists instead of lists to tuples, and change tests
BUG=632130
Committed: https://chromium.googlesource.com/infra/infra/+/9f0bd313ad2d8251f974e2f3764f76b87b21be63
Patch Set 1 #
Total comments: 8
Patch Set 2 : Slightly redo GenPotentialCulpritTupleList #Patch Set 3 : Nit function rename, update comment wording #
Total comments: 2
Messages
Total messages: 20 (10 generated)
Description was changed from ========== Compare lists to lists instead of lists to tuples, and change tests BUG= ========== to ========== Compare lists to lists instead of lists to tuples, and change tests BUG=632130 ==========
josiahk@google.com changed reviewers: + chanli@chromium.org, stgao@chromium.org
josiahk@google.com changed reviewers: + lijeffrey@chromium.org
Hello Chan and Jeff, I changed suspected_tuples members from tuples into lists. This allows for an accurate comparison to what is stored in ndb. I also fixed some tests, and made some new tests. Thanks! Josiah
https://codereview.chromium.org/2179283009/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/test/try_job_util_test.py (right): https://codereview.chromium.org/2179283009/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/test/try_job_util_test.py:439: self.assertTrue(WfFailureGroup.Get(master_name, builder_name, build_number)) WfFailureGroup isn't a bool, it's an entity. Instead assertNotNone https://codereview.chromium.org/2179283009/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/try_job_util.py (right): https://codereview.chromium.org/2179283009/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/try_job_util.py:208: def _GetMatchingGroup(wf_failure_groups, blame_list, suspected_tuples): a change like this is better tested with unit tests that call this function directly, once with suspected_tuples as an actual list of tuples and once with it list-ified by json
https://codereview.chromium.org/2179283009/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/try_job_util.py (right): https://codereview.chromium.org/2179283009/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/try_job_util.py:211: suspected_tuples = [list(tup) for tup in suspected_tuples] Instead of modifying it here, you can just change GenPotentialCulpritTupleList to directly produce the expected format. https://codereview.chromium.org/2179283009/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/try_job_util.py:274: existing_group.build_number]) Move the list to a separated line.
https://codereview.chromium.org/2179283009/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/test/try_job_util_test.py (right): https://codereview.chromium.org/2179283009/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/test/try_job_util_test.py:439: self.assertTrue(WfFailureGroup.Get(master_name, builder_name, build_number)) On 2016/07/27 22:49:36, lijeffrey wrote: > WfFailureGroup isn't a bool, it's an entity. Instead assertNotNone Thanks! https://codereview.chromium.org/2179283009/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/try_job_util.py (right): https://codereview.chromium.org/2179283009/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/try_job_util.py:208: def _GetMatchingGroup(wf_failure_groups, blame_list, suspected_tuples): On 2016/07/27 22:49:36, lijeffrey wrote: > a change like this is better tested with unit tests that call this function > directly, once with suspected_tuples as an actual list of tuples and once with > it list-ified by json Thanks! I changed the code to use lists instead of tuples. https://codereview.chromium.org/2179283009/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/try_job_util.py:211: suspected_tuples = [list(tup) for tup in suspected_tuples] On 2016/07/28 17:40:25, chanli wrote: > Instead of modifying it here, you can just change GenPotentialCulpritTupleList > to directly produce the expected format. Thank you. I changed GenPotentialCulpritTupleList (side note: renamed to GenSuspectedCLsWithFailures) to return list of lists instead of list of tuples. https://codereview.chromium.org/2179283009/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/try_job_util.py:274: existing_group.build_number]) On 2016/07/28 17:40:26, chanli wrote: > Move the list to a separated line. Thanks!
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
Hello Chan and Jeff! I would like to get this CL in before the next Findit update, so I can more accurately observe Build Failure grouping (through logs) in production. If you could PTAL, and LGTM if you think it does, I'd appreciate! :-) Josiah
lgtm
https://codereview.chromium.org/2179283009/diff/120001/appengine/findit/handl... File appengine/findit/handlers/triage_analysis.py (right): https://codereview.chromium.org/2179283009/diff/120001/appengine/findit/handl... appengine/findit/handlers/triage_analysis.py:44: suspected_cls_with_failures_1 = GetSuspectedCLsWithFailures(analysis_1.result) This seems not correct. For try_job_util.GetSuspectedCLsWithFailures, it only need to check heuristic results, but here we also need to check try job results https://codereview.chromium.org/2179283009/diff/120001/appengine/findit/water... File appengine/findit/waterfall/try_job_util.py (right): https://codereview.chromium.org/2179283009/diff/120001/appengine/findit/water... appengine/findit/waterfall/try_job_util.py:232: def _GetMatchingTestFailureGroups(failed_steps_and_tests): Have you done anything to make sure the order for the steps and tests?
On 2016/08/01 19:43:49, chanli wrote: > https://codereview.chromium.org/2179283009/diff/120001/appengine/findit/handl... > File appengine/findit/handlers/triage_analysis.py (right): > > https://codereview.chromium.org/2179283009/diff/120001/appengine/findit/handl... > appengine/findit/handlers/triage_analysis.py:44: suspected_cls_with_failures_1 = > GetSuspectedCLsWithFailures(analysis_1.result) > This seems not correct. For try_job_util.GetSuspectedCLsWithFailures, it only > need to check heuristic results, but here we also need to check try job results > > https://codereview.chromium.org/2179283009/diff/120001/appengine/findit/water... > File appengine/findit/waterfall/try_job_util.py (right): > > https://codereview.chromium.org/2179283009/diff/120001/appengine/findit/water... > appengine/findit/waterfall/try_job_util.py:232: def > _GetMatchingTestFailureGroups(failed_steps_and_tests): > Have you done anything to make sure the order for the steps and tests? Based on our discussion off line, LGTM
The CQ bit was checked by josiahk@google.com
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 ========== Compare lists to lists instead of lists to tuples, and change tests BUG=632130 ========== to ========== Compare lists to lists instead of lists to tuples, and change tests BUG=632130 Committed: https://chromium.googlesource.com/infra/infra/+/9f0bd313ad2d8251f974e2f3764f7... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:120001) as https://chromium.googlesource.com/infra/infra/+/9f0bd313ad2d8251f974e2f3764f7...
Message was sent while issue was closed.
Description was changed from ========== Compare lists to lists instead of lists to tuples, and change tests BUG=632130 Committed: https://chromium.googlesource.com/infra/infra/+/9f0bd313ad2d8251f974e2f3764f7... ========== to ========== Compare lists to lists instead of lists to tuples, and change tests BUG=632130 Committed: https://chromium.googlesource.com/infra/infra/+/9f0bd313ad2d8251f974e2f3764f7... ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
