Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2086113004:
[Findit] Show build analysis references in UI for Findit Cross-platform auto-triage (Closed)
DescriptionShow build analysis references in UI for Findit Cross-platform auto-triage
BUG=615127
Committed: https://chromium.googlesource.com/infra/infra/+/238f402c828dcb86c32ee097bb88b3777a8f332c
Patch Set 1 #
Total comments: 14
Patch Set 2 : Merged with ("rebased" on) Issue 2029873002's code. Other changes. #
Total comments: 21
Patch Set 3 : Test changes, one minute added, style changes #Patch Set 4 : Merged with build-matching #
Total comments: 6
Patch Set 5 : Moved duplicator reference reset code, put duplicate info behind debug boolean, fixed js style #
Total comments: 4
Patch Set 6 : Changed duplicate docsting and comment wording #Patch Set 7 : Changed two days ago to three days ago for tests. #Patch Set 8 : Rebased on tip of tree #
Depends on Patchset: Messages
Total messages: 29 (7 generated)
Description was changed from ========== Show build analysis references in UI for FindIt Cross-platform auto-triage BUG= ========== to ========== Show build analysis references in UI for FindIt Cross-platform auto-triage BUG= ==========
josiahk@google.com changed reviewers: + chanli@chromium.org, lijeffrey@chromium.org, stgao@chromium.org
Hello Chan, Jeff, and Shuotao, Here is my second CL. Now the code can tell the user which analysis was responsible for the triage result status of the current analysis if it's a duplicate. The code can also tell the user how many duplicate analyses they just automatically triaged after a manual triage.
Description was changed from ========== Show build analysis references in UI for FindIt Cross-platform auto-triage BUG= ========== to ========== Show build analysis references in UI for FindIt Cross-platform auto-triage BUG=615127 ==========
https://codereview.chromium.org/2086113004/diff/1/appengine/findit/common/bas... File appengine/findit/common/base_handler.py (left): https://codereview.chromium.org/2086113004/diff/1/appengine/findit/common/bas... appengine/findit/common/base_handler.py:26: This two lines should not be deleted. https://codereview.chromium.org/2086113004/diff/1/appengine/findit/handlers/t... File appengine/findit/handlers/triage_analysis.py (right): https://codereview.chromium.org/2086113004/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:98: correct: True if the history record should indicate a correct judgement, Did you rebase your patch? I remembered you have changed this to 'is_correct'? https://codereview.chromium.org/2086113004/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:100: user_name: The user_name of the person to include in the triage record. Add description of 'duplicate' here https://codereview.chromium.org/2086113004/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:144: analysis.triage_reference_analysis_build_number = None These lines are not needed. https://codereview.chromium.org/2086113004/diff/1/appengine/findit/model/wf_a... File appengine/findit/model/wf_analysis.py (right): https://codereview.chromium.org/2086113004/diff/1/appengine/findit/model/wf_a... appengine/findit/model/wf_analysis.py:99: return False Nit: you can also use: return True if self.result_status in (result_status.FOUND_CORRECT_DUPLICATE,result_status.FOUND_INCORRECT_DUPLICATE) else False https://codereview.chromium.org/2086113004/diff/1/appengine/findit/model/wf_a... appengine/findit/model/wf_analysis.py:174: # triage_reference_analysis_key = ndb.StringProperty(indexed=False) If you don't need this anymore, please remove it.
Hello! I have merged my UI issue's code on my first issue's code. I also addressed some comments, and made some other changes. If you could take a look, I'd appreciate it! Thanks! Josiah https://codereview.chromium.org/2086113004/diff/1/appengine/findit/common/bas... File appengine/findit/common/base_handler.py (left): https://codereview.chromium.org/2086113004/diff/1/appengine/findit/common/bas... appengine/findit/common/base_handler.py:26: On 2016/06/24 18:24:20, chanli wrote: > This two lines should not be deleted. Done. https://codereview.chromium.org/2086113004/diff/1/appengine/findit/handlers/t... File appengine/findit/handlers/triage_analysis.py (right): https://codereview.chromium.org/2086113004/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/triage_analysis.py:100: user_name: The user_name of the person to include in the triage record. On 2016/06/24 18:24:20, chanli wrote: > Add description of 'duplicate' here Done. https://codereview.chromium.org/2086113004/diff/1/appengine/findit/model/wf_a... File appengine/findit/model/wf_analysis.py (right): https://codereview.chromium.org/2086113004/diff/1/appengine/findit/model/wf_a... appengine/findit/model/wf_analysis.py:99: return False On 2016/06/24 18:24:20, chanli wrote: > Nit: you can also use: > > return True if self.result_status in > (result_status.FOUND_CORRECT_DUPLICATE,result_status.FOUND_INCORRECT_DUPLICATE) > else False Done. https://codereview.chromium.org/2086113004/diff/1/appengine/findit/model/wf_a... appengine/findit/model/wf_analysis.py:174: # triage_reference_analysis_key = ndb.StringProperty(indexed=False) On 2016/06/24 18:24:20, chanli wrote: > If you don't need this anymore, please remove it. Done.
Hello! I have merged my UI issue's code on my first issue's code. I also addressed some comments, and made some other changes. If you could take a look, I'd appreciate it! Thanks! Josiah
https://codereview.chromium.org/2086113004/diff/1/appengine/findit/common/bas... File appengine/findit/common/base_handler.py (left): https://codereview.chromium.org/2086113004/diff/1/appengine/findit/common/bas... appengine/findit/common/base_handler.py:26: I think gpylin will complain if there aren't 2 lines but I could be wrong (and it in fact complained there were 2 lines here), be sure to run gpylint <path to your .py> before submitting a patch for review https://codereview.chromium.org/2086113004/diff/1/appengine/findit/common/bas... appengine/findit/common/base_handler.py:29: It also doesn't seem like there are any functional changes to this module so you may want to revert this file before submitting https://codereview.chromium.org/2086113004/diff/1/appengine/findit/handlers/t... File appengine/findit/handlers/test/triage_analysis_test.py (right): https://codereview.chromium.org/2086113004/diff/1/appengine/findit/handlers/t... appengine/findit/handlers/test/triage_analysis_test.py:140: {'success': True, 'num_duplicate_analyses': 0}, nit: I would put this second field on a separate line i.e. { 'success': True, 'num_duplicate_analyses': 0 } https://codereview.chromium.org/2086113004/diff/20001/appengine/findit/handle... File appengine/findit/handlers/test/triage_analysis_test.py (right): https://codereview.chromium.org/2086113004/diff/20001/appengine/findit/handle... appengine/findit/handlers/test/triage_analysis_test.py:8: from testing_utils import testing I think you can leave this one where it was even though gpylint complains about it, i think the order we want is general (i.e. datetime), then these third party ones(not 100% sure), then findit-specific. They should otherwise be alphabetized within their groups https://codereview.chromium.org/2086113004/diff/20001/appengine/findit/handle... appengine/findit/handlers/test/triage_analysis_test.py:138: {'success': True, 'num_duplicate_analyses': 0}, nit: put each field on a new line, same with the other dicts i.e. { 'success': True, 'num_duplicate_analyses': 0, ... } If it's a dict with just 1 entry then all inline is ok too. https://codereview.chromium.org/2086113004/diff/20001/appengine/findit/handle... appengine/findit/handlers/test/triage_analysis_test.py:152: def testDuplicatePropertyWorksWhenNotDuplicate(self): nit: you can name this testDuplicatePropertyNoDuplicates(self): ... no need for the word "works" in the test since that's what a test does (makes sure it works!) same with the other test https://codereview.chromium.org/2086113004/diff/20001/appengine/findit/handle... appengine/findit/handlers/test/triage_analysis_test.py:483: self._createAnalysis(313, original_time) why not make the second analysis a slightly later time, since that's the more likely scenario? https://codereview.chromium.org/2086113004/diff/20001/appengine/findit/handle... File appengine/findit/handlers/triage_analysis.py (right): https://codereview.chromium.org/2086113004/diff/20001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:51: nit: delete this empty line https://codereview.chromium.org/2086113004/diff/20001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:146: analysis.triage_reference_analysis_master_name = None so what are these for, and why are we setting them to None here? https://codereview.chromium.org/2086113004/diff/20001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:211: return len(matching_analyses) It looks like you're returning the number of matching analyses that were found. I would rename your method to something like _GetTriageDuplicateResultsCount or something to make it more clear that this method returns a count and not just performs actions https://codereview.chromium.org/2086113004/diff/20001/appengine/findit/model/... File appengine/findit/model/wf_analysis.py (right): https://codereview.chromium.org/2086113004/diff/20001/appengine/findit/model/... appengine/findit/model/wf_analysis.py:87: def is_duplicate(self): nit: I would name this just duplicate but this is fine too, if no one else has objections you can leave it as is
Since this change has UI updates you can deploy a test version and share a link for us to see the new UI you can deploy by following instructions under util_scripts/run.sh --help, I believe the option you want is deploy-test-prod. That will deploy a test version (against live data however so be careful) of Findit with whatever you have in your local checkout
https://codereview.chromium.org/2086113004/diff/20001/appengine/findit/handle... File appengine/findit/handlers/triage_analysis.py (right): https://codereview.chromium.org/2086113004/diff/20001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:146: analysis.triage_reference_analysis_master_name = None On 2016/06/27 20:38:22, lijeffrey wrote: > so what are these for, and why are we setting them to None here? +1
https://codereview.chromium.org/2086113004/diff/20001/appengine/findit/handle... File appengine/findit/handlers/build_failure.py (right): https://codereview.chromium.org/2086113004/diff/20001/appengine/findit/handle... appengine/findit/handlers/build_failure.py:278: users.is_current_user_admin() or self.request.get('debug') == '1') nit: could fit in one line
Here are my latest changes for the UI task! * Substantial stuff: - Added/removed tests relating to WfAnalysis.is_duplicate. Now 100% code coverage. - Added one minute to two Triage Analysis tests. * Nit stuff: - Style changes. - Import order changes. - Naming changes. If you'd like to try out the last changelists' code live, you can go here (thanks Chan!): https://chanli-dot-findit-for-me.appspot.com/ Thanks! Josiah https://codereview.chromium.org/2086113004/diff/1/appengine/findit/common/bas... File appengine/findit/common/base_handler.py (left): https://codereview.chromium.org/2086113004/diff/1/appengine/findit/common/bas... appengine/findit/common/base_handler.py:26: On 2016/06/27 20:38:22, lijeffrey wrote: > I think gpylin will complain if there aren't 2 lines but I could be wrong (and > it in fact complained there were 2 lines here), be sure to run gpylint <path to > your .py> before submitting a patch for review Fixed. Thanks! https://codereview.chromium.org/2086113004/diff/20001/appengine/findit/handle... File appengine/findit/handlers/test/triage_analysis_test.py (right): https://codereview.chromium.org/2086113004/diff/20001/appengine/findit/handle... appengine/findit/handlers/test/triage_analysis_test.py:8: from testing_utils import testing On 2016/06/27 20:38:22, lijeffrey wrote: > I think you can leave this one where it was even though gpylint complains about > it, i think the order we want is general (i.e. datetime), then these third party > ones(not 100% sure), then findit-specific. They should otherwise be alphabetized > within their groups Kept per conversation. https://codereview.chromium.org/2086113004/diff/20001/appengine/findit/handle... appengine/findit/handlers/test/triage_analysis_test.py:138: {'success': True, 'num_duplicate_analyses': 0}, On 2016/06/27 20:38:22, lijeffrey wrote: > nit: put each field on a new line, same with the other dicts > > i.e. > > { > 'success': True, > 'num_duplicate_analyses': 0, > ... > } > > If it's a dict with just 1 entry then all inline is ok too. Done! Thanks! https://codereview.chromium.org/2086113004/diff/20001/appengine/findit/handle... appengine/findit/handlers/test/triage_analysis_test.py:152: def testDuplicatePropertyWorksWhenNotDuplicate(self): On 2016/06/27 20:38:22, lijeffrey wrote: > nit: you can name this > > testDuplicatePropertyNoDuplicates(self): > ... > > no need for the word "works" in the test since that's what a test does (makes > sure it works!) > > same with the other test Thanks! https://codereview.chromium.org/2086113004/diff/20001/appengine/findit/handle... appengine/findit/handlers/test/triage_analysis_test.py:483: self._createAnalysis(313, original_time) On 2016/06/27 20:38:22, lijeffrey wrote: > why not make the second analysis a slightly later time, since that's the more > likely scenario? Done! https://codereview.chromium.org/2086113004/diff/20001/appengine/findit/handle... File appengine/findit/handlers/triage_analysis.py (right): https://codereview.chromium.org/2086113004/diff/20001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:51: On 2016/06/27 20:38:22, lijeffrey wrote: > nit: delete this empty line Done. https://codereview.chromium.org/2086113004/diff/20001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:146: analysis.triage_reference_analysis_master_name = None On 2016/06/27 22:27:56, chanli wrote: > On 2016/06/27 20:38:22, lijeffrey wrote: > > so what are these for, and why are we setting them to None here? > > +1 When another "first-cause" build analysis is triaged, and this build analysis is marked as a duplicate from that other "first-cause" build analysis, these are the variables that hold the reference back to that "first-cause" build analysis. It's possible that someone could then manually re-triage this build analysis, in which case this build analysis is no longer a duplicate, and we want to erase the reference to the no-longer-relevant "first-cause" build_analysis, which is what these lines are doing. Maybe it would be a good idea for me to move these lines into _AppendTriageHistoryRecord(), because that is where other properties of the analysis get updated. https://codereview.chromium.org/2086113004/diff/20001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:211: return len(matching_analyses) On 2016/06/27 20:38:22, lijeffrey wrote: > It looks like you're returning the number of matching analyses that were found. > I would rename your method to something like _GetTriageDuplicateResultsCount or > something to make it more clear that this method returns a count and not just > performs actions Renamed to _TriageAndCountDuplicateResults()
Also put [Findit] as the first thing in your change description, making it easier to see which CLs are findit related and which are general infra as they all go into the same repo https://codereview.chromium.org/2086113004/diff/20001/appengine/findit/handle... File appengine/findit/handlers/triage_analysis.py (right): https://codereview.chromium.org/2086113004/diff/20001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:146: analysis.triage_reference_analysis_master_name = None On 2016/06/28 22:59:23, josiahk wrote: > On 2016/06/27 22:27:56, chanli wrote: > > On 2016/06/27 20:38:22, lijeffrey wrote: > > > so what are these for, and why are we setting them to None here? > > > > +1 > > When another "first-cause" build analysis is triaged, and this build analysis is > marked as a duplicate from that other "first-cause" build analysis, these are > the variables that hold the reference back to that "first-cause" build analysis. > > It's possible that someone could then manually re-triage this build analysis, in > which case this build analysis is no longer a duplicate, and we want to erase > the reference to the no-longer-relevant "first-cause" build_analysis, which is > what these lines are doing. > > Maybe it would be a good idea for me to move these lines into > _AppendTriageHistoryRecord(), because that is where other properties of the > analysis get updated. you don't need to initialize them to None, if they're not explicitly set they should default to None so these lines are unnecessary unless you specifically want to overwrite any previously set values
Description was changed from ========== Show build analysis references in UI for FindIt Cross-platform auto-triage BUG=615127 ========== to ========== Show build analysis references in UI for Findit Cross-platform auto-triage BUG=615127 ==========
I did a git pull, which merged my change with more recent code from master/build-matching. https://codereview.chromium.org/2086113004/diff/20001/appengine/findit/handle... File appengine/findit/handlers/triage_analysis.py (right): https://codereview.chromium.org/2086113004/diff/20001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:146: analysis.triage_reference_analysis_master_name = None On 2016/06/29 00:48:23, lijeffrey wrote: > On 2016/06/28 22:59:23, josiahk wrote: > > On 2016/06/27 22:27:56, chanli wrote: > > > On 2016/06/27 20:38:22, lijeffrey wrote: > > > > so what are these for, and why are we setting them to None here? > > > > > > +1 > > > > When another "first-cause" build analysis is triaged, and this build analysis > is > > marked as a duplicate from that other "first-cause" build analysis, these are > > the variables that hold the reference back to that "first-cause" build > analysis. > > > > It's possible that someone could then manually re-triage this build analysis, > in > > which case this build analysis is no longer a duplicate, and we want to erase > > the reference to the no-longer-relevant "first-cause" build_analysis, which is > > what these lines are doing. > > > > Maybe it would be a good idea for me to move these lines into > > _AppendTriageHistoryRecord(), because that is where other properties of the > > analysis get updated. > > you don't need to initialize them to None, if they're not explicitly set they > should default to None so these lines are unnecessary unless you specifically > want to overwrite any previously set values Yes, I want to potentially overwrite previously set values. There is a possibility they might be set to something other than None (see lines 202-207). Thanks!
https://codereview.chromium.org/2086113004/diff/60001/appengine/findit/templa... File appengine/findit/templates/build_failure.html (right): https://codereview.chromium.org/2086113004/diff/60001/appengine/findit/templa... appengine/findit/templates/build_failure.html:79: findit.triageReferenceAnalysisUrl = '/waterfall/build-failure?url=https://build.chromium.org/p/{{triage_reference_analysis_master_name}}/builders/{{triage_reference_analysis_builder_name}}/builds/{{triage_reference_analysis_build_number}}' Per coding style, JS statement should always end with a ";". Same for other lines of new code in this CL. https://codereview.chromium.org/2086113004/diff/60001/appengine/findit/templa... appengine/findit/templates/build_failure.html:101: $('#duplicates_marked').html("Number of matching build analyses that were also triaged: " + data['num_duplicate_analyses']) code style: no double quote ["] in JS code. Same for new JS code below. Note: in html code below, we do you double quote instead of single quote for attribute values. https://codereview.chromium.org/2086113004/diff/60001/appengine/findit/templa... appengine/findit/templates/build_failure.html:444: if (findit.analysisIsDuplicate) { To double check: do we want to show this to everyone or just to Findit admins?
https://codereview.chromium.org/2086113004/diff/20001/appengine/findit/handle... File appengine/findit/handlers/triage_analysis.py (right): https://codereview.chromium.org/2086113004/diff/20001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:146: analysis.triage_reference_analysis_master_name = None On 2016/06/29 22:40:00, josiahk wrote: > On 2016/06/29 00:48:23, lijeffrey wrote: > > On 2016/06/28 22:59:23, josiahk wrote: > > > On 2016/06/27 22:27:56, chanli wrote: > > > > On 2016/06/27 20:38:22, lijeffrey wrote: > > > > > so what are these for, and why are we setting them to None here? > > > > > > > > +1 > > > > > > When another "first-cause" build analysis is triaged, and this build > analysis > > is > > > marked as a duplicate from that other "first-cause" build analysis, these > are > > > the variables that hold the reference back to that "first-cause" build > > analysis. > > > > > > It's possible that someone could then manually re-triage this build > analysis, > > in > > > which case this build analysis is no longer a duplicate, and we want to > erase > > > the reference to the no-longer-relevant "first-cause" build_analysis, which > is > > > what these lines are doing. > > > > > > Maybe it would be a good idea for me to move these lines into > > > _AppendTriageHistoryRecord(), because that is where other properties of the > > > analysis get updated. > > > > you don't need to initialize them to None, if they're not explicitly set they > > should default to None so these lines are unnecessary unless you specifically > > want to overwrite any previously set values > > Yes, I want to potentially overwrite previously set values. There is a > possibility they might be set to something other than None (see lines 202-207). > Thanks! It appears with what you have triage duplicate results which will do the writing is always called after this function. If that's indeed the case then setting these to None here would just be overwritten anyway too, making them unnecessary
Hello all, I made some changes including moving duplicator reference reset code, putting duplicate info behind a debug boolean, and fixing js style. Hopefully everything is good to go! Thank you! Josiah https://codereview.chromium.org/2086113004/diff/20001/appengine/findit/handle... File appengine/findit/handlers/triage_analysis.py (right): https://codereview.chromium.org/2086113004/diff/20001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:146: analysis.triage_reference_analysis_master_name = None On 2016/06/30 01:09:46, lijeffrey wrote: > On 2016/06/29 22:40:00, josiahk wrote: > > On 2016/06/29 00:48:23, lijeffrey wrote: > > > On 2016/06/28 22:59:23, josiahk wrote: > > > > On 2016/06/27 22:27:56, chanli wrote: > > > > > On 2016/06/27 20:38:22, lijeffrey wrote: > > > > > > so what are these for, and why are we setting them to None here? > > > > > > > > > > +1 > > > > > > > > When another "first-cause" build analysis is triaged, and this build > > analysis > > > is > > > > marked as a duplicate from that other "first-cause" build analysis, these > > are > > > > the variables that hold the reference back to that "first-cause" build > > > analysis. > > > > > > > > It's possible that someone could then manually re-triage this build > > analysis, > > > in > > > > which case this build analysis is no longer a duplicate, and we want to > > erase > > > > the reference to the no-longer-relevant "first-cause" build_analysis, > which > > is > > > > what these lines are doing. > > > > > > > > Maybe it would be a good idea for me to move these lines into > > > > _AppendTriageHistoryRecord(), because that is where other properties of > the > > > > analysis get updated. > > > > > > you don't need to initialize them to None, if they're not explicitly set > they > > > should default to None so these lines are unnecessary unless you > specifically > > > want to overwrite any previously set values > > > > Yes, I want to potentially overwrite previously set values. There is a > > possibility they might be set to something other than None (see lines > 202-207). > > Thanks! > > It appears with what you have triage duplicate results which will do the writing > is always called after this function. If that's indeed the case then setting > these to None here would just be overwritten anyway too, making them unnecessary _UpdateAnalysisResultStatus() operates on a single original build analysis that the user directly triages. _TriageDuplicateResults() operates on potentially many build analyses that match that first single original build analysis. Since _TriageDuplicateResults() doesn't operate on the original build analysis, the None's don't get overwritten right away. I moved this code to inside _AppendTriageHistoryRecord(), and added a conditional and a comment; hopefully the code is more self-explanatory now! https://codereview.chromium.org/2086113004/diff/60001/appengine/findit/templa... File appengine/findit/templates/build_failure.html (right): https://codereview.chromium.org/2086113004/diff/60001/appengine/findit/templa... appengine/findit/templates/build_failure.html:79: findit.triageReferenceAnalysisUrl = '/waterfall/build-failure?url=https://build.chromium.org/p/{{triage_reference_analysis_master_name}}/builders/{{triage_reference_analysis_builder_name}}/builds/{{triage_reference_analysis_build_number}}' On 2016/06/30 01:08:16, stgao wrote: > Per coding style, JS statement should always end with a ";". > > Same for other lines of new code in this CL. Done! https://codereview.chromium.org/2086113004/diff/60001/appengine/findit/templa... appengine/findit/templates/build_failure.html:101: $('#duplicates_marked').html("Number of matching build analyses that were also triaged: " + data['num_duplicate_analyses']) On 2016/06/30 01:08:16, stgao wrote: > code style: no double quote ["] in JS code. > Same for new JS code below. > > Note: in html code below, we do you double quote instead of single quote for > attribute values. Done. https://codereview.chromium.org/2086113004/diff/60001/appengine/findit/templa... appengine/findit/templates/build_failure.html:444: if (findit.analysisIsDuplicate) { On 2016/06/30 01:08:16, stgao wrote: > To double check: do we want to show this to everyone or just to Findit admins? Changed to show duplicate information only if show_debug_info is true (which will be true if the user is an admin, or if the &debug=1 flag is present in the URL)
lgtm with a nit. https://codereview.chromium.org/2086113004/diff/80001/appengine/findit/handle... File appengine/findit/handlers/triage_analysis.py (right): https://codereview.chromium.org/2086113004/diff/80001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:126: # Resets the reference to the "duplicator" triage analysis. This comment is a little confusing... How about adding reasoning about why we need to reset here?
https://codereview.chromium.org/2086113004/diff/80001/appengine/findit/handle... File appengine/findit/handlers/triage_analysis.py (right): https://codereview.chromium.org/2086113004/diff/80001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:103: a duplicate, otherwise False. After the discussion, is_duplicate is not determined by history record to me. It's actually determined by whether someone triages this analysis result or it's triaged as a duplicate.
I changed "duplicate" related docstring and comment wording. Thanks! Josiah https://codereview.chromium.org/2086113004/diff/80001/appengine/findit/handle... File appengine/findit/handlers/triage_analysis.py (right): https://codereview.chromium.org/2086113004/diff/80001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:103: a duplicate, otherwise False. On 2016/06/30 23:52:25, chanli wrote: > After the discussion, is_duplicate is not determined by history record to me. > It's actually determined by whether someone triages this analysis result or it's > triaged as a duplicate. Thanks! I have changed the docstring. https://codereview.chromium.org/2086113004/diff/80001/appengine/findit/handle... appengine/findit/handlers/triage_analysis.py:126: # Resets the reference to the "duplicator" triage analysis. On 2016/06/30 23:46:37, chanli wrote: > This comment is a little confusing... > > How about adding reasoning about why we need to reset here? Done.
On 2016/07/01 21:45:27, josiahk wrote: > I changed "duplicate" related docstring and comment wording. > > Thanks! > Josiah > > https://codereview.chromium.org/2086113004/diff/80001/appengine/findit/handle... > File appengine/findit/handlers/triage_analysis.py (right): > > https://codereview.chromium.org/2086113004/diff/80001/appengine/findit/handle... > appengine/findit/handlers/triage_analysis.py:103: a duplicate, otherwise False. > On 2016/06/30 23:52:25, chanli wrote: > > After the discussion, is_duplicate is not determined by history record to me. > > It's actually determined by whether someone triages this analysis result or > it's > > triaged as a duplicate. > Thanks! I have changed the docstring. > > https://codereview.chromium.org/2086113004/diff/80001/appengine/findit/handle... > appengine/findit/handlers/triage_analysis.py:126: # Resets the reference to the > "duplicator" triage analysis. > On 2016/06/30 23:46:37, chanli wrote: > > This comment is a little confusing... > > > > How about adding reasoning about why we need to reset here? > > Done. lgtm. please rebase and before commit.
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/2086113004/#ps140001 (title: "Rebased on tip of tree")
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 ========== Show build analysis references in UI for Findit Cross-platform auto-triage BUG=615127 ========== to ========== Show build analysis references in UI for Findit Cross-platform auto-triage BUG=615127 Committed: https://chromium.googlesource.com/infra/infra/+/238f402c828dcb86c32ee097bb88b... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/infra/infra/+/238f402c828dcb86c32ee097bb88b...
Message was sent while issue was closed.
CQ bit was unchecked. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
