Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
284493005:
Add methods to get build status from tryserver perf bisect builders. (Closed)
|
Created:
6 years, 7 months ago by prasadv Modified:
6 years, 7 months ago CC:
chromium-reviews, shatch Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd methods to get build status from tryserver perf bisect builders.
These methods will be used bisect script to track the status of build.
BUG=
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270865
Patch Set 1 #
Total comments: 18
Patch Set 2 : #Patch Set 3 : #Messages
Total messages: 10 (0 generated)
lg with question https://chromiumcodereview.appspot.com/284493005/diff/1/tools/post_perf_build... File tools/post_perf_builder_job.py (right): https://chromiumcodereview.appspot.com/284493005/diff/1/tools/post_perf_build... tools/post_perf_builder_job.py:160: item.get('results')[0] in OK): OK includes SKIPPED, did you want to include that here? https://chromiumcodereview.appspot.com/284493005/diff/1/tools/post_perf_build... tools/post_perf_builder_job.py:235: A tuple consists of build status (SUCCESS, FAILED or PENDING ) and a link nit: remove space https://chromiumcodereview.appspot.com/284493005/diff/1/tools/post_perf_build... tools/post_perf_builder_job.py:260: def GetBuildNumFromBuilder(build_reason, bot_name, builder_host, builder_port): just checking, these methods aren't used yet right?
Looks good, the functions are all short and well-commented. Got a couple nits before committing. https://codereview.chromium.org/284493005/diff/1/tools/post_perf_builder_job.py File tools/post_perf_builder_job.py (right): https://codereview.chromium.org/284493005/diff/1/tools/post_perf_builder_job.... tools/post_perf_builder_job.py:15: Technically, I think there's no need for two blank lines here (or on line 31). (However, the Chromium style guide (and Python style guide PEP-8) doesn't explicitly say how many blank lines to put here.) https://codereview.chromium.org/284493005/diff/1/tools/post_perf_builder_job.... tools/post_perf_builder_job.py:106: build_data: JSON object with build data, loaded from buildbot JSON API. It could be slightly confusing to say that the expected argument is JSON because that suggests that it is a string, but what's actually expected is a dictionary. Same applies for the two functions below. https://codereview.chromium.org/284493005/diff/1/tools/post_perf_builder_job.... tools/post_perf_builder_job.py:109: Returns True if build is in progress, otherwise False. The word "Returns" is redundant here. Same applies for the two functions below. https://codereview.chromium.org/284493005/diff/1/tools/post_perf_builder_job.... tools/post_perf_builder_job.py:139: def _IsBuildSucceed(build_data): Should rename to _IsBuildSuccessful. (Also update in the docstring of _IsBuildFailed.) https://codereview.chromium.org/284493005/diff/1/tools/post_perf_builder_job.... tools/post_perf_builder_job.py:257: return (PENDING, results_url) The function _IsBuildRunning isn't used anywhere currently, maybe it can be removed? https://codereview.chromium.org/284493005/diff/1/tools/post_perf_builder_job.... tools/post_perf_builder_job.py:276: A build number as a string if found, otherwise None Could add a period on this line.
https://chromiumcodereview.appspot.com/284493005/diff/1/tools/post_perf_build... File tools/post_perf_builder_job.py (right): https://chromiumcodereview.appspot.com/284493005/diff/1/tools/post_perf_build... tools/post_perf_builder_job.py:15: On 2014/05/15 19:40:06, qyearsley wrote: > Technically, I think there's no need for two blank lines here (or on line 31). > (However, the Chromium style guide (and Python style guide PEP-8) doesn't > explicitly say how many blank lines to put here.) Done. https://chromiumcodereview.appspot.com/284493005/diff/1/tools/post_perf_build... tools/post_perf_builder_job.py:106: build_data: JSON object with build data, loaded from buildbot JSON API. On 2014/05/15 19:40:06, qyearsley wrote: > It could be slightly confusing to say that the expected argument is JSON because > that suggests that it is a string, but what's actually expected is a dictionary. > Same applies for the two functions below. Done. https://chromiumcodereview.appspot.com/284493005/diff/1/tools/post_perf_build... tools/post_perf_builder_job.py:109: Returns True if build is in progress, otherwise False. On 2014/05/15 19:40:06, qyearsley wrote: > The word "Returns" is redundant here. Same applies for the two functions below. Done. https://chromiumcodereview.appspot.com/284493005/diff/1/tools/post_perf_build... tools/post_perf_builder_job.py:139: def _IsBuildSucceed(build_data): On 2014/05/15 19:40:06, qyearsley wrote: > Should rename to _IsBuildSuccessful. (Also update in the docstring of > _IsBuildFailed.) Done. https://chromiumcodereview.appspot.com/284493005/diff/1/tools/post_perf_build... tools/post_perf_builder_job.py:160: item.get('results')[0] in OK): I'm really not sure why a build could be skipped, I'm assuming since we are explicitly posting a build request, builder should build it anyhow. Should we treat skipped build as failure? On 2014/05/15 19:27:55, stip wrote: > OK includes SKIPPED, did you want to include that here? https://chromiumcodereview.appspot.com/284493005/diff/1/tools/post_perf_build... tools/post_perf_builder_job.py:235: A tuple consists of build status (SUCCESS, FAILED or PENDING ) and a link On 2014/05/15 19:27:55, stip wrote: > nit: remove space Done. https://chromiumcodereview.appspot.com/284493005/diff/1/tools/post_perf_build... tools/post_perf_builder_job.py:257: return (PENDING, results_url) Added this to check if build is in pending queue or in build progress mode. I might be use this to make sure build is actually running. On 2014/05/15 19:40:06, qyearsley wrote: > The function _IsBuildRunning isn't used anywhere currently, maybe it can be > removed? https://chromiumcodereview.appspot.com/284493005/diff/1/tools/post_perf_build... tools/post_perf_builder_job.py:260: def GetBuildNumFromBuilder(build_reason, bot_name, builder_host, builder_port): Yes, these aren't used anywhere yet. On 2014/05/15 19:27:55, stip wrote: > just checking, these methods aren't used yet right? https://chromiumcodereview.appspot.com/284493005/diff/1/tools/post_perf_build... tools/post_perf_builder_job.py:276: A build number as a string if found, otherwise None On 2014/05/15 19:40:06, qyearsley wrote: > Could add a period on this line. Done.
On 2014/05/15 20:09:42, prasadv wrote: > https://chromiumcodereview.appspot.com/284493005/diff/1/tools/post_perf_build... > File tools/post_perf_builder_job.py (right): > > https://chromiumcodereview.appspot.com/284493005/diff/1/tools/post_perf_build... > tools/post_perf_builder_job.py:15: > On 2014/05/15 19:40:06, qyearsley wrote: > > Technically, I think there's no need for two blank lines here (or on line 31). > > (However, the Chromium style guide (and Python style guide PEP-8) doesn't > > explicitly say how many blank lines to put here.) > > Done. > > https://chromiumcodereview.appspot.com/284493005/diff/1/tools/post_perf_build... > tools/post_perf_builder_job.py:106: build_data: JSON object with build data, > loaded from buildbot JSON API. > On 2014/05/15 19:40:06, qyearsley wrote: > > It could be slightly confusing to say that the expected argument is JSON > because > > that suggests that it is a string, but what's actually expected is a > dictionary. > > Same applies for the two functions below. > > Done. > > https://chromiumcodereview.appspot.com/284493005/diff/1/tools/post_perf_build... > tools/post_perf_builder_job.py:109: Returns True if build is in progress, > otherwise False. > On 2014/05/15 19:40:06, qyearsley wrote: > > The word "Returns" is redundant here. Same applies for the two functions > below. > > Done. > > https://chromiumcodereview.appspot.com/284493005/diff/1/tools/post_perf_build... > tools/post_perf_builder_job.py:139: def _IsBuildSucceed(build_data): > On 2014/05/15 19:40:06, qyearsley wrote: > > Should rename to _IsBuildSuccessful. (Also update in the docstring of > > _IsBuildFailed.) > > Done. > > https://chromiumcodereview.appspot.com/284493005/diff/1/tools/post_perf_build... > tools/post_perf_builder_job.py:160: item.get('results')[0] in OK): > I'm really not sure why a build could be skipped, I'm assuming since we are > explicitly posting a build request, builder should build it anyhow. > > Should we treat skipped build as failure? > > On 2014/05/15 19:27:55, stip wrote: > > OK includes SKIPPED, did you want to include that here? > > https://chromiumcodereview.appspot.com/284493005/diff/1/tools/post_perf_build... > tools/post_perf_builder_job.py:235: A tuple consists of build status (SUCCESS, > FAILED or PENDING ) and a link > On 2014/05/15 19:27:55, stip wrote: > > nit: remove space > > Done. > > https://chromiumcodereview.appspot.com/284493005/diff/1/tools/post_perf_build... > tools/post_perf_builder_job.py:257: return (PENDING, results_url) > Added this to check if build is in pending queue or in build progress mode. I > might be use this to make sure build is actually running. > > On 2014/05/15 19:40:06, qyearsley wrote: > > The function _IsBuildRunning isn't used anywhere currently, maybe it can be > > removed? > > https://chromiumcodereview.appspot.com/284493005/diff/1/tools/post_perf_build... > tools/post_perf_builder_job.py:260: def GetBuildNumFromBuilder(build_reason, > bot_name, builder_host, builder_port): > Yes, these aren't used anywhere yet. > On 2014/05/15 19:27:55, stip wrote: > > just checking, these methods aren't used yet right? > > https://chromiumcodereview.appspot.com/284493005/diff/1/tools/post_perf_build... > tools/post_perf_builder_job.py:276: A build number as a string if found, > otherwise None > On 2014/05/15 19:40:06, qyearsley wrote: > > Could add a period on this line. > > Done. LGTM, should first make sure we know what we want to do in case of "SKIPPED" though.
lgtm with moving SKIPPED to count as a failure
Done, now SKIPPED is counted as failure.
The CQ bit was checked by prasadv@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/prasadv@chromium.org/284493005/30001
Message was sent while issue was closed.
Change committed as 270865 |
