Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(929)

Issue 1044263002: Pull BuildFailurePipeline to a separate file and add test cases. (Closed)

Created:
5 years, 8 months ago by chanli
Modified:
5 years, 8 months ago
Reviewers:
stgao, qyearsley
CC:
chromium-reviews
Base URL:
https://chromium.googlesource.com/infra/infra.git@master
Target Ref:
refs/heads/master
Project:
infra
Visibility:
Public.

Description

Pull BuildFailurePipeline to a separate file and add test cases. TBR=stgao Committed: https://chromium.googlesource.com/infra/infra/+/a2075cdd3a3421b2b5c34482b7104fe559e533f6

Patch Set 1 #

Total comments: 22

Patch Set 2 : Refactor the unittest in appengine/findit/waterfall/test/build_failure_analysis_pipelines_test.py t… #

Total comments: 12
Unified diffs Side-by-side diffs Delta from patch set Stats (+155 lines, -193 lines) Patch
A appengine/findit/waterfall/analyze_build_failure_pipeline.py View 1 1 chunk +61 lines, -0 lines 2 comments Download
M appengine/findit/waterfall/build_failure_analysis_pipelines.py View 1 2 chunks +3 lines, -49 lines 2 comments Download
A + appengine/findit/waterfall/test/analyze_build_failure_pipeline_test.py View 1 5 chunks +78 lines, -70 lines 5 comments Download
M appengine/findit/waterfall/test/build_failure_analysis_pipelines_test.py View 1 2 chunks +13 lines, -74 lines 3 comments Download

Messages

