Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2029873002:
[Findit] Cross-platform triage (Closed)
DescriptionEnable cross-platform triage of duplicate build analyses for Findit sheriffs, by triggering a single build analysis.
BUG=614478
Committed: https://chromium.googlesource.com/infra/infra/+/6815a5aa75d80c51d8d35c2ab8936da701d05cc8
Patch Set 1 #
Total comments: 47
Patch Set 2 : Test and style corrections #
Total comments: 45
Patch Set 3 : Some refactoring #
Total comments: 10
Patch Set 4 : Pacific time midnight matching analysis cutoff #
Total comments: 13
Patch Set 5 : Naming, periods, and whitespace. #
Dependent Patchsets: Messages
Total messages: 33 (9 generated)
josiahk@google.com changed reviewers: + chanli@chromium.org, stgao@chromium.org
chanli@chromium.org changed reviewers: + lijeffrey@chromium.org
Few more things: 1. Please clean up the issue description: it should be related to what the issue is and how to fix it. 2. Added lijeffrey@ as another reviewer. 3. I think you need to add more unit tests if code coverage is not 100% now. You can address my comments and upload another patch after your're done : ) https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... File appengine/findit/handlers/triage_analysis.py (right): https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:12: from datetime import datetime, timedelta Nit: Imports should be on separate lines. Please refer to https://engdoc.corp.google.com/eng/doc/devguide/py/styleguide.shtml?cl=head#s... https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:23: # TODO: Is there a better place to put this constant? I think we can just keep this constant here. https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:25: # results. Please remove these TODOs before you committing the code. Same as others https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:28: def genPotentialCulpritTupleList(analysis): Function name nit: the convention for function names is FunctionName or _FunctionName(private functions). Please refer to: https://engdoc.corp.google.com/eng/doc/devguide/py/styleguide.shtml?cl=head#s... https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:29: potentialCulpritTupleList = [] Naming nit: variable names should be like 'var_name' Please refer to: https://engdoc.corp.google.com/eng/doc/devguide/py/styleguide.shtml?cl=head#s... https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:31: # Iterate through the failures, tests, and suspected_cls, appending potential Nit: Iterates https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:33: # the list Nit: ending . https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:43: if "suspected_cls" in failure: Shouldn't here be elif? https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:51: def doAnalysesMatch(analysis1, analysis2): argument name nits Please refer to: https://engdoc.corp.google.com/eng/doc/devguide/py/styleguide.shtml?cl=head#s... https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:57: if not analysis1.result['failures'] or not analysis2.result['failures']: analysis1.result.get('failures') or not analysis2.result.get('failures') to avoid keyerror https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:74: def doAnalysesMatchTests(): If this is a test for you to monitor some middle results, please remove it before you committing the code. Otherwise move it to ./test/triage_analysis_test.py - all unit tests for this handler should be there. https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:137: # @ndb.transactional Just a thought: I didn't do the experiment but it may work: @ndb.transactional def _UpdateAnalysisResultStatus(master_name, builder_name, build_number, correct, user_name=None): # get analysis by master_name, builder_name, build_number # save the triage result for this analysis only # return analysis @ndb.transactional def _TriageDuplicateResults(analysis): # query for all results within MATCHING_ANALYSIS_DAYS_AGO # get duplicates # modify result status for those duplicates def HandlePost(self): success, analysis = _UpdateAnalysisResultStatus(master_name, builder_name, build_number, correct, user_name) _TriageDuplicateResults(analysis) https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:140: doAnalysesMatchTests() Move this to ./test/triage_analysis_test.py https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:146: # TODO: Limit query range to only yesterday and not today and yesterday. To do it, you can also specify end date https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:154: ndb.OR( IMO, You should only query for FOUND_UNTRIAGED here https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:159: -WfAnalysis.build_start_time).fetch() No need to order https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:170: for i, a in enumerate(analysis_results): Use descriptive variable names
Posted a couple of comments. Will take another look in a later patchset. https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... File appengine/findit/handlers/triage_analysis.py (right): https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:26: MATCHING_ANALYSIS_DAYS_AGO = 7 Leaving it here for this CL is fine. But seems better to put in the config ultimately. https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:92: u'3cf9343f4602d4ec11717cb6ff56a793c1d5f84b', u'url': Format and move the test to the corresponding unittest? https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:137: # @ndb.transactional As we are now dealing with multiple NDB entities, transaction might have to be removed. Cross-NDB-group transaction is expensive and has limitation to the number of NDB groups. You may still try and experiment the usage of transaction.
In general you can run gpylint <path to your file> before submitting for review and it will catch a lot of the formatting issues in terms of indentation, variable name style, etc. https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... File appengine/findit/handlers/triage_analysis.py (right): https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:34: for failure in analysis.result["failures"]: not sure if this has been mentioned in someone else's comments but use single quotes when passing strings, i.e. result['failures'] https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:40: failure["step_name"], 4 spaces if putting arguments to a function call on a new line, same for the rest of the code
Here are some responses to chromium code review findings. There are other chromium code review findings I still need to address! https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... File appengine/findit/handlers/triage_analysis.py (right): https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:12: from datetime import datetime, timedelta On 2016/06/02 00:55:31, chanli wrote: > Nit: Imports should be on separate lines. > > Please refer to > https://engdoc.corp.google.com/eng/doc/devguide/py/styleguide.shtml?cl=head#s... Done. https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:23: # TODO: Is there a better place to put this constant? On 2016/06/02 00:55:31, chanli wrote: > I think we can just keep this constant here. Done. https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:28: def genPotentialCulpritTupleList(analysis): On 2016/06/02 00:55:31, chanli wrote: > Function name nit: > the convention for function names is FunctionName or _FunctionName(private > functions). > > > Please refer to: > https://engdoc.corp.google.com/eng/doc/devguide/py/styleguide.shtml?cl=head#s... Done. https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:31: # Iterate through the failures, tests, and suspected_cls, appending potential On 2016/06/02 00:55:31, chanli wrote: > Nit: Iterates Done. https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:33: # the list On 2016/06/02 00:55:31, chanli wrote: > Nit: ending . Done. https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:34: for failure in analysis.result["failures"]: On 2016/06/06 23:00:59, lijeffrey wrote: > not sure if this has been mentioned in someone else's comments but use single > quotes when passing strings, i.e. result['failures'] Done. https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:43: if "suspected_cls" in failure: On 2016/06/02 00:55:30, chanli wrote: > Shouldn't here be elif? I don't think so, but maybe you see a case I don't see. If "suspected_cls" is not in failure, then no additional potential culprit tuples get added to the list. https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:51: def doAnalysesMatch(analysis1, analysis2): On 2016/06/02 00:55:31, chanli wrote: > argument name nits > > Please refer to: > https://engdoc.corp.google.com/eng/doc/devguide/py/styleguide.shtml?cl=head#s... I have renamed to analysis_1 and analysis_2. Is this good? https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:57: if not analysis1.result['failures'] or not analysis2.result['failures']: On 2016/06/02 00:55:31, chanli wrote: > analysis1.result.get('failures') or not analysis2.result.get('failures') to > avoid keyerror Ah, thanks! Also, could I use this instead? if 'failures' not in analysis_1.result https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:137: # @ndb.transactional On 2016/06/02 00:55:31, chanli wrote: > Just a thought: I didn't do the experiment but it may work: > @ndb.transactional > def _UpdateAnalysisResultStatus(master_name, builder_name, build_number, > correct, user_name=None): > # get analysis by master_name, builder_name, build_number > # save the triage result for this analysis only > # return analysis > > @ndb.transactional > def _TriageDuplicateResults(analysis): > # query for all results within MATCHING_ANALYSIS_DAYS_AGO > # get duplicates > # modify result status for those duplicates > > > def HandlePost(self): > success, analysis = _UpdateAnalysisResultStatus(master_name, builder_name, > build_number, correct, user_name) > _TriageDuplicateResults(analysis) > > Yay! The concept of your idea works! Except... _TriageDuplicateResults() still throws the error when @ndb.transactional is present: "BadRequestError: Only ancestor queries are allowed inside transactions." https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:146: # TODO: Limit query range to only yesterday and not today and yesterday. On 2016/06/02 00:55:31, chanli wrote: > To do it, you can also specify end date Done. https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:159: -WfAnalysis.build_start_time).fetch() On 2016/06/02 00:55:31, chanli wrote: > No need to order Done.
On 2016/06/07 18:34:41, josiahk wrote: > Here are some responses to chromium code review findings. There are other > chromium code review findings I still need to address! > > https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... > File appengine/findit/handlers/triage_analysis.py (right): > > https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... > appengine/findit/handlers/triage_analysis.py:12: from datetime import datetime, > timedelta > On 2016/06/02 00:55:31, chanli wrote: > > Nit: Imports should be on separate lines. > > > > Please refer to > > > https://engdoc.corp.google.com/eng/doc/devguide/py/styleguide.shtml?cl=head#s... > > Done. > > https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... > appengine/findit/handlers/triage_analysis.py:23: # TODO: Is there a better place > to put this constant? > On 2016/06/02 00:55:31, chanli wrote: > > I think we can just keep this constant here. > > Done. > > https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... > appengine/findit/handlers/triage_analysis.py:28: def > genPotentialCulpritTupleList(analysis): > On 2016/06/02 00:55:31, chanli wrote: > > Function name nit: > > the convention for function names is FunctionName or _FunctionName(private > > functions). > > > > > > Please refer to: > > > https://engdoc.corp.google.com/eng/doc/devguide/py/styleguide.shtml?cl=head#s... > > Done. > > https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... > appengine/findit/handlers/triage_analysis.py:31: # Iterate through the failures, > tests, and suspected_cls, appending potential > On 2016/06/02 00:55:31, chanli wrote: > > Nit: Iterates > > Done. > > https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... > appengine/findit/handlers/triage_analysis.py:33: # the list > On 2016/06/02 00:55:31, chanli wrote: > > Nit: ending . > > Done. > > https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... > appengine/findit/handlers/triage_analysis.py:34: for failure in > analysis.result["failures"]: > On 2016/06/06 23:00:59, lijeffrey wrote: > > not sure if this has been mentioned in someone else's comments but use single > > quotes when passing strings, i.e. result['failures'] > > Done. > > https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... > appengine/findit/handlers/triage_analysis.py:43: if "suspected_cls" in failure: > On 2016/06/02 00:55:30, chanli wrote: > > Shouldn't here be elif? > > I don't think so, but maybe you see a case I don't see. If "suspected_cls" is > not in failure, then no additional potential culprit tuples get added to the > list. > > https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... > appengine/findit/handlers/triage_analysis.py:51: def doAnalysesMatch(analysis1, > analysis2): > On 2016/06/02 00:55:31, chanli wrote: > > argument name nits > > > > Please refer to: > > > https://engdoc.corp.google.com/eng/doc/devguide/py/styleguide.shtml?cl=head#s... > > I have renamed to analysis_1 and analysis_2. Is this good? > > https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... > appengine/findit/handlers/triage_analysis.py:57: if not > analysis1.result['failures'] or not analysis2.result['failures']: > On 2016/06/02 00:55:31, chanli wrote: > > analysis1.result.get('failures') or not analysis2.result.get('failures') to > > avoid keyerror > > Ah, thanks! Also, could I use this instead? > > if 'failures' not in analysis_1.result > > https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... > appengine/findit/handlers/triage_analysis.py:137: # @ndb.transactional > On 2016/06/02 00:55:31, chanli wrote: > > Just a thought: I didn't do the experiment but it may work: > > @ndb.transactional > > def _UpdateAnalysisResultStatus(master_name, builder_name, build_number, > > correct, user_name=None): > > # get analysis by master_name, builder_name, build_number > > # save the triage result for this analysis only > > # return analysis > > > > @ndb.transactional > > def _TriageDuplicateResults(analysis): > > # query for all results within MATCHING_ANALYSIS_DAYS_AGO > > # get duplicates > > # modify result status for those duplicates > > > > > > def HandlePost(self): > > success, analysis = _UpdateAnalysisResultStatus(master_name, builder_name, > > build_number, correct, user_name) > > _TriageDuplicateResults(analysis) > > > > > > Yay! The concept of your idea works! > Except... _TriageDuplicateResults() still throws the error when > @ndb.transactional is present: "BadRequestError: Only ancestor queries are > allowed inside transactions." > > https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... > appengine/findit/handlers/triage_analysis.py:146: # TODO: Limit query range to > only yesterday and not today and yesterday. > On 2016/06/02 00:55:31, chanli wrote: > > To do it, you can also specify end date > > Done. > > https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... > appengine/findit/handlers/triage_analysis.py:159: > -WfAnalysis.build_start_time).fetch() > On 2016/06/02 00:55:31, chanli wrote: > > No need to order > > Done. Hi Josiah, I only see one patch here. Did you forget to upload the second patch?
Description was changed from ========== First draft at enabling cross-platform* triage for Findit sheriffs There are still some TODOs in this code which I will go back and take care of later-- but feel free to comment on them as well! *cross-platform? cross-builder? cross-build-analysis? Thanks! --Josiah Commit log: Shortened/prettyied code Made code easier for development by expanding build analysis query Used hard coded value instead of constant More TODO comments Cleaned up code, plus another thing Mark matching build analysis Nit Another test More tests Pretty output Preliminary matching works! Initial build analysis matching code BUG=614478 ========== to ========== First draft at enabling cross-platform triage for Findit sheriffs BUG=614478 ==========
Description was changed from ========== First draft at enabling cross-platform triage for Findit sheriffs BUG=614478 ========== to ========== First draft at enabling cross-platform triage for Findit sheriffs Enables Find-it sherrifs to triage similar build analyses by triaging one build analysis. BUG=614478 ==========
Hope you guys are having fun in Kirkland! https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... File appengine/findit/handlers/triage_analysis.py (right): https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:25: # results. On 2016/06/02 00:55:31, chanli wrote: > Please remove these TODOs before you committing the code. > > Same as others Done. https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:26: MATCHING_ANALYSIS_DAYS_AGO = 7 On 2016/06/03 07:09:34, stgao wrote: > Leaving it here for this CL is fine. But seems better to put in the config > ultimately. Acknowledged. https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:29: potentialCulpritTupleList = [] On 2016/06/02 00:55:31, chanli wrote: > Naming nit: variable names should be like 'var_name' > > Please refer to: > https://engdoc.corp.google.com/eng/doc/devguide/py/styleguide.shtml?cl=head#s... Done. https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:40: failure["step_name"], On 2016/06/06 23:00:59, lijeffrey wrote: > 4 spaces if putting arguments to a function call on a new line, same for the > rest of the code Done. https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:43: if "suspected_cls" in failure: On 2016/06/07 18:34:41, josiahk wrote: > On 2016/06/02 00:55:30, chanli wrote: > > Shouldn't here be elif? > > I don't think so, but maybe you see a case I don't see. If "suspected_cls" is > not in failure, then no additional potential culprit tuples get added to the > list. Fixed with 'else'. https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:51: def doAnalysesMatch(analysis1, analysis2): On 2016/06/07 18:34:41, josiahk wrote: > On 2016/06/02 00:55:31, chanli wrote: > > argument name nits > > > > Please refer to: > > > https://engdoc.corp.google.com/eng/doc/devguide/py/styleguide.shtml?cl=head#s... > > I have renamed to analysis_1 and analysis_2. Is this good? Done. https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:57: if not analysis1.result['failures'] or not analysis2.result['failures']: On 2016/06/07 18:34:41, josiahk wrote: > On 2016/06/02 00:55:31, chanli wrote: > > analysis1.result.get('failures') or not analysis2.result.get('failures') to > > avoid keyerror > > Ah, thanks! Also, could I use this instead? > > if 'failures' not in analysis_1.result Relevant code removed. https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:74: def doAnalysesMatchTests(): On 2016/06/02 00:55:31, chanli wrote: > If this is a test for you to monitor some middle results, please remove it > before you committing the code. Otherwise move it to > ./test/triage_analysis_test.py - all unit tests for this handler should be > there. Done. https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:92: u'3cf9343f4602d4ec11717cb6ff56a793c1d5f84b', u'url': On 2016/06/03 07:09:34, stgao wrote: > Format and move the test to the corresponding unittest? Done. https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:140: doAnalysesMatchTests() On 2016/06/02 00:55:31, chanli wrote: > Move this to ./test/triage_analysis_test.py Done. https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:154: ndb.OR( On 2016/06/02 00:55:31, chanli wrote: > IMO, You should only query for FOUND_UNTRIAGED here Done. https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:170: for i, a in enumerate(analysis_results): On 2016/06/02 00:55:31, chanli wrote: > Use descriptive variable names Relevant code removed.
On 2016/06/10 18:34:26, josiahk wrote: > Hope you guys are having fun in Kirkland! > > https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... > File appengine/findit/handlers/triage_analysis.py (right): > > https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... > appengine/findit/handlers/triage_analysis.py:25: # results. > On 2016/06/02 00:55:31, chanli wrote: > > Please remove these TODOs before you committing the code. > > > > Same as others > > Done. > > https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... > appengine/findit/handlers/triage_analysis.py:26: MATCHING_ANALYSIS_DAYS_AGO = 7 > On 2016/06/03 07:09:34, stgao wrote: > > Leaving it here for this CL is fine. But seems better to put in the config > > ultimately. > > Acknowledged. > > https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... > appengine/findit/handlers/triage_analysis.py:29: potentialCulpritTupleList = [] > On 2016/06/02 00:55:31, chanli wrote: > > Naming nit: variable names should be like 'var_name' > > > > Please refer to: > > > https://engdoc.corp.google.com/eng/doc/devguide/py/styleguide.shtml?cl=head#s... > > Done. > > https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... > appengine/findit/handlers/triage_analysis.py:40: failure["step_name"], > On 2016/06/06 23:00:59, lijeffrey wrote: > > 4 spaces if putting arguments to a function call on a new line, same for the > > rest of the code > > Done. > > https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... > appengine/findit/handlers/triage_analysis.py:43: if "suspected_cls" in failure: > On 2016/06/07 18:34:41, josiahk wrote: > > On 2016/06/02 00:55:30, chanli wrote: > > > Shouldn't here be elif? > > > > I don't think so, but maybe you see a case I don't see. If "suspected_cls" is > > not in failure, then no additional potential culprit tuples get added to the > > list. > > Fixed with 'else'. > > https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... > appengine/findit/handlers/triage_analysis.py:51: def doAnalysesMatch(analysis1, > analysis2): > On 2016/06/07 18:34:41, josiahk wrote: > > On 2016/06/02 00:55:31, chanli wrote: > > > argument name nits > > > > > > Please refer to: > > > > > > https://engdoc.corp.google.com/eng/doc/devguide/py/styleguide.shtml?cl=head#s... > > > > I have renamed to analysis_1 and analysis_2. Is this good? > > Done. > > https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... > appengine/findit/handlers/triage_analysis.py:57: if not > analysis1.result['failures'] or not analysis2.result['failures']: > On 2016/06/07 18:34:41, josiahk wrote: > > On 2016/06/02 00:55:31, chanli wrote: > > > analysis1.result.get('failures') or not analysis2.result.get('failures') to > > > avoid keyerror > > > > Ah, thanks! Also, could I use this instead? > > > > if 'failures' not in analysis_1.result > > Relevant code removed. > > https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... > appengine/findit/handlers/triage_analysis.py:74: def doAnalysesMatchTests(): > On 2016/06/02 00:55:31, chanli wrote: > > If this is a test for you to monitor some middle results, please remove it > > before you committing the code. Otherwise move it to > > ./test/triage_analysis_test.py - all unit tests for this handler should be > > there. > > Done. > > https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... > appengine/findit/handlers/triage_analysis.py:92: > u'3cf9343f4602d4ec11717cb6ff56a793c1d5f84b', u'url': > On 2016/06/03 07:09:34, stgao wrote: > > Format and move the test to the corresponding unittest? > > Done. > > https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... > appengine/findit/handlers/triage_analysis.py:140: doAnalysesMatchTests() > On 2016/06/02 00:55:31, chanli wrote: > > Move this to ./test/triage_analysis_test.py > > Done. > > https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... > appengine/findit/handlers/triage_analysis.py:154: ndb.OR( > On 2016/06/02 00:55:31, chanli wrote: > > IMO, You should only query for FOUND_UNTRIAGED here > > Done. > > https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... > appengine/findit/handlers/triage_analysis.py:170: for i, a in > enumerate(analysis_results): > On 2016/06/02 00:55:31, chanli wrote: > > Use descriptive variable names > > Relevant code removed. ...And, I am ready for you guys to review my second patchset. I apologize for only having the text about you guys having fun in Kirkland (though I hope you do)!
https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/.gitig... File appengine/findit/.gitignore (right): https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/.gitig... appengine/findit/.gitignore:8: findit.sublime-workspace what's this? https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... File appengine/findit/handlers/test/triage_analysis_test.py (right): https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... appengine/findit/handlers/test/triage_analysis_test.py:8: from datetime import datetime nit: i would move these imports to the very top https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... appengine/findit/handlers/test/triage_analysis_test.py:138: analysis_with_empty_failures = WfAnalysis.Create( i would create these and modify them in the order they're created. i.e. analysis_with_empty_failiures = WfAnalysis.Create(...) analysis_with_empty_failures.result = {...} analysis_with_no_suspected_cls = ... analysis_with_no_suspected_Cls.result = ... ... https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... appengine/findit/handlers/test/triage_analysis_test.py:155: # Set time to yesterday nit: comment ends with . https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... appengine/findit/handlers/test/triage_analysis_test.py:156: build_start_time = datetime.utcnow() - timedelta( if it's yesterday you can do datetime.utcnow() - timedelta(days=1) https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... appengine/findit/handlers/test/triage_analysis_test.py:326: # Empty failures list nit: all comments should end with . https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... appengine/findit/handlers/test/triage_analysis_test.py:356: triage_analysis._TriageDuplicateResults(analysis_with_suspected_cls_3, True) why not just do this instead of leaving it in TODO? https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... File appengine/findit/handlers/triage_analysis.py (right): https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:86: if (sorted(potential_culprit_tuple_list_1) != you can just do return sorted(potential_culprit_tuple_list_1) == sorted(potential_culprit_tuple_list_2) https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:97: analysis: The analysis to which to append the history record . after sentences. https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:140: # TODO: Add @ndb.transactional? do some research as to what ndb.transactional means and determine whether or not it belongs here. https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:142: # QUESTION: Instead of looking for analyses with start_dates relative to when you find out the answer take this comment out before committing the code. In general just ask instead of putting comments like this in the code, even if it's just for review https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:162: for a in matching_analyses: nit: use meaningful variable names https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:165: # QUESTION: Should this function ever return anything but True? see lines 133-134 https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:193: success = not not success_and_maybe_analysis and _TriageDuplicateResults( not not shouldn't be necessary
https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... File appengine/findit/handlers/test/triage_analysis_test.py (right): https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... appengine/findit/handlers/test/triage_analysis_test.py:9: from datetime import timedelta Move this one to top as well. https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... appengine/findit/handlers/test/triage_analysis_test.py:138: analysis_with_empty_failures = WfAnalysis.Create( On 2016/06/13 18:15:05, lijeffrey wrote: > i would create these and modify them in the order they're created. > > i.e. > > analysis_with_empty_failiures = WfAnalysis.Create(...) > analysis_with_empty_failures.result = {...} > > analysis_with_no_suspected_cls = ... > analysis_with_no_suspected_Cls.result = ... > > ... +1 analysis_with_empty_failiures = WfAnalysis.Create(...) analysis_with_empty_failures.result = {...} analysis_with_empty_failures.put() https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... appengine/findit/handlers/test/triage_analysis_test.py:155: # Set time to yesterday nit: 'Sets' https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... appengine/findit/handlers/test/triage_analysis_test.py:356: triage_analysis._TriageDuplicateResults(analysis_with_suspected_cls_3, True) On 2016/06/13 18:15:05, lijeffrey wrote: > why not just do this instead of leaving it in TODO? +1 https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... File appengine/findit/handlers/triage_analysis.py (right): https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:13: from datetime import timedelta nit: need an empty line below. https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:18: from waterfall import buildbot Nit: move ln 14 - 18 under ln 22. And also leave 2 lines blank between 'from waterfall import buildbot' and 'MATCHING_ANALYSIS_DAYS_AGO_START = 1'. https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:142: # QUESTION: Instead of looking for analyses with start_dates relative to I think your idea makes sense, but how relative would it be? https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:148: end_date = datetime.utcnow() - timedelta(MATCHING_ANALYSIS_DAYS_AGO_END - 1) Why didn't make MATCHING_ANALYSIS_DAYS_AGO_END = 0 and just use MATCHING_ANALYSIS_DAYS_AGO_END here? https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:193: success = not not success_and_maybe_analysis and _TriageDuplicateResults( Based your current change, how could you display the original analysis link to the duplicates?
https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... File appengine/findit/handlers/triage_analysis.py (right): https://codereview.chromium.org/2029873002/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:137: # @ndb.transactional On 2016/06/07 18:34:41, josiahk wrote: > On 2016/06/02 00:55:31, chanli wrote: > > Just a thought: I didn't do the experiment but it may work: > > @ndb.transactional > > def _UpdateAnalysisResultStatus(master_name, builder_name, build_number, > > correct, user_name=None): > > # get analysis by master_name, builder_name, build_number > > # save the triage result for this analysis only > > # return analysis > > > > @ndb.transactional > > def _TriageDuplicateResults(analysis): > > # query for all results within MATCHING_ANALYSIS_DAYS_AGO > > # get duplicates > > # modify result status for those duplicates > > > > > > def HandlePost(self): > > success, analysis = _UpdateAnalysisResultStatus(master_name, builder_name, > > build_number, correct, user_name) > > _TriageDuplicateResults(analysis) > > > > > > Yay! The concept of your idea works! > Except... _TriageDuplicateResults() still throws the error when > @ndb.transactional is present: "BadRequestError: Only ancestor queries are > allowed inside transactions." > That's due to Cross-NDB-group query. I had a comment above on this. https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... File appengine/findit/handlers/triage_analysis.py (right): https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:21: from google.appengine.ext import ndb for imports, there is an order for different group: standard python lib third-party python lib app-specific python lib You may refer to the python style guide line for more details. https://google.github.io/styleguide/pyguide.html https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:144: # date of the original analysis? Should it be relative to the wf_analysis.WfAnalysis.build_start_time instead?
https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/.gitig... File appengine/findit/.gitignore (right): https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/.gitig... appengine/findit/.gitignore:8: findit.sublime-workspace On 2016/06/13 18:15:05, lijeffrey wrote: > what's this? It's removed! :-) https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... File appengine/findit/handlers/test/triage_analysis_test.py (right): https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... appengine/findit/handlers/test/triage_analysis_test.py:8: from datetime import datetime On 2016/06/13 18:15:05, lijeffrey wrote: > nit: i would move these imports to the very top Done. https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... appengine/findit/handlers/test/triage_analysis_test.py:9: from datetime import timedelta On 2016/06/13 20:21:03, chanli wrote: > Move this one to top as well. Done. https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... appengine/findit/handlers/test/triage_analysis_test.py:138: analysis_with_empty_failures = WfAnalysis.Create( On 2016/06/13 18:15:05, lijeffrey wrote: > i would create these and modify them in the order they're created. > > i.e. > > analysis_with_empty_failiures = WfAnalysis.Create(...) > analysis_with_empty_failures.result = {...} > > analysis_with_no_suspected_cls = ... > analysis_with_no_suspected_Cls.result = ... > > ... Done. https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... appengine/findit/handlers/test/triage_analysis_test.py:155: # Set time to yesterday On 2016/06/13 20:21:03, chanli wrote: > nit: 'Sets' Thanks! https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... appengine/findit/handlers/test/triage_analysis_test.py:156: build_start_time = datetime.utcnow() - timedelta( On 2016/06/13 18:15:05, lijeffrey wrote: > if it's yesterday you can do > > datetime.utcnow() - timedelta(days=1) Thanks! https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... appengine/findit/handlers/test/triage_analysis_test.py:326: # Empty failures list On 2016/06/13 18:15:06, lijeffrey wrote: > nit: all comments should end with . Done. https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... appengine/findit/handlers/test/triage_analysis_test.py:356: triage_analysis._TriageDuplicateResults(analysis_with_suspected_cls_3, True) On 2016/06/13 18:15:05, lijeffrey wrote: > why not just do this instead of leaving it in TODO? There is now a testTriageDuplicateResults test, though it does a bit of a different thing. https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... File appengine/findit/handlers/triage_analysis.py (right): https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:13: from datetime import timedelta On 2016/06/13 20:21:04, chanli wrote: > nit: need an empty line below. Done. https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:18: from waterfall import buildbot On 2016/06/13 20:21:04, chanli wrote: > Nit: move ln 14 - 18 under ln 22. And also leave 2 lines blank between 'from > waterfall import buildbot' and 'MATCHING_ANALYSIS_DAYS_AGO_START = 1'. Done. https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:21: from google.appengine.ext import ndb On 2016/06/13 23:39:49, stgao wrote: > for imports, there is an order for different group: > > standard python lib > > third-party python lib > > app-specific python lib > > You may refer to the python style guide line for more details. > https://google.github.io/styleguide/pyguide.html Thanks! https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:86: if (sorted(potential_culprit_tuple_list_1) != On 2016/06/13 18:15:06, lijeffrey wrote: > you can just do > > return sorted(potential_culprit_tuple_list_1) == > sorted(potential_culprit_tuple_list_2) Done. Thanks! https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:97: analysis: The analysis to which to append the history record On 2016/06/13 18:15:06, lijeffrey wrote: > . after sentences. Done. https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:142: # QUESTION: Instead of looking for analyses with start_dates relative to On 2016/06/13 18:15:06, lijeffrey wrote: > when you find out the answer take this comment out before committing the code. > In general just ask instead of putting comments like this in the code, even if > it's just for review Thanks! I have taken the comment out. I also talked with Chan, and wrote code and tests that addressed my question. https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:144: # date of the original analysis? On 2016/06/13 23:39:49, stgao wrote: > Should it be relative to the wf_analysis.WfAnalysis.build_start_time instead? Yes! Thank you! https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:148: end_date = datetime.utcnow() - timedelta(MATCHING_ANALYSIS_DAYS_AGO_END - 1) On 2016/06/13 20:21:03, chanli wrote: > Why didn't make MATCHING_ANALYSIS_DAYS_AGO_END = 0 and just use > MATCHING_ANALYSIS_DAYS_AGO_END here? Weird code removed. Underlying logic changed. https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:162: for a in matching_analyses: On 2016/06/13 18:15:06, lijeffrey wrote: > nit: use meaningful variable names Done. https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:165: # QUESTION: Should this function ever return anything but True? On 2016/06/13 18:15:06, lijeffrey wrote: > see lines 133-134 Thanks! https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:193: success = not not success_and_maybe_analysis and _TriageDuplicateResults( On 2016/06/13 18:15:06, lijeffrey wrote: > not not shouldn't be necessary Done. https://codereview.chromium.org/2029873002/diff/20001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:193: success = not not success_and_maybe_analysis and _TriageDuplicateResults( On 2016/06/13 20:21:04, chanli wrote: > Based your current change, how could you display the original analysis link to > the duplicates? The original analysis link information will hopefully go into a new ndb field of WfAnalysis. Some of this code may need to be modified to support displaying duplicate analyses information, which I may do for the UI task.
https://codereview.chromium.org/2029873002/diff/40001/appengine/findit/handle... File appengine/findit/handlers/test/triage_analysis_test.py (right): https://codereview.chromium.org/2029873002/diff/40001/appengine/findit/handle... appengine/findit/handlers/test/triage_analysis_test.py:16: from model import analysis_status Nit: you need to adjust the order of ln 14-16 https://codereview.chromium.org/2029873002/diff/40001/appengine/findit/handle... File appengine/findit/handlers/triage_analysis.py (right): https://codereview.chromium.org/2029873002/diff/40001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:96: correct: True if the history record should indicate a correct judgement, Nit: this argument is now 'is_correct' now. https://codereview.chromium.org/2029873002/diff/40001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:146: midnight = datetime.utcnow().replace( I think 'midnight' should refer to local(PST) midnight instead of UTC midnight.
Midnight matching analysis cutoff is now in Pacific Time instead of UTC https://codereview.chromium.org/2029873002/diff/40001/appengine/findit/handle... File appengine/findit/handlers/test/triage_analysis_test.py (right): https://codereview.chromium.org/2029873002/diff/40001/appengine/findit/handle... appengine/findit/handlers/test/triage_analysis_test.py:16: from model import analysis_status On 2016/06/21 18:32:34, chanli wrote: > Nit: you need to adjust the order of ln 14-16 Done. https://codereview.chromium.org/2029873002/diff/40001/appengine/findit/handle... File appengine/findit/handlers/triage_analysis.py (right): https://codereview.chromium.org/2029873002/diff/40001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:96: correct: True if the history record should indicate a correct judgement, On 2016/06/21 18:32:34, chanli wrote: > Nit: this argument is now 'is_correct' now. Done. https://codereview.chromium.org/2029873002/diff/40001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:146: midnight = datetime.utcnow().replace( On 2016/06/21 18:32:34, chanli wrote: > I think 'midnight' should refer to local(PST) midnight instead of UTC midnight. Done!
On 2016/06/23 22:41:03, josiahk wrote: > Midnight matching analysis cutoff is now in Pacific Time instead of UTC > > https://codereview.chromium.org/2029873002/diff/40001/appengine/findit/handle... > File appengine/findit/handlers/test/triage_analysis_test.py (right): > > https://codereview.chromium.org/2029873002/diff/40001/appengine/findit/handle... > appengine/findit/handlers/test/triage_analysis_test.py:16: from model import > analysis_status > On 2016/06/21 18:32:34, chanli wrote: > > Nit: you need to adjust the order of ln 14-16 > > Done. > > https://codereview.chromium.org/2029873002/diff/40001/appengine/findit/handle... > File appengine/findit/handlers/triage_analysis.py (right): > > https://codereview.chromium.org/2029873002/diff/40001/appengine/findit/handle... > appengine/findit/handlers/triage_analysis.py:96: correct: True if the history > record should indicate a correct judgement, > On 2016/06/21 18:32:34, chanli wrote: > > Nit: this argument is now 'is_correct' now. > > Done. > > https://codereview.chromium.org/2029873002/diff/40001/appengine/findit/handle... > appengine/findit/handlers/triage_analysis.py:146: midnight = > datetime.utcnow().replace( > On 2016/06/21 18:32:34, chanli wrote: > > I think 'midnight' should refer to local(PST) midnight instead of UTC > midnight. > > Done! lgtm. Please wait for Shuotao and Jeff's review.
lgtm https://codereview.chromium.org/2029873002/diff/60001/appengine/findit/handle... File appengine/findit/handlers/triage_analysis.py (right): https://codereview.chromium.org/2029873002/diff/60001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:129: @ndb.transactional This looks good to me. https://codereview.chromium.org/2029873002/diff/60001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:149: current_time_as_utc = pytz.utc.localize(datetime.utcnow()) Just curious about the context on making the limit to midnight PST?
https://codereview.chromium.org/2029873002/diff/40001/appengine/findit/handle... File appengine/findit/handlers/test/triage_analysis_test.py (right): https://codereview.chromium.org/2029873002/diff/40001/appengine/findit/handle... appengine/findit/handlers/test/triage_analysis_test.py:180: 'revision': '3cf9343f4602d4ec11717cb6ff56a793c1d5f84b', nit: for consistency with the rest of the changes just use the short revision hashes, we generally don't need such long ones for unit tests https://codereview.chromium.org/2029873002/diff/40001/appengine/findit/handle... appengine/findit/handlers/test/triage_analysis_test.py:289: 'step_name': 'interactive_ui_tests', nit: use general test names, i.e. 'step_name': 'step1' https://codereview.chromium.org/2029873002/diff/60001/appengine/findit/handle... File appengine/findit/handlers/test/triage_analysis_test.py (right): https://codereview.chromium.org/2029873002/diff/60001/appengine/findit/handle... appengine/findit/handlers/test/triage_analysis_test.py:247: 'revision': '0e8dc209f5e4a6140e43551de0e036324c68a383', nit: this is fine but you can get away with using shorter revisions like 'rev1' 'rev2' in general https://codereview.chromium.org/2029873002/diff/60001/appengine/findit/handle... appengine/findit/handlers/test/triage_analysis_test.py:361: def createAnalysis(self, build_number, build_start_time): nit: I would make this an internal method, i.e. _createAnalysis(...) https://codereview.chromium.org/2029873002/diff/60001/appengine/findit/handle... File appengine/findit/handlers/triage_analysis.py (right): https://codereview.chromium.org/2029873002/diff/60001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:149: current_time_as_utc = pytz.utc.localize(datetime.utcnow()) isn't current_time_as_utc just datetime.utcnow()? https://codereview.chromium.org/2029873002/diff/60001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:151: # Convert to pacific time nit: comments end with . https://codereview.chromium.org/2029873002/diff/60001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:153: pytz.timezone(MATCHING_ANALYSIS_END_BOUND_TIME_ZONE)) is there a reason we're using pst and not utc?
Also I would update the description of this change from "First Draft ..." to what it actually does, i.e. "[Findit] Checks duplicate analyses triage results across platforms" or something similar
Description was changed from ========== First draft at enabling cross-platform triage for Findit sheriffs Enables Find-it sherrifs to triage similar build analyses by triaging one build analysis. BUG=614478 ========== to ========== Enable cross-platform triage of duplicate build analyses for Findit sheriffs, by triggering a single build analysis. BUG=614478 ==========
https://codereview.chromium.org/2029873002/diff/60001/appengine/findit/handle... File appengine/findit/handlers/triage_analysis.py (right): https://codereview.chromium.org/2029873002/diff/60001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:149: current_time_as_utc = pytz.utc.localize(datetime.utcnow()) On 2016/06/24 01:07:23, stgao wrote: > Just curious about the context on making the limit to midnight PST? This is to copy what we're doing right now when we triage manually. https://codereview.chromium.org/2029873002/diff/60001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:153: pytz.timezone(MATCHING_ANALYSIS_END_BOUND_TIME_ZONE)) On 2016/06/24 01:30:06, lijeffrey wrote: > is there a reason we're using pst and not utc? This is to copy what we're doing right now when we triage manually.
Hello all, I made a few small changes, and hopefully this is good to go! Josiah https://codereview.chromium.org/2029873002/diff/40001/appengine/findit/handle... File appengine/findit/handlers/test/triage_analysis_test.py (right): https://codereview.chromium.org/2029873002/diff/40001/appengine/findit/handle... appengine/findit/handlers/test/triage_analysis_test.py:180: 'revision': '3cf9343f4602d4ec11717cb6ff56a793c1d5f84b', On 2016/06/24 01:30:06, lijeffrey wrote: > nit: for consistency with the rest of the changes just use the short revision > hashes, we generally don't need such long ones for unit tests Done. https://codereview.chromium.org/2029873002/diff/40001/appengine/findit/handle... appengine/findit/handlers/test/triage_analysis_test.py:289: 'step_name': 'interactive_ui_tests', On 2016/06/24 01:30:06, lijeffrey wrote: > nit: use general test names, i.e. 'step_name': 'step1' Done. https://codereview.chromium.org/2029873002/diff/60001/appengine/findit/handle... File appengine/findit/handlers/test/triage_analysis_test.py (right): https://codereview.chromium.org/2029873002/diff/60001/appengine/findit/handle... appengine/findit/handlers/test/triage_analysis_test.py:247: 'revision': '0e8dc209f5e4a6140e43551de0e036324c68a383', On 2016/06/24 01:30:06, lijeffrey wrote: > nit: this is fine but you can get away with using shorter revisions like 'rev1' > 'rev2' in general Ah ok. I changed them. Thanks! https://codereview.chromium.org/2029873002/diff/60001/appengine/findit/handle... appengine/findit/handlers/test/triage_analysis_test.py:361: def createAnalysis(self, build_number, build_start_time): On 2016/06/24 01:30:06, lijeffrey wrote: > nit: I would make this an internal method, i.e. _createAnalysis(...) Done. https://codereview.chromium.org/2029873002/diff/60001/appengine/findit/handle... File appengine/findit/handlers/triage_analysis.py (right): https://codereview.chromium.org/2029873002/diff/60001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:149: current_time_as_utc = pytz.utc.localize(datetime.utcnow()) On 2016/06/24 01:30:06, lijeffrey wrote: > isn't current_time_as_utc just datetime.utcnow()? No, because datetime.utcnow() doesn't contain timezone information: | >>> dt = datetime.utcnow() | >>> dt | datetime.datetime(2016, 6, 24, 16, 16, 35, 416300) | >>> pytz.utc.localize(dt) | datetime.datetime(2016, 6, 24, 16, 16, 35, 416300, tzinfo=<UTC>) https://codereview.chromium.org/2029873002/diff/60001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:151: # Convert to pacific time On 2016/06/24 01:30:06, lijeffrey wrote: > nit: comments end with . Done.
Hello all, I made a few small changes, and hopefully this is good to go! Josiah
lgtm
The CQ bit was checked by josiahk@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from chanli@chromium.org, stgao@chromium.org Link to the patchset: https://codereview.chromium.org/2029873002/#ps80001 (title: "Naming, periods, and whitespace.")
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 ========== Enable cross-platform triage of duplicate build analyses for Findit sheriffs, by triggering a single build analysis. BUG=614478 ========== to ========== Enable cross-platform triage of duplicate build analyses for Findit sheriffs, by triggering a single build analysis. BUG=614478 Committed: https://chromium.googlesource.com/infra/infra/+/6815a5aa75d80c51d8d35c2ab8936... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/infra/infra/+/6815a5aa75d80c51d8d35c2ab8936...
Message was sent while issue was closed.
Description was changed from ========== Enable cross-platform triage of duplicate build analyses for Findit sheriffs, by triggering a single build analysis. BUG=614478 Committed: https://chromium.googlesource.com/infra/infra/+/6815a5aa75d80c51d8d35c2ab8936... ========== to ========== Enable cross-platform triage of duplicate build analyses for Findit sheriffs, by triggering a single build analysis. BUG=614478 Committed: https://chromium.googlesource.com/infra/infra/+/6815a5aa75d80c51d8d35c2ab8936... ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
