Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1566013002:
Add support for bisect bots to post results to dashboard. (Closed)
DescriptionAdd support for bisect bots to post results to dashboard.
High level changes:
update_bug_with_results.py
- Removed the need to query rietveld, buildbucket, and buildbots. This will make it simpler. We want to get all the necessary results in one place which is from the bots.
- Removed tracking infra failure. Current method is not very reliable.
- TryJobs now do not get removed.
post_bisect_results.py
- To make it simple, we verify the data, and save it directly to a datastore JSONProperty.
bisect_report.py
- This is where we create report base on TryJob states, whether they failed, staled, or completed.
try_job.py
- Added 'results_data' which is data directly from bisect bots.
- Added a 'staled' state.
BUG=catapult:#1869
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/4e0a366a9842da32ef20e8af466e5d0d7519d5e1
Patch Set 1 #
Total comments: 29
Patch Set 2 : addressed comments #
Total comments: 34
Patch Set 3 : address comments #
Total comments: 8
Patch Set 4 : rebase #
Total comments: 5
Patch Set 5 : address comments #Patch Set 6 : check confidence to cc author #
Total comments: 1
Patch Set 7 : rebase #Patch Set 8 : rebase #
Messages
Total messages: 22 (6 generated)
chrisphan@chromium.org changed reviewers: + prasadv@chromium.org, qyearsley@chromium.org
In the work is adding doc and tests. https://codereview.chromium.org/1566013002/diff/1/dashboard/dashboard/update_... File dashboard/dashboard/update_bug_with_results_test.py (right): https://codereview.chromium.org/1566013002/diff/1/dashboard/dashboard/update_... dashboard/dashboard/update_bug_with_results_test.py:1: # Copyright 2016 The Chromium Authors. All rights reserved. Currently be worked on: - update tests that are left here - add applicable new tests The tests that were removed are tests that are no applicable anymore. For example testGet_PerfTryJob, asserts bisect results data exist, but with posting method, data is only as good as that bots posting them.
https://codereview.chromium.org/1566013002/diff/1/dashboard/dashboard/email_t... File dashboard/dashboard/email_template.py (right): https://codereview.chromium.org/1566013002/diff/1/dashboard/dashboard/email_t... dashboard/dashboard/email_template.py:77: _PERF_TRY_EMAIL_TEXT_BODY = """ Pradsad, do you think all these can be replaced with a more generic email message and link to a bisect result page which uses bisect_report.py included here? The reason is that, this kind of duplicates bisect_report.py. Perf Try Job %(status)s. Bisect found a culprit. Please visit https://chromeperf.appspot.com/bisect_results?try_job_id=12345
Description was changed from ========== Add support for bisect bots to post results to dashboard. High level changes: update_bug_with_results.py - Removed the need to query rietveld, buildbucket, and buildbots. This will make it simpler. We want to get all the necessary results in one place which is from the bots. - Removed tracking infra failure. Current method is not very reliable. - TryJobs now do not get removed. post_bisect_results.py - To make it simple, we verify the data, and save it directly to a datastore JSONProperty. bisect_report.py - This is where we create report base on TryJob states, whether they failed, staled, or completed. try_job.py - Added 'results_data' which is data directly from bisect bots. - Added a 'staled' state. Bug: crbug.com/573308 BUG=catapult:# ========== to ========== Add support for bisect bots to post results to dashboard. High level changes: update_bug_with_results.py - Removed the need to query rietveld, buildbucket, and buildbots. This will make it simpler. We want to get all the necessary results in one place which is from the bots. - Removed tracking infra failure. Current method is not very reliable. - TryJobs now do not get removed. post_bisect_results.py - To make it simple, we verify the data, and save it directly to a datastore JSONProperty. bisect_report.py - This is where we create report base on TryJob states, whether they failed, staled, or completed. try_job.py - Added 'results_data' which is data directly from bisect bots. - Added a 'staled' state. BUG=catapult:# ==========
Description was changed from ========== Add support for bisect bots to post results to dashboard. High level changes: update_bug_with_results.py - Removed the need to query rietveld, buildbucket, and buildbots. This will make it simpler. We want to get all the necessary results in one place which is from the bots. - Removed tracking infra failure. Current method is not very reliable. - TryJobs now do not get removed. post_bisect_results.py - To make it simple, we verify the data, and save it directly to a datastore JSONProperty. bisect_report.py - This is where we create report base on TryJob states, whether they failed, staled, or completed. try_job.py - Added 'results_data' which is data directly from bisect bots. - Added a 'staled' state. BUG=catapult:# ========== to ========== Add support for bisect bots to post results to dashboard. High level changes: update_bug_with_results.py - Removed the need to query rietveld, buildbucket, and buildbots. This will make it simpler. We want to get all the necessary results in one place which is from the bots. - Removed tracking infra failure. Current method is not very reliable. - TryJobs now do not get removed. post_bisect_results.py - To make it simple, we verify the data, and save it directly to a datastore JSONProperty. bisect_report.py - This is where we create report base on TryJob states, whether they failed, staled, or completed. try_job.py - Added 'results_data' which is data directly from bisect bots. - Added a 'staled' state. BUG=catapult:#1869 ==========
https://codereview.chromium.org/1566013002/diff/1/dashboard/dashboard/bisect_... File dashboard/dashboard/bisect_report.py (right): https://codereview.chromium.org/1566013002/diff/1/dashboard/dashboard/bisect_... dashboard/dashboard/bisect_report.py:5: """URL endpoint for a cron job to update bugs after bisects.""" File docstring should be removed or updated. https://codereview.chromium.org/1566013002/diff/1/dashboard/dashboard/bisect_... File dashboard/dashboard/bisect_report_test.py (right): https://codereview.chromium.org/1566013002/diff/1/dashboard/dashboard/bisect_... dashboard/dashboard/bisect_report_test.py:159: pass Pending implementation. When you implement this, it may be a good idea to avoid big global strings, because they tend to make the tests more fragile and harder to read. Even if you use a big literal string, if possible it's good to put them in the one test method where they're used, and when possible, it's less fragile to assert that just one or two key things is contained in the string. https://codereview.chromium.org/1566013002/diff/1/dashboard/dashboard/post_bi... File dashboard/dashboard/post_bisect_results.py (right): https://codereview.chromium.org/1566013002/diff/1/dashboard/dashboard/post_bi... dashboard/dashboard/post_bisect_results.py:11: import re Some of these appear unused; you can run pylint --enable=W0611 to find unused imports. https://codereview.chromium.org/1566013002/diff/1/dashboard/dashboard/post_bi... dashboard/dashboard/post_bisect_results.py:27: 'status': ['completed', 'failed', 'pending', 'aborted'], Nit: 4 space indent https://codereview.chromium.org/1566013002/diff/1/dashboard/dashboard/post_bi... dashboard/dashboard/post_bisect_results.py:37: """URL endpoint to post data to the dashboard.""" This docstring can be removed. https://codereview.chromium.org/1566013002/diff/1/dashboard/dashboard/post_bi... dashboard/dashboard/post_bisect_results.py:40: """Validates data parameter and save to TryJob entity. s/save/saves/ Is there something that implicitly happens when _UpdateTryJob is called? When is a post made to crbug? https://codereview.chromium.org/1566013002/diff/1/dashboard/dashboard/post_bi... dashboard/dashboard/post_bisect_results.py:42: Bisect results come from a "data" parameter, which is a JSON encoding of a I advocate changing the name from "data" to "bisect_results". https://codereview.chromium.org/1566013002/diff/1/dashboard/dashboard/post_bi... dashboard/dashboard/post_bisect_results.py:95: """ I wonder whether this can be put somewhere else and be reused for other places where we're looking at received data or requests. Anyway for this CL it can be kept here, but it might be nice to re-use it. https://codereview.chromium.org/1566013002/diff/1/dashboard/dashboard/post_bi... dashboard/dashboard/post_bisect_results.py:99: act_type = type(actual) I think it's a little more readable (in general) if you avoid abbreviations, and use names like expected_type and actual_type instead -- "exp" and "act" in different contexts can mean different things (expiry, experimental, expansion; active, action, actor...) https://codereview.chromium.org/1566013002/diff/1/dashboard/dashboard/update_... File dashboard/dashboard/update_bug_with_results.py (right): https://codereview.chromium.org/1566013002/diff/1/dashboard/dashboard/update_... dashboard/dashboard/update_bug_with_results.py:97: # Do we want to send a FYI Bisect email here? Currently we're sending it below in _CheckFYIBisectJob. Seems like this if clause is just for the case where we want to stop retrying a bisect job (and possibly tell people that the bisect job repeatedly failed). Side note: If you think it improves readability, you could add a little function: def _IsStale(job): if not job.last_ran_timestamp: return False time_since_last_ran = datetime.datetime.now() - job.last_ran_timestamp return time_since_last_ran > _STALE_TRYJOB_DELTA https://codereview.chromium.org/1566013002/diff/1/dashboard/dashboard/update_... dashboard/dashboard/update_bug_with_results.py:309: # TODO(chrisphan): Use the new quick_logger. What's the new quick_logger? A new log name? This comment can be made more specific. Also, the input of this function is try_job entity, right? Optionally you can add a docstring here.
Updated tests for: bisect_report_test.py update_bug_with_results_test.py https://chromiumcodereview-hr.appspot.com/1566013002/diff/1/dashboard/dashboa... File dashboard/dashboard/bisect_report.py (right): https://chromiumcodereview-hr.appspot.com/1566013002/diff/1/dashboard/dashboa... dashboard/dashboard/bisect_report.py:5: """URL endpoint for a cron job to update bugs after bisects.""" On 2016/01/11 22:16:44, qyearsley wrote: > File docstring should be removed or updated. Done. https://chromiumcodereview-hr.appspot.com/1566013002/diff/1/dashboard/dashboa... File dashboard/dashboard/bisect_report_test.py (right): https://chromiumcodereview-hr.appspot.com/1566013002/diff/1/dashboard/dashboa... dashboard/dashboard/bisect_report_test.py:159: pass On 2016/01/11 22:16:44, qyearsley wrote: > Pending implementation. > > When you implement this, it may be a good idea to avoid big global strings, > because they tend to make the tests more fragile and harder to read. > > Even if you use a big literal string, if possible it's good to put them in the > one test method where they're used, and when possible, it's less fragile to > assert that just one or two key things is contained in the string. Agree, in this case, I think it's useful to see the expected layout in a bug message. LMK. https://chromiumcodereview-hr.appspot.com/1566013002/diff/1/dashboard/dashboa... File dashboard/dashboard/post_bisect_results.py (right): https://chromiumcodereview-hr.appspot.com/1566013002/diff/1/dashboard/dashboa... dashboard/dashboard/post_bisect_results.py:11: import re On 2016/01/11 22:16:45, qyearsley wrote: > Some of these appear unused; you can run pylint --enable=W0611 to find unused > imports. Done. https://chromiumcodereview-hr.appspot.com/1566013002/diff/1/dashboard/dashboa... dashboard/dashboard/post_bisect_results.py:27: 'status': ['completed', 'failed', 'pending', 'aborted'], On 2016/01/11 22:16:45, qyearsley wrote: > Nit: 4 space indent Done. https://chromiumcodereview-hr.appspot.com/1566013002/diff/1/dashboard/dashboa... dashboard/dashboard/post_bisect_results.py:37: """URL endpoint to post data to the dashboard.""" On 2016/01/11 22:16:44, qyearsley wrote: > This docstring can be removed. Done. https://chromiumcodereview-hr.appspot.com/1566013002/diff/1/dashboard/dashboa... dashboard/dashboard/post_bisect_results.py:40: """Validates data parameter and save to TryJob entity. On 2016/01/11 22:16:45, qyearsley wrote: > s/save/saves/ > > Is there something that implicitly happens when _UpdateTryJob is called? When is > a post made to crbug? This handler will serve to bots updating partial and full results. So we'll simply validate and store the results. Posting to crbug will happen in the same place update_bug_with_results.py. https://chromiumcodereview-hr.appspot.com/1566013002/diff/1/dashboard/dashboa... dashboard/dashboard/post_bisect_results.py:42: Bisect results come from a "data" parameter, which is a JSON encoding of a On 2016/01/11 22:16:45, qyearsley wrote: > I advocate changing the name from "data" to "bisect_results". This comes from a generic post_json.py which is shared with posting data to /add_point. https://chromiumcodereview-hr.appspot.com/1566013002/diff/1/dashboard/dashboa... dashboard/dashboard/post_bisect_results.py:95: """ On 2016/01/11 22:16:44, qyearsley wrote: > I wonder whether this can be put somewhere else and be reused for other places > where we're looking at received data or requests. > > Anyway for this CL it can be kept here, but it might be nice to re-use it. Acknowledged. https://chromiumcodereview-hr.appspot.com/1566013002/diff/1/dashboard/dashboa... dashboard/dashboard/post_bisect_results.py:99: act_type = type(actual) On 2016/01/11 22:16:45, qyearsley wrote: > I think it's a little more readable (in general) if you avoid abbreviations, and > use names like expected_type and actual_type instead -- "exp" and "act" in > different contexts can mean different things (expiry, experimental, expansion; > active, action, actor...) Done. https://chromiumcodereview-hr.appspot.com/1566013002/diff/1/dashboard/dashboa... File dashboard/dashboard/update_bug_with_results.py (right): https://chromiumcodereview-hr.appspot.com/1566013002/diff/1/dashboard/dashboa... dashboard/dashboard/update_bug_with_results.py:97: # Do we want to send a FYI Bisect email here? On 2016/01/11 22:16:45, qyearsley wrote: > Currently we're sending it below in _CheckFYIBisectJob. Seems like this if > clause is just for the case where we want to stop retrying a bisect job (and > possibly tell people that the bisect job repeatedly failed). > > Side note: If you think it improves readability, you could add a little > function: > > def _IsStale(job): > if not job.last_ran_timestamp: > return False > time_since_last_ran = datetime.datetime.now() - job.last_ran_timestamp > return time_since_last_ran > _STALE_TRYJOB_DELTA Added a to do for sending FYI on staled bisect. https://chromiumcodereview-hr.appspot.com/1566013002/diff/1/dashboard/dashboa... dashboard/dashboard/update_bug_with_results.py:309: # TODO(chrisphan): Use the new quick_logger. On 2016/01/11 22:16:45, qyearsley wrote: > What's the new quick_logger? A new log name? This comment can be made more > specific. > > Also, the input of this function is try_job entity, right? Optionally you can > add a docstring here. Removing this for now. I have a quicker-logger update for displaying clearer bisect results. Will add it later.
https://chromiumcodereview-hr.appspot.com/1566013002/diff/1/dashboard/dashboa... File dashboard/dashboard/post_bisect_results.py (right): https://chromiumcodereview-hr.appspot.com/1566013002/diff/1/dashboard/dashboa... dashboard/dashboard/post_bisect_results.py:40: """Validates data parameter and save to TryJob entity. On 2016/01/13 00:32:54, chrisphan wrote: > On 2016/01/11 22:16:45, qyearsley wrote: > > s/save/saves/ > > > > Is there something that implicitly happens when _UpdateTryJob is called? When > is > > a post made to crbug? > > This handler will serve to bots updating partial and full results. So we'll > simply validate and store the results. Posting to crbug will happen in the same > place update_bug_with_results.py. Also, we are updating quick-log. So at all time, quick-log will have the latest bisect results.
Really happy with this change, especially because it simplifies update_bug_with_results.py. Prasad should review email_template.py and bisect_fyi.py. https://codereview.chromium.org/1566013002/diff/20001/dashboard/dashboard/bis... File dashboard/dashboard/bisect_report.py (right): https://codereview.chromium.org/1566013002/diff/20001/dashboard/dashboard/bis... dashboard/dashboard/bisect_report.py:9: _CONFIDENCE_THRESHOLD = 99.5 This appears to be unused here now -- this "confidence threshold" is used to determine whether the bisect results "status" should be "Positive or "Negative". In the current draft of the auto-bisect side changes (https://codereview.chromium.org/1573293002), this is changed so that status doesn't indicate "Positive" or "Negative". I think it seems OK to put the deciding of positive/negative status on the dashboard side, but it's also OK to decide on the bisect side and send it over. Note that the "Positive"/"Negative" status was used to decide whether to update bugs or not -- bugs are only updated for positive results. I think we want to keep that behavior. https://codereview.chromium.org/1566013002/diff/20001/dashboard/dashboard/bis... File dashboard/dashboard/bisect_report_test.py (right): https://codereview.chromium.org/1566013002/diff/20001/dashboard/dashboard/bis... dashboard/dashboard/bisect_report_test.py:64: } Formatting should be adjusted here; see comments in post_bisect_results_test.py https://codereview.chromium.org/1566013002/diff/20001/dashboard/dashboard/bis... dashboard/dashboard/bisect_report_test.py:186: self.assertEqual(_LOG_FAILED_BISECT, bisect_report.GetReport(job)) I think that the literal expected string directly in the test method here is a good idea, even if it's very long and involves multi-line strings -- the reason is that this puts the example expected string in the context of where it's expected, and means that the reader doesn't need to scroll up and down. Additionally, in some cases instead of using self.assertEqual(<full expected text>, bisect_report.GetReport(job)) it might be less fragile while keeping the same coverage if we use self.assertIn(<key part>, bisect_report.GetReport(job)) https://codereview.chromium.org/1566013002/diff/20001/dashboard/dashboard/mod... File dashboard/dashboard/models/try_job.py (right): https://codereview.chromium.org/1566013002/diff/20001/dashboard/dashboard/mod... dashboard/dashboard/models/try_job.py:36: # Bisect run status (e.g., started, failed). I think this comment actually doesn't tell you anything besides what the property name and choices already tell you, so it could be removed. https://codereview.chromium.org/1566013002/diff/20001/dashboard/dashboard/mod... dashboard/dashboard/models/try_job.py:39: choices=['started', 'failed', 'staled', 'completed'], At first I was thinking "stale" might be better than "staled", since is a much more common word that staled (see https://www.google.com/trends/explore#q=stale%2C%20staled) and it's an adjective (not just a verb) so it works here. But now I think staled is also good, since it seems more consistent with the other status strings which all end in "ed". https://codereview.chromium.org/1566013002/diff/20001/dashboard/dashboard/pos... File dashboard/dashboard/post_bisect_results.py (right): https://codereview.chromium.org/1566013002/diff/20001/dashboard/dashboard/pos... dashboard/dashboard/post_bisect_results.py:121: job.put() When is the results posted to the issue tracker? https://codereview.chromium.org/1566013002/diff/20001/dashboard/dashboard/pos... dashboard/dashboard/post_bisect_results.py:122: update_bug_with_results.UpdateQuickLog(job) The name of this function indicates that all it does is update a TryJob entity with results_data, and updating the quick log seems to be a separate thing. Could this be moved outside (after _UpdateTryJob is called on line 71)? https://codereview.chromium.org/1566013002/diff/20001/dashboard/dashboard/pos... dashboard/dashboard/post_bisect_results.py:126: """Updates bisect job's result data with known values.""" What other parts of the bisect job's result data might be updated? If there's nothing else for now, then for now this function could be called something more specific, like _SetResultsIssueURL. Perhaps it could also take a results dict instead of a TryJob? https://codereview.chromium.org/1566013002/diff/20001/dashboard/dashboard/pos... dashboard/dashboard/post_bisect_results.py:131: def _IssueURL(job): Optional docstring: """Returns a URL for information about a bisect try job.""" https://codereview.chromium.org/1566013002/diff/20001/dashboard/dashboard/pos... File dashboard/dashboard/post_bisect_results_test.py (right): https://codereview.chromium.org/1566013002/diff/20001/dashboard/dashboard/pos... dashboard/dashboard/post_bisect_results_test.py:29: 'change': '', I wonder what the 'change' field is for -- is it regression vs improvement, increase vs decrease, or relative change as a percentage? https://codereview.chromium.org/1566013002/diff/20001/dashboard/dashboard/pos... dashboard/dashboard/post_bisect_results_test.py:60: }] If you think it improves readability you could change the formatting here to: 'revision_data': [ { 'depot_name': 'chromium', ... }, { 'depot_name': 'chromium', ... } ] (Generally I feel that being able to see structure through indentation level is more important that saving space.) https://codereview.chromium.org/1566013002/diff/20001/dashboard/dashboard/pos... dashboard/dashboard/post_bisect_results_test.py:61: } Closing brace should have no indent https://codereview.chromium.org/1566013002/diff/20001/dashboard/dashboard/pos... dashboard/dashboard/post_bisect_results_test.py:142: {'values': {'nested_values': 'orange'}}) Each of the failing and passing cases could be split into separate test methods, e.g. testValidate_StringNotInOptionList_Fails testValidate_StringInOptionList_Passes testValidate_IntRequiredStringGiven_Fails testValidate_StringRequiredStringGiven_Passes testValidate_TypeNoneStringGiven_Passes etc. The advantage of separating them into separate test methods is: 1. When it fails, it's clear from the method name which behavior isn't working 2. The test runner can run the methods in parallel, which is usually faster https://codereview.chromium.org/1566013002/diff/20001/dashboard/dashboard/upd... File dashboard/dashboard/update_bug_with_results.py (right): https://codereview.chromium.org/1566013002/diff/20001/dashboard/dashboard/upd... dashboard/dashboard/update_bug_with_results.py:1: # Copyright 2016 The Chromium Authors. All rights reserved. Summary of changes in this file: Removed: UnexpectedJsonError _CheckPerfTryJob _ParseCloudLinksFromOutput _LoadConfigFromString _GetPerfTryJobResults _GetBisectResults _FetchBuildData _GetBotFailureInfo _GetPartialBisectResult _ValidateAndConvertBuildbucketResponse _ValidateRietveldResponse _CheckBisectBotForInfraFailure _GetBisectScriptStepIndex _LogBisectInfraFailure _BisectResultIsPositive _BeautifyContent _FetchURL _FetchRietveldIssueJSON _RietveldIssueURL _BuildbucketStatusToStatusConstant Changed/renamed: _PostFailedResult, _PostSuccessfulResult -> _PostResult _GetReviewersFromBisectLog -> _GetReviewersFromCulpritData Added: _UpdateQuickLog Does that look right? Question: After this CL, tracing perf try job results (for try jobs starting using the trace button in the dashboard) won't be checked from here, right? If this is the case, then you should file a bug to make sure that trace perf try job results are surfaced; we may also want to send those to the dashboard. https://codereview.chromium.org/1566013002/diff/20001/dashboard/dashboard/upd... dashboard/dashboard/update_bug_with_results.py:316: return If a report is always expected when this function is called, this could potentially raise an error or log a warning. https://codereview.chromium.org/1566013002/diff/20001/dashboard/dashboard/upd... File dashboard/dashboard/update_bug_with_results_test.py (right): https://codereview.chromium.org/1566013002/diff/20001/dashboard/dashboard/upd... dashboard/dashboard/update_bug_with_results_test.py:69: } Formatting; see post_bisect_results_test.py
https://chromiumcodereview-hr.appspot.com/1566013002/diff/20001/dashboard/das... File dashboard/dashboard/bisect_report.py (right): https://chromiumcodereview-hr.appspot.com/1566013002/diff/20001/dashboard/das... dashboard/dashboard/bisect_report.py:9: _CONFIDENCE_THRESHOLD = 99.5 On 2016/01/18 21:10:24, qyearsley wrote: > This appears to be unused here now -- this "confidence threshold" is used to > determine whether the bisect results "status" should be "Positive or "Negative". > > In the current draft of the auto-bisect side changes > (https://codereview.chromium.org/1573293002), this is changed so that status > doesn't indicate "Positive" or "Negative". > > I think it seems OK to put the deciding of positive/negative status on the > dashboard side, but it's also OK to decide on the bisect side and send it over. > > Note that the "Positive"/"Negative" status was used to decide whether to update > bugs or not -- bugs are only updated for positive results. I think we want to > keep that behavior. Acknowledged. https://chromiumcodereview-hr.appspot.com/1566013002/diff/20001/dashboard/das... File dashboard/dashboard/bisect_report_test.py (right): https://chromiumcodereview-hr.appspot.com/1566013002/diff/20001/dashboard/das... dashboard/dashboard/bisect_report_test.py:64: } On 2016/01/18 21:10:24, qyearsley wrote: > Formatting should be adjusted here; see comments in post_bisect_results_test.py Done. https://chromiumcodereview-hr.appspot.com/1566013002/diff/20001/dashboard/das... dashboard/dashboard/bisect_report_test.py:186: self.assertEqual(_LOG_FAILED_BISECT, bisect_report.GetReport(job)) On 2016/01/18 21:10:24, qyearsley wrote: > I think that the literal expected string directly in the test method here is a > good idea, even if it's very long and involves multi-line strings -- the reason > is that this puts the example expected string in the context of where it's > expected, and means that the reader doesn't need to scroll up and down. > > Additionally, in some cases instead of using > > self.assertEqual(<full expected text>, bisect_report.GetReport(job)) > > it might be less fragile while keeping the same coverage if we use > > self.assertIn(<key part>, bisect_report.GetReport(job)) Done. Though I think tests can be use to see examples and it would be easier on top, plus it would be re-usable and easy to update. https://chromiumcodereview-hr.appspot.com/1566013002/diff/20001/dashboard/das... File dashboard/dashboard/post_bisect_results.py (right): https://chromiumcodereview-hr.appspot.com/1566013002/diff/20001/dashboard/das... dashboard/dashboard/post_bisect_results.py:121: job.put() On 2016/01/18 21:10:25, qyearsley wrote: > When is the results posted to the issue tracker? In update_bug_with_results.py cron job. https://chromiumcodereview-hr.appspot.com/1566013002/diff/20001/dashboard/das... dashboard/dashboard/post_bisect_results.py:122: update_bug_with_results.UpdateQuickLog(job) On 2016/01/18 21:10:24, qyearsley wrote: > The name of this function indicates that all it does is update a TryJob entity > with results_data, and updating the quick log seems to be a separate thing. > Could this be moved outside (after _UpdateTryJob is called on line 71)? Done. https://chromiumcodereview-hr.appspot.com/1566013002/diff/20001/dashboard/das... dashboard/dashboard/post_bisect_results.py:126: """Updates bisect job's result data with known values.""" On 2016/01/18 21:10:24, qyearsley wrote: > What other parts of the bisect job's result data might be updated? There were a few stuff, moved it to the bot side. I think there are others in the future. > > If there's nothing else for now, then for now this function could be called > something more specific, like _SetResultsIssueURL. Done. Moved to _UpdateTryJob. > > Perhaps it could also take a results dict instead of a TryJob? https://chromiumcodereview-hr.appspot.com/1566013002/diff/20001/dashboard/das... dashboard/dashboard/post_bisect_results.py:131: def _IssueURL(job): On 2016/01/18 21:10:25, qyearsley wrote: > Optional docstring: > > """Returns a URL for information about a bisect try job.""" Done. https://chromiumcodereview-hr.appspot.com/1566013002/diff/20001/dashboard/das... File dashboard/dashboard/post_bisect_results_test.py (right): https://chromiumcodereview-hr.appspot.com/1566013002/diff/20001/dashboard/das... dashboard/dashboard/post_bisect_results_test.py:29: 'change': '', On 2016/01/18 21:10:25, qyearsley wrote: > I wonder what the 'change' field is for -- is it regression vs improvement, > increase vs decrease, or relative change as a percentage? I see relative change percentage or string message for legacy. And relative change percentage for bisect recipe. https://chromiumcodereview-hr.appspot.com/1566013002/diff/20001/dashboard/das... dashboard/dashboard/post_bisect_results_test.py:60: }] On 2016/01/18 21:10:25, qyearsley wrote: > If you think it improves readability you could change the formatting here to: > > 'revision_data': [ > { > 'depot_name': 'chromium', > ... > }, > { > 'depot_name': 'chromium', > ... > } > ] > > (Generally I feel that being able to see structure through indentation level is > more important that saving space.) Done. https://chromiumcodereview-hr.appspot.com/1566013002/diff/20001/dashboard/das... dashboard/dashboard/post_bisect_results_test.py:61: } On 2016/01/18 21:10:25, qyearsley wrote: > Closing brace should have no indent Done. https://chromiumcodereview-hr.appspot.com/1566013002/diff/20001/dashboard/das... dashboard/dashboard/post_bisect_results_test.py:142: {'values': {'nested_values': 'orange'}}) On 2016/01/18 21:10:25, qyearsley wrote: > Each of the failing and passing cases could be split into separate test methods, > e.g. > > testValidate_StringNotInOptionList_Fails > testValidate_StringInOptionList_Passes > testValidate_IntRequiredStringGiven_Fails > testValidate_StringRequiredStringGiven_Passes > testValidate_TypeNoneStringGiven_Passes > etc. > > The advantage of separating them into separate test methods is: > 1. When it fails, it's clear from the method name which behavior isn't working > 2. The test runner can run the methods in parallel, which is usually faster Done. https://chromiumcodereview-hr.appspot.com/1566013002/diff/20001/dashboard/das... File dashboard/dashboard/update_bug_with_results.py (right): https://chromiumcodereview-hr.appspot.com/1566013002/diff/20001/dashboard/das... dashboard/dashboard/update_bug_with_results.py:1: # Copyright 2016 The Chromium Authors. All rights reserved. On 2016/01/18 21:10:25, qyearsley wrote: > Summary of changes in this file: > > Removed: > UnexpectedJsonError > _CheckPerfTryJob > _ParseCloudLinksFromOutput > _LoadConfigFromString > _GetPerfTryJobResults > _GetBisectResults > _FetchBuildData > _GetBotFailureInfo > _GetPartialBisectResult > _ValidateAndConvertBuildbucketResponse > _ValidateRietveldResponse > _CheckBisectBotForInfraFailure > _GetBisectScriptStepIndex > _LogBisectInfraFailure > _BisectResultIsPositive > _BeautifyContent > _FetchURL > _FetchRietveldIssueJSON > _RietveldIssueURL > _BuildbucketStatusToStatusConstant > > Changed/renamed: > _PostFailedResult, _PostSuccessfulResult -> _PostResult > _GetReviewersFromBisectLog -> _GetReviewersFromCulpritData > > Added: > _UpdateQuickLog > > Does that look right? That's correct. > > Question: After this CL, tracing perf try job results (for try jobs starting > using the trace button in the dashboard) won't be checked from here, right? If > this is the case, then you should file a bug to make sure that trace perf try > job results are surfaced; we may also want to send those to the dashboard. Perf try results should be uploaded as well in this CL: 1573293002, unless I'm missing something? https://chromiumcodereview-hr.appspot.com/1566013002/diff/20001/dashboard/das... dashboard/dashboard/update_bug_with_results.py:316: return On 2016/01/18 21:10:25, qyearsley wrote: > If a report is always expected when this function is called, this could > potentially raise an error or log a warning. Done. https://chromiumcodereview-hr.appspot.com/1566013002/diff/20001/dashboard/das... File dashboard/dashboard/update_bug_with_results_test.py (right): https://chromiumcodereview-hr.appspot.com/1566013002/diff/20001/dashboard/das... dashboard/dashboard/update_bug_with_results_test.py:69: } On 2016/01/18 21:10:25, qyearsley wrote: > Formatting; see post_bisect_results_test.py Done.
https://codereview.chromium.org/1566013002/diff/1/dashboard/dashboard/bisect_... File dashboard/dashboard/bisect_report_test.py (right): https://codereview.chromium.org/1566013002/diff/1/dashboard/dashboard/bisect_... dashboard/dashboard/bisect_report_test.py:159: pass On 2016/01/13 00:32:54, chrisphan wrote: > On 2016/01/11 22:16:44, qyearsley wrote: > > Pending implementation. > > > > When you implement this, it may be a good idea to avoid big global strings, > > because they tend to make the tests more fragile and harder to read. > > > > Even if you use a big literal string, if possible it's good to put them in the > > one test method where they're used, and when possible, it's less fragile to > > assert that just one or two key things is contained in the string. > > Agree, in this case, I think it's useful to see the expected layout in a bug > message. LMK. Putting the expected layout in a bug message is also OK, and I agree that it's helpful to see at least one full expected bug message string in the test. https://codereview.chromium.org/1566013002/diff/1/dashboard/dashboard/post_bi... File dashboard/dashboard/post_bisect_results.py (right): https://codereview.chromium.org/1566013002/diff/1/dashboard/dashboard/post_bi... dashboard/dashboard/post_bisect_results.py:40: """Validates data parameter and save to TryJob entity. On 2016/01/13 00:35:35, chrisphan wrote: > On 2016/01/13 00:32:54, chrisphan wrote: > > On 2016/01/11 22:16:45, qyearsley wrote: > > > s/save/saves/ > > > > > > Is there something that implicitly happens when _UpdateTryJob is called? > When > > is > > > a post made to crbug? > > > > This handler will serve to bots updating partial and full results. So we'll > > simply validate and store the results. Posting to crbug will happen in the > same > > place update_bug_with_results.py. > > Also, we are updating quick-log. So at all time, quick-log will have the latest > bisect results. Ah, that makes sense :-) https://codereview.chromium.org/1566013002/diff/1/dashboard/dashboard/post_bi... dashboard/dashboard/post_bisect_results.py:42: Bisect results come from a "data" parameter, which is a JSON encoding of a On 2016/01/13 00:32:54, chrisphan wrote: > On 2016/01/11 22:16:45, qyearsley wrote: > > I advocate changing the name from "data" to "bisect_results". > > This comes from a generic post_json.py which is shared with posting data to > /add_point. Alright, seems good, especially since it's noted here that it's a JSON dict with fields "master", "bot" and "test". https://codereview.chromium.org/1566013002/diff/1/dashboard/dashboard/update_... File dashboard/dashboard/update_bug_with_results_test.py (right): https://codereview.chromium.org/1566013002/diff/1/dashboard/dashboard/update_... dashboard/dashboard/update_bug_with_results_test.py:1: # Copyright 2016 The Chromium Authors. All rights reserved. On 2016/01/06 23:28:10, chrisphan wrote: > Currently be worked on: > - update tests that are left here > - add applicable new tests > > The tests that were removed are tests that are no applicable anymore. For > example testGet_PerfTryJob, asserts bisect results data exist, but with posting > method, data is only as good as that bots posting them. > Acknowledged. https://codereview.chromium.org/1566013002/diff/20001/dashboard/dashboard/bis... File dashboard/dashboard/bisect_report.py (right): https://codereview.chromium.org/1566013002/diff/20001/dashboard/dashboard/bis... dashboard/dashboard/bisect_report.py:9: _CONFIDENCE_THRESHOLD = 99.5 On 2016/01/20 21:47:18, chrisphan wrote: > On 2016/01/18 21:10:24, qyearsley wrote: > > This appears to be unused here now -- this "confidence threshold" is used to > > determine whether the bisect results "status" should be "Positive or > "Negative". > > > > In the current draft of the auto-bisect side changes > > (https://codereview.chromium.org/1573293002), this is changed so that status > > doesn't indicate "Positive" or "Negative". > > > > I think it seems OK to put the deciding of positive/negative status on the > > dashboard side, but it's also OK to decide on the bisect side and send it > over. > > > > Note that the "Positive"/"Negative" status was used to decide whether to > update > > bugs or not -- bugs are only updated for positive results. I think we want to > > keep that behavior. > > Acknowledged. _CONFIDENCE_THRESHOLD is still unused, and should be removed if it's unused. https://codereview.chromium.org/1566013002/diff/20001/dashboard/dashboard/bis... File dashboard/dashboard/bisect_report_test.py (right): https://codereview.chromium.org/1566013002/diff/20001/dashboard/dashboard/bis... dashboard/dashboard/bisect_report_test.py:186: self.assertEqual(_LOG_FAILED_BISECT, bisect_report.GetReport(job)) On 2016/01/20 21:47:19, chrisphan wrote: > On 2016/01/18 21:10:24, qyearsley wrote: > > I think that the literal expected string directly in the test method here is a > > good idea, even if it's very long and involves multi-line strings -- the > reason > > is that this puts the example expected string in the context of where it's > > expected, and means that the reader doesn't need to scroll up and down. > > > > Additionally, in some cases instead of using > > > > self.assertEqual(<full expected text>, bisect_report.GetReport(job)) > > > > it might be less fragile while keeping the same coverage if we use > > > > self.assertIn(<key part>, bisect_report.GetReport(job)) > > Done. Though I think tests can be use to see examples and it would be easier on > top, plus it would be re-usable and easy to update. That's true, I agree. Although, in the case of start_try_job_test and update_bug_with_results_test, there are lot of similar, long samples and when reading the test methods below, I felt like I had to jump up and down to see what was being tested, https://codereview.chromium.org/1566013002/diff/20001/dashboard/dashboard/pos... File dashboard/dashboard/post_bisect_results.py (right): https://codereview.chromium.org/1566013002/diff/20001/dashboard/dashboard/pos... dashboard/dashboard/post_bisect_results.py:121: job.put() On 2016/01/20 21:47:19, chrisphan wrote: > On 2016/01/18 21:10:25, qyearsley wrote: > > When is the results posted to the issue tracker? > > In update_bug_with_results.py cron job. Since this is one thing that I was uncertain about while reading the code initially, I think it would be helpful to add a note here saying that the results will actually be posted to the bug in update_bug_with_results. Alternatively, we could also consider posting the results within this same request. Possible advantages: (1) full results will be posted as soon as they're ready (2) this may allow the bisect to know whether or not the bisect failed to be posted, and potentially output a warning there. Either way is fine though. https://codereview.chromium.org/1566013002/diff/20001/dashboard/dashboard/pos... File dashboard/dashboard/post_bisect_results_test.py (right): https://codereview.chromium.org/1566013002/diff/20001/dashboard/dashboard/pos... dashboard/dashboard/post_bisect_results_test.py:29: 'change': '', On 2016/01/20 21:47:19, chrisphan wrote: > On 2016/01/18 21:10:25, qyearsley wrote: > > I wonder what the 'change' field is for -- is it regression vs improvement, > > increase vs decrease, or relative change as a percentage? > > I see relative change percentage or string message for legacy. And relative > change percentage for bisect recipe. Ah, right. Could you put in a sample number (e.g "10%")? https://codereview.chromium.org/1566013002/diff/40001/dashboard/dashboard/bis... File dashboard/dashboard/bisect_report_test.py (right): https://codereview.chromium.org/1566013002/diff/40001/dashboard/dashboard/bis... dashboard/dashboard/bisect_report_test.py:47: 'result': 'good' Indentation https://codereview.chromium.org/1566013002/diff/40001/dashboard/dashboard/bis... dashboard/dashboard/bisect_report_test.py:56: },{ There should be a space after the comma. Also, I slightly prefer putting the opening brace on its own line, e.g. { ... }, { ... }, (This style makes it slightly easier to copy/cut/delete one dict in a list) https://codereview.chromium.org/1566013002/diff/40001/dashboard/dashboard/pos... File dashboard/dashboard/post_bisect_results_test.py (right): https://codereview.chromium.org/1566013002/diff/40001/dashboard/dashboard/pos... dashboard/dashboard/post_bisect_results_test.py:152: {'values': {'nested_values': 'orange'}}) I like the way these tests are written now :-) Of course, now these tests are in utils_test and after rebasing, you can now use utils.Validate in post_bisect_results. https://codereview.chromium.org/1566013002/diff/40001/dashboard/dashboard/upd... File dashboard/dashboard/update_bug_with_results.py (right): https://codereview.chromium.org/1566013002/diff/40001/dashboard/dashboard/upd... dashboard/dashboard/update_bug_with_results.py:259: emails = [culprit_data['email']] or [] This will always be equivalent to [culprit_data['email']]. Even if culprit_data['email'] is None, a non-empty array is always considered true so the other side of the `or` will never be used. Maybe this should be: emails = [culprit_data['email']] if culprit_data['email'] else []
Rebased. https://codereview.chromium.org/1566013002/diff/40001/dashboard/dashboard/bis... File dashboard/dashboard/bisect_report_test.py (right): https://codereview.chromium.org/1566013002/diff/40001/dashboard/dashboard/bis... dashboard/dashboard/bisect_report_test.py:47: 'result': 'good' On 2016/01/26 18:43:27, qyearsley wrote: > Indentation Done. https://codereview.chromium.org/1566013002/diff/40001/dashboard/dashboard/bis... dashboard/dashboard/bisect_report_test.py:56: },{ On 2016/01/26 18:43:27, qyearsley wrote: > There should be a space after the comma. > > Also, I slightly prefer putting the opening brace on its own line, e.g. > > { > ... > }, > { > ... > }, > > (This style makes it slightly easier to copy/cut/delete one dict in a list) Done. https://codereview.chromium.org/1566013002/diff/40001/dashboard/dashboard/pos... File dashboard/dashboard/post_bisect_results_test.py (right): https://codereview.chromium.org/1566013002/diff/40001/dashboard/dashboard/pos... dashboard/dashboard/post_bisect_results_test.py:152: {'values': {'nested_values': 'orange'}}) On 2016/01/26 18:43:27, qyearsley wrote: > I like the way these tests are written now :-) > > Of course, now these tests are in utils_test and after rebasing, you can now use > utils.Validate in post_bisect_results. Done. https://codereview.chromium.org/1566013002/diff/40001/dashboard/dashboard/upd... File dashboard/dashboard/update_bug_with_results.py (right): https://codereview.chromium.org/1566013002/diff/40001/dashboard/dashboard/upd... dashboard/dashboard/update_bug_with_results.py:259: emails = [culprit_data['email']] or [] On 2016/01/26 18:43:27, qyearsley wrote: > This will always be equivalent to [culprit_data['email']]. > > Even if culprit_data['email'] is None, a non-empty array is always considered > true so the other side of the `or` will never be used. > > Maybe this should be: > > emails = [culprit_data['email']] if culprit_data['email'] else [] Woops. Done. https://codereview.chromium.org/1566013002/diff/60001/dashboard/dashboard/pos... File dashboard/dashboard/post_bisect_results.py (right): https://codereview.chromium.org/1566013002/diff/60001/dashboard/dashboard/pos... dashboard/dashboard/post_bisect_results.py:70: if not job: Going to give 200 status code for non-existing job since perf-try may not have one (for now).
https://codereview.chromium.org/1566013002/diff/60001/dashboard/dashboard/bis... File dashboard/dashboard/bisect_fyi.py (right): https://codereview.chromium.org/1566013002/diff/60001/dashboard/dashboard/bis... dashboard/dashboard/bisect_fyi.py:128: expected = set(config.keys()) Here we actually want to compare the values for the "expected_results" keys. And tries to match if the value is present in the bisect results. We don't exactly match the keys of bisect fyi config and results keys. https://chromeperf.appspot.com/edit_site_config?key=bisect_fyi_config_map https://codereview.chromium.org/1566013002/diff/60001/dashboard/dashboard/upd... File dashboard/dashboard/update_bug_with_results.py (right): https://codereview.chromium.org/1566013002/diff/60001/dashboard/dashboard/upd... dashboard/dashboard/update_bug_with_results.py:137: _SendFYIBisectEmail(job, error_message) How do we make sure bisect FYI jobs are not retried by auto_bisect/ handler. Every time we create run bisect FYI we want to create a new tryjob entity.
https://chromiumcodereview-hr.appspot.com/1566013002/diff/60001/dashboard/das... File dashboard/dashboard/bisect_fyi.py (right): https://chromiumcodereview-hr.appspot.com/1566013002/diff/60001/dashboard/das... dashboard/dashboard/bisect_fyi.py:128: expected = set(config.keys()) On 2016/02/09 21:09:41, prasadv wrote: > Here we actually want to compare the values for the "expected_results" keys. And > tries to match if the value is present in the bisect results. We don't exactly > match the keys of bisect fyi config and results keys. > > https://chromeperf.appspot.com/edit_site_config?key=bisect_fyi_config_map For a second I forgot why I'm doing this. So we bisect results are in totally different format now (see post_bisect_results_test.py). I did lazy check here until we're solid of what we expect from the bisect result dict. I just updated to check values as well since we have new tool, and we'll update "bisect_fyi_config_map" once this version is deploy live. https://chromiumcodereview-hr.appspot.com/1566013002/diff/60001/dashboard/das... File dashboard/dashboard/update_bug_with_results.py (right): https://chromiumcodereview-hr.appspot.com/1566013002/diff/60001/dashboard/das... dashboard/dashboard/update_bug_with_results.py:137: _SendFYIBisectEmail(job, error_message) On 2016/02/09 21:09:41, prasadv wrote: > How do we make sure bisect FYI jobs are not retried by auto_bisect/ handler. This can be done in auto_bisect and checking for job type. > Every time we create run bisect FYI we want to create a new tryjob entity. Quick look at bisect_fy.py, it should already creating new TryJob.
https://chromiumcodereview-hr.appspot.com/1566013002/diff/100001/dashboard/da... File dashboard/dashboard/update_bug_with_results.py (right): https://chromiumcodereview-hr.appspot.com/1566013002/diff/100001/dashboard/da... dashboard/dashboard/update_bug_with_results.py:260: if results_data.get('score') < _CONFIDENCE_LEVEL_TO_CC_AUTHOR: Check confidence level before ccing author. This comes from legacy bisect's bisect_printer.py.
On 2016/02/09 23:42:36, chrisphan wrote: > https://chromiumcodereview-hr.appspot.com/1566013002/diff/100001/dashboard/da... > File dashboard/dashboard/update_bug_with_results.py (right): > > https://chromiumcodereview-hr.appspot.com/1566013002/diff/100001/dashboard/da... > dashboard/dashboard/update_bug_with_results.py:260: if results_data.get('score') > < _CONFIDENCE_LEVEL_TO_CC_AUTHOR: > Check confidence level before ccing author. This comes from legacy bisect's > bisect_printer.py. Ready to deploy this change?
On 2016/02/16 19:13:36, chrisphan wrote: > On 2016/02/09 23:42:36, chrisphan wrote: > > > https://chromiumcodereview-hr.appspot.com/1566013002/diff/100001/dashboard/da... > > File dashboard/dashboard/update_bug_with_results.py (right): > > > > > https://chromiumcodereview-hr.appspot.com/1566013002/diff/100001/dashboard/da... > > dashboard/dashboard/update_bug_with_results.py:260: if > results_data.get('score') > > < _CONFIDENCE_LEVEL_TO_CC_AUTHOR: > > Check confidence level before ccing author. This comes from legacy bisect's > > bisect_printer.py. > > Ready to deploy this change? Yep, ready -- LGTM
The CQ bit was checked by chrisphan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from qyearsley@chromium.org Link to the patchset: https://chromiumcodereview-hr.appspot.com/1566013002/#ps140001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1566013002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1566013002/140001
Message was sent while issue was closed.
Description was changed from ========== Add support for bisect bots to post results to dashboard. High level changes: update_bug_with_results.py - Removed the need to query rietveld, buildbucket, and buildbots. This will make it simpler. We want to get all the necessary results in one place which is from the bots. - Removed tracking infra failure. Current method is not very reliable. - TryJobs now do not get removed. post_bisect_results.py - To make it simple, we verify the data, and save it directly to a datastore JSONProperty. bisect_report.py - This is where we create report base on TryJob states, whether they failed, staled, or completed. try_job.py - Added 'results_data' which is data directly from bisect bots. - Added a 'staled' state. BUG=catapult:#1869 ========== to ========== Add support for bisect bots to post results to dashboard. High level changes: update_bug_with_results.py - Removed the need to query rietveld, buildbucket, and buildbots. This will make it simpler. We want to get all the necessary results in one place which is from the bots. - Removed tracking infra failure. Current method is not very reliable. - TryJobs now do not get removed. post_bisect_results.py - To make it simple, we verify the data, and save it directly to a datastore JSONProperty. bisect_report.py - This is where we create report base on TryJob states, whether they failed, staled, or completed. try_job.py - Added 'results_data' which is data directly from bisect bots. - Added a 'staled' state. BUG=catapult:#1869 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