Total messages: 23 (4 generated)
chanli
Hi Shuotao and Quinten, Could you review this cl when you have time? This cl ...
5 years, 8 months ago (2015-03-31 17:46:02 UTC) #2
stgao
https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/build_failure_pipeline.py File appengine/findit/waterfall/build_failure_pipeline.py (right): https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/build_failure_pipeline.py#newcode15 appengine/findit/waterfall/build_failure_pipeline.py:15: class BuildFailurePipeline(BasePipeline): nit: to be consistent with other pipelines, ...
5 years, 8 months ago (2015-03-31 21:43:26 UTC) #3
chanli
https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/build_failure_pipeline.py File appengine/findit/waterfall/build_failure_pipeline.py (right): https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/build_failure_pipeline.py#newcode24 appengine/findit/waterfall/build_failure_pipeline.py:24: def LogUnexpectedAborting(self, was_aborted): On 2015/03/31 21:43:24, Shuotao wrote: > ...
5 years, 8 months ago (2015-03-31 21:51:05 UTC) #4
qyearsley
https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/build_failure_pipeline.py File appengine/findit/waterfall/build_failure_pipeline.py (right): https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/build_failure_pipeline.py#newcode28 appengine/findit/waterfall/build_failure_pipeline.py:28: analysis as ERROR. The analysis is created before the ...
5 years, 8 months ago (2015-03-31 22:00:58 UTC) #5
chanli
https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/build_failure_pipeline.py File appengine/findit/waterfall/build_failure_pipeline.py (right): https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/build_failure_pipeline.py#newcode42 appengine/findit/waterfall/build_failure_pipeline.py:42: self.LogUnexpectedAborting(self.was_aborted) On 2015/03/31 22:00:58, qyearsley wrote: > How about: ...
5 years, 8 months ago (2015-03-31 22:19:29 UTC) #6
stgao
https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/build_failure_pipeline.py File appengine/findit/waterfall/build_failure_pipeline.py (right): https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/build_failure_pipeline.py#newcode24 appengine/findit/waterfall/build_failure_pipeline.py:24: def LogUnexpectedAborting(self, was_aborted): On 2015/03/31 21:51:05, chanli wrote: > ...
5 years, 8 months ago (2015-03-31 23:07:33 UTC) #7
chanli
https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/test/build_failure_pipeline_test.py File appengine/findit/waterfall/test/build_failure_pipeline_test.py (right): https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/test/build_failure_pipeline_test.py#newcode92 appengine/findit/waterfall/test/build_failure_pipeline_test.py:92: def testBuildFailurePipeline(self): On 2015/03/31 23:07:33, Shuotao wrote: > On ...
5 years, 8 months ago (2015-04-01 17:45:18 UTC) #8
stgao
https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/test/build_failure_pipeline_test.py File appengine/findit/waterfall/test/build_failure_pipeline_test.py (right): https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/test/build_failure_pipeline_test.py#newcode92 appengine/findit/waterfall/test/build_failure_pipeline_test.py:92: def testBuildFailurePipeline(self): On 2015/04/01 17:45:18, chanli wrote: > On ...
5 years, 8 months ago (2015-04-01 18:26:43 UTC) #9
chanli
On 2015/04/01 18:26:43, Shuotao wrote: > https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/test/build_failure_pipeline_test.py > File appengine/findit/waterfall/test/build_failure_pipeline_test.py (right): > > https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/test/build_failure_pipeline_test.py#newcode92 > ...
5 years, 8 months ago (2015-04-01 23:42:23 UTC) #10
qyearsley
On 2015/04/01 23:42:23, chanli wrote: > On 2015/04/01 18:26:43, Shuotao wrote: > > > https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/test/build_failure_pipeline_test.py ...
5 years, 8 months ago (2015-04-02 04:20:26 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1044263002/20001
5 years, 8 months ago (2015-04-02 17:18:29 UTC) #13
commit-bot: I haz the power
Try jobs failed on following builders: infra_tester on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/infra_tester/builds/1433)
5 years, 8 months ago (2015-04-02 17:47:35 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1044263002/20001
5 years, 8 months ago (2015-04-02 17:50:07 UTC) #17
commit-bot: I haz the power
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/infra/infra/+/a2075cdd3a3421b2b5c34482b7104fe559e533f6
5 years, 8 months ago (2015-04-02 17:57:25 UTC) #18
stgao
Sorry for my late review. I posted some more comments. Most of them are about ...
5 years, 8 months ago (2015-04-02 18:26:35 UTC) #19
chromium-reviews
Got it. I'll get back to this later^_^ On Thu, Apr 2, 2015 at 11:26 ...
5 years, 8 months ago (2015-04-02 18:27:31 UTC) #20
chanli
https://codereview.chromium.org/1044263002/diff/20001/appengine/findit/waterfall/build_failure_analysis_pipelines.py File appengine/findit/waterfall/build_failure_analysis_pipelines.py (right): https://codereview.chromium.org/1044263002/diff/20001/appengine/findit/waterfall/build_failure_analysis_pipelines.py#newcode12 appengine/findit/waterfall/build_failure_analysis_pipelines.py:12: from waterfall import analyze_build_failure_pipeline On 2015/04/02 18:26:35, Shuotao wrote: ...
5 years, 8 months ago (2015-04-03 00:52:21 UTC) #21
stgao
https://codereview.chromium.org/1044263002/diff/20001/appengine/findit/waterfall/test/analyze_build_failure_pipeline_test.py File appengine/findit/waterfall/test/analyze_build_failure_pipeline_test.py (right): https://codereview.chromium.org/1044263002/diff/20001/appengine/findit/waterfall/test/analyze_build_failure_pipeline_test.py#newcode162 appengine/findit/waterfall/test/analyze_build_failure_pipeline_test.py:162: self.assertIsNone(analysis) On 2015/04/03 00:52:21, chanli wrote: > On 2015/04/02 ...
5 years, 8 months ago (2015-04-03 17:22:27 UTC) #22
chanli
5 years, 8 months ago (2015-04-03 17:24:29 UTC) #23
Message was sent while issue was closed.
https://codereview.chromium.org/1044263002/diff/20001/appengine/findit/waterf...
File appengine/findit/waterfall/test/analyze_build_failure_pipeline_test.py
(right):

https://codereview.chromium.org/1044263002/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/test/analyze_build_failure_pipeline_test.py:162:
self.assertIsNone(analysis)
On 2015/04/03 17:22:27, Shuotao wrote:
> On 2015/04/03 00:52:21, chanli wrote:
> > On 2015/04/02 18:26:35, Shuotao wrote:
> > > How about adding another case: pipeline is not aborted?
> > 
> > I think the first test case: testBuildFailurePipeline is the one you are
> looking
> > for.
> 
> Yes, I think that one covered this case implicitly, otherwise test on code
> coverage won't pass. But it is more like an end-to-end test.
> 
> The test I suggested is more like a unittest for the function
> _LogUnexpectedAborting itself.

Got it.

Powered by Google App Engine
This is Rietveld 408576698