|
|
DescriptionPull 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
Messages
Total messages: 23 (4 generated)
chanli@chromium.org changed reviewers: + qyearsley@chromium.org, stgao@chromium.org
Hi Shuotao and Quinten, Could you review this cl when you have time? This cl is about refactoring the build_failure_analysis_pipelines.py and pull the BuildFailurePipeline to a separate file. Thank you.
https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/build_failure_pipeline.py (right): https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/build_failure_pipeline.py:15: class BuildFailurePipeline(BasePipeline): nit: to be consistent with other pipelines, we may rename it to AnalyzeBuildFailurePipeline and also rename the file name. https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/build_failure_pipeline.py:24: def LogUnexpectedAborting(self, was_aborted): style: Since this function is used within this class only, we could prefix it with a underscore like "_LogUnexpectedAborting" to make it private. https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/build_failure_pipeline.py:31: was_aborted (bool): Takes in the value of self.was_aborted. nit: no need to mention self.was_aborted, because it is more like implementation detail. https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/build_failure_pipeline.py:34: if was_aborted: # pragma: no cover As we have unittest for this, we could remove the "no cover" comments. https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/test/build_failure_pipeline_test.py (right): https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/test/build_failure_pipeline_test.py:15: style: no tailing empty space. Same for other lines below in this file. https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/test/build_failure_pipeline_test.py:92: def testBuildFailurePipeline(self): If we test the whole pipeline here, in appengine/findit/waterfall/test/build_failure_analysis_pipelines_test.py, we don't have to run the pipeline again. That said, we could mock the root pipeline there. https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/test/build_failure_pipeline_test.py:134: def testAbortBuildFailurePipeline(self): If the "no cover" was removed, more unittests might be needed for that function. https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/test/build_failure_pipeline_test.py:144: root_pipeline.start(queue_name='default') Since it is a unittest for a specific function, We don't need to start the pipeline task.
https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/build_failure_pipeline.py (right): https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/build_failure_pipeline.py:24: def LogUnexpectedAborting(self, was_aborted): On 2015/03/31 21:43:24, Shuotao wrote: > style: Since this function is used within this class only, we could prefix it > with a underscore like "_LogUnexpectedAborting" to make it private. If we make it private, is there a way we can call it in unittest directly without actually abort the pipeline? https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/build_failure_pipeline.py:31: was_aborted (bool): Takes in the value of self.was_aborted. On 2015/03/31 21:43:24, Shuotao wrote: > nit: no need to mention self.was_aborted, because it is more like implementation > detail. Should I rephrase the description or I can just remove the Args for this method? https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/test/build_failure_pipeline_test.py (right): https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/test/build_failure_pipeline_test.py:92: def testBuildFailurePipeline(self): On 2015/03/31 21:43:26, Shuotao wrote: > If we test the whole pipeline here, in > appengine/findit/waterfall/test/build_failure_analysis_pipelines_test.py, we > don't have to run the pipeline again. That said, we could mock the root pipeline > there. Do you mean we don't need this test case at all? Do we just need to test the case when the root pipeline is aborted?
https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/build_failure_pipeline.py (right): https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/build_failure_pipeline.py:28: analysis as ERROR. The analysis is created before the pipeline starts. In general, docstrings should start with a maximum one-line summary to quickly give the reader a better idea of what the function does, followed by more details below. e.g. """Marks the WfAnalysis status as error, indicating that it was aborted. [DETAILS] """ https://www.python.org/dev/peps/pep-0257/#id17 https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/build_failure_pipeline.py:31: was_aborted (bool): Takes in the value of self.was_aborted. Personally, I usually would like to have descriptions of every argument, although the description "Takes in the value of self.was_aborted" doesn't really tell me what it means, or what its purpose is. That said, in this case you might want to pull out the check for the value of self.was_aborted outside of this function. https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/build_failure_pipeline.py:33: This blank line could be removed https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/build_failure_pipeline.py:42: self.LogUnexpectedAborting(self.was_aborted) How about: def finalized(self): if self.was_aborted: self.LogUnexpectedAborting() Then LogUnexpectedAborting doesn't have to take any args.
https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/build_failure_pipeline.py (right): https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/build_failure_pipeline.py:42: self.LogUnexpectedAborting(self.was_aborted) On 2015/03/31 22:00:58, qyearsley wrote: > How about: > > def finalized(self): > if self.was_aborted: > self.LogUnexpectedAborting() > > Then LogUnexpectedAborting doesn't have to take any args. The reason why I pass self.was_aborted as a parameter is that I need to test this method in unittest without actually abort the pipeline. Any idea about how make it more reasonable and testable?
https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/build_failure_pipeline.py (right): https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/build_failure_pipeline.py:24: def LogUnexpectedAborting(self, was_aborted): On 2015/03/31 21:51:05, chanli wrote: > On 2015/03/31 21:43:24, Shuotao wrote: > > style: Since this function is used within this class only, we could prefix it > > with a underscore like "_LogUnexpectedAborting" to make it private. > > If we make it private, is there a way we can call it in unittest directly > without actually abort the pipeline? Yes, we should be able to access it in unittest, although in functional code we should avoid that (I guess pylint will complain in the later case). https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/build_failure_pipeline.py:31: was_aborted (bool): Takes in the value of self.was_aborted. On 2015/03/31 21:51:05, chanli wrote: > On 2015/03/31 21:43:24, Shuotao wrote: > > nit: no need to mention self.was_aborted, because it is more like > implementation > > detail. > > Should I rephrase the description or I can just remove the Args for this method? Probably we could just rephrase the description to something like "True if the pipeline was aborted, otherwise False". https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/build_failure_pipeline.py:42: self.LogUnexpectedAborting(self.was_aborted) On 2015/03/31 22:19:28, chanli wrote: > On 2015/03/31 22:00:58, qyearsley wrote: > > How about: > > > > def finalized(self): > > if self.was_aborted: > > self.LogUnexpectedAborting() > > > > Then LogUnexpectedAborting doesn't have to take any args. > > The reason why I pass self.was_aborted as a parameter is that I need to test > this method in unittest without actually abort the pipeline. > > Any idea about how make it more reasonable and testable? For testing purpose, we might have to pass it over into the function. For the alternative Quinten suggested, you may have to add a "no cover" like the original code. That should be fine too. https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/test/build_failure_pipeline_test.py (right): https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/test/build_failure_pipeline_test.py:92: def testBuildFailurePipeline(self): On 2015/03/31 21:51:05, chanli wrote: > On 2015/03/31 21:43:26, Shuotao wrote: > > If we test the whole pipeline here, in > > appengine/findit/waterfall/test/build_failure_analysis_pipelines_test.py, we > > don't have to run the pipeline again. That said, we could mock the root > pipeline > > there. > > Do you mean we don't need this test case at all? Do we just need to test the > case when the root pipeline is aborted? Sorry for the confusion. Here, it is good to have an end-to-end test for a positive case (no error occurs and it yields the correct result we want). But this new testcase will make the existing one in appengine/findit/waterfall/test/build_failure_analysis_pipelines_test.py redundant. (This was my concern.) From the prospective of unittest, in build_failure_analysis_pipelines_test.py, we could mock out the root pipeline class "[Analyze]BuildFailurePipeline" to avoid start a real pipeline task.
https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/test/build_failure_pipeline_test.py (right): https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/test/build_failure_pipeline_test.py:92: def testBuildFailurePipeline(self): On 2015/03/31 23:07:33, Shuotao wrote: > On 2015/03/31 21:51:05, chanli wrote: > > On 2015/03/31 21:43:26, Shuotao wrote: > > > If we test the whole pipeline here, in > > > appengine/findit/waterfall/test/build_failure_analysis_pipelines_test.py, we > > > don't have to run the pipeline again. That said, we could mock the root > > pipeline > > > there. > > > > Do you mean we don't need this test case at all? Do we just need to test the > > case when the root pipeline is aborted? > > Sorry for the confusion. > > Here, it is good to have an end-to-end test for a positive case (no error occurs > and it yields the correct result we want). > > But this new testcase will make the existing one in > appengine/findit/waterfall/test/build_failure_analysis_pipelines_test.py > redundant. (This was my concern.) > > From the prospective of unittest, in build_failure_analysis_pipelines_test.py, > we could mock out the root pipeline class "[Analyze]BuildFailurePipeline" to > avoid start a real pipeline task. For the test case in appengine/findit/waterfall/test/build_failure_analysis_pipelines_test.py, since the pipeline is created in the ScheduleAnalysisIfNeeded function, I'm not sure how we can mock the pipeline in unittest. And actually, it's pretty much the same as the test case here. Maybe we should just delete the test case in build_failure_analysis_pipelines_test and mark the ScheduleAnalysisIfNeeded as "no cover"?
https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/test/build_failure_pipeline_test.py (right): https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/test/build_failure_pipeline_test.py:92: def testBuildFailurePipeline(self): On 2015/04/01 17:45:18, chanli wrote: > On 2015/03/31 23:07:33, Shuotao wrote: > > On 2015/03/31 21:51:05, chanli wrote: > > > On 2015/03/31 21:43:26, Shuotao wrote: > > > > If we test the whole pipeline here, in > > > > appengine/findit/waterfall/test/build_failure_analysis_pipelines_test.py, > we > > > > don't have to run the pipeline again. That said, we could mock the root > > > pipeline > > > > there. > > > > > > Do you mean we don't need this test case at all? Do we just need to test the > > > case when the root pipeline is aborted? > > > > Sorry for the confusion. > > > > Here, it is good to have an end-to-end test for a positive case (no error > occurs > > and it yields the correct result we want). > > > > But this new testcase will make the existing one in > > appengine/findit/waterfall/test/build_failure_analysis_pipelines_test.py > > redundant. (This was my concern.) > > > > From the prospective of unittest, in build_failure_analysis_pipelines_test.py, > > we could mock out the root pipeline class "[Analyze]BuildFailurePipeline" to > > avoid start a real pipeline task. > > For the test case in > appengine/findit/waterfall/test/build_failure_analysis_pipelines_test.py, since > the pipeline is created in the ScheduleAnalysisIfNeeded function, I'm not sure > how we can mock the pipeline in unittest. And actually, it's pretty much the > same as the test case here. Maybe we should just delete the test case in > build_failure_analysis_pipelines_test and mark the ScheduleAnalysisIfNeeded as > "no cover"? As discussed offline, we could mock the root pipeline like what we did for lock_util.WaitUntilDownloadAllowed above in line #66-69. And we need to verify the case: when an analysis is needed, a pipeline task is indeed started for the analysis. It is good to have a test for the opposite case too.
On 2015/04/01 18:26:43, Shuotao wrote: > https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/... > File appengine/findit/waterfall/test/build_failure_pipeline_test.py (right): > > https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/... > appengine/findit/waterfall/test/build_failure_pipeline_test.py:92: def > testBuildFailurePipeline(self): > On 2015/04/01 17:45:18, chanli wrote: > > On 2015/03/31 23:07:33, Shuotao wrote: > > > On 2015/03/31 21:51:05, chanli wrote: > > > > On 2015/03/31 21:43:26, Shuotao wrote: > > > > > If we test the whole pipeline here, in > > > > > > appengine/findit/waterfall/test/build_failure_analysis_pipelines_test.py, > > we > > > > > don't have to run the pipeline again. That said, we could mock the root > > > > pipeline > > > > > there. > > > > > > > > Do you mean we don't need this test case at all? Do we just need to test > the > > > > case when the root pipeline is aborted? > > > > > > Sorry for the confusion. > > > > > > Here, it is good to have an end-to-end test for a positive case (no error > > occurs > > > and it yields the correct result we want). > > > > > > But this new testcase will make the existing one in > > > appengine/findit/waterfall/test/build_failure_analysis_pipelines_test.py > > > redundant. (This was my concern.) > > > > > > From the prospective of unittest, in > build_failure_analysis_pipelines_test.py, > > > we could mock out the root pipeline class "[Analyze]BuildFailurePipeline" to > > > avoid start a real pipeline task. > > > > For the test case in > > appengine/findit/waterfall/test/build_failure_analysis_pipelines_test.py, > since > > the pipeline is created in the ScheduleAnalysisIfNeeded function, I'm not sure > > how we can mock the pipeline in unittest. And actually, it's pretty much the > > same as the test case here. Maybe we should just delete the test case in > > build_failure_analysis_pipelines_test and mark the ScheduleAnalysisIfNeeded as > > "no cover"? > > As discussed offline, we could mock the root pipeline like what we did for > lock_util.WaitUntilDownloadAllowed above in line #66-69. > > And we need to verify the case: when an analysis is needed, a pipeline task is > indeed started for the analysis. > It is good to have a test for the opposite case too. Hi Shuotao and Quinten, I have made the changes as we discussed, could you review it again when you have time? Thank you.
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/... > > File appengine/findit/waterfall/test/build_failure_pipeline_test.py (right): > > > > > https://codereview.chromium.org/1044263002/diff/1/appengine/findit/waterfall/... > > appengine/findit/waterfall/test/build_failure_pipeline_test.py:92: def > > testBuildFailurePipeline(self): > > On 2015/04/01 17:45:18, chanli wrote: > > > On 2015/03/31 23:07:33, Shuotao wrote: > > > > On 2015/03/31 21:51:05, chanli wrote: > > > > > On 2015/03/31 21:43:26, Shuotao wrote: > > > > > > If we test the whole pipeline here, in > > > > > > > > appengine/findit/waterfall/test/build_failure_analysis_pipelines_test.py, > > > we > > > > > > don't have to run the pipeline again. That said, we could mock the > root > > > > > pipeline > > > > > > there. > > > > > > > > > > Do you mean we don't need this test case at all? Do we just need to test > > the > > > > > case when the root pipeline is aborted? > > > > > > > > Sorry for the confusion. > > > > > > > > Here, it is good to have an end-to-end test for a positive case (no error > > > occurs > > > > and it yields the correct result we want). > > > > > > > > But this new testcase will make the existing one in > > > > appengine/findit/waterfall/test/build_failure_analysis_pipelines_test.py > > > > redundant. (This was my concern.) > > > > > > > > From the prospective of unittest, in > > build_failure_analysis_pipelines_test.py, > > > > we could mock out the root pipeline class "[Analyze]BuildFailurePipeline" > to > > > > avoid start a real pipeline task. > > > > > > For the test case in > > > appengine/findit/waterfall/test/build_failure_analysis_pipelines_test.py, > > since > > > the pipeline is created in the ScheduleAnalysisIfNeeded function, I'm not > sure > > > how we can mock the pipeline in unittest. And actually, it's pretty much the > > > same as the test case here. Maybe we should just delete the test case in > > > build_failure_analysis_pipelines_test and mark the ScheduleAnalysisIfNeeded > as > > > "no cover"? > > > > As discussed offline, we could mock the root pipeline like what we did for > > lock_util.WaitUntilDownloadAllowed above in line #66-69. > > > > And we need to verify the case: when an analysis is needed, a pipeline task is > > indeed started for the analysis. > > It is good to have a test for the opposite case too. > > Hi Shuotao and Quinten, > > I have made the changes as we discussed, could you review it again when you have > time? > > Thank you. LGTM :-)
The CQ bit was checked by chanli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1044263002/20001
The CQ bit was unchecked by commit-bot@chromium.org
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/bu...)
The CQ bit was checked by chanli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1044263002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/infra/infra/+/a2075cdd3a3421b2b5c34482b7104...
Message was sent while issue was closed.
Sorry for my late review. I posted some more comments. Most of them are about coding style, while others are on the testcases. Would you mind addressing my comments in another CL? https://codereview.chromium.org/1044263002/diff/20001/appengine/findit/waterf... File appengine/findit/waterfall/analyze_build_failure_pipeline.py (right): https://codereview.chromium.org/1044263002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/analyze_build_failure_pipeline.py:15: class AnalyzeBuildFailurePipeline(BasePipeline): style nit: top-level classes/functions/constants should be separated by two empty lines. https://codereview.chromium.org/1044263002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/analyze_build_failure_pipeline.py:60: nit: remove two tailing empty lines? https://codereview.chromium.org/1044263002/diff/20001/appengine/findit/waterf... File appengine/findit/waterfall/build_failure_analysis_pipelines.py (right): https://codereview.chromium.org/1044263002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/build_failure_analysis_pipelines.py:12: from waterfall import analyze_build_failure_pipeline nit: to be consistent with other imports, may use below instead. from waterfall.analyze_build_failure_pipeline import AnalyzeBuildFailurePipeline 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:16: style nit: no tailing whitespace. Same for other occurrences below in this file. https://codereview.chromium.org/1044263002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/test/analyze_build_failure_pipeline_test.py:162: self.assertIsNone(analysis) How about adding another case: pipeline is not aborted? https://codereview.chromium.org/1044263002/diff/20001/appengine/findit/waterf... File appengine/findit/waterfall/test/build_failure_analysis_pipelines_test.py (right): https://codereview.chromium.org/1044263002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/test/build_failure_analysis_pipelines_test.py:15: style nit: top-level classes should be separated by two empty lines. http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Blank_Lines https://codereview.chromium.org/1044263002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/test/build_failure_analysis_pipelines_test.py:16: class MockRootPipeline(): nit: make it private (_MockRootPipeline) since it won't be used by other module. https://codereview.chromium.org/1044263002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/test/build_failure_analysis_pipelines_test.py:93: self.assertIsNotNone(analysis) It seems we didn't verify that the pipeline was started. One way to implement this: In class MockRootPipeline, add a class-level var: class MockRootPipeline(object): STARTED = False In the MockRootPipeline.start, update STARTED to True. We could also add another case that a pipeline should not be started when a new analysis is not needed.
Message was sent while issue was closed.
Got it. I'll get back to this later^_^ On Thu, Apr 2, 2015 at 11:26 AM, <stgao@chromium.org> wrote: > Sorry for my late review. > > I posted some more comments. > Most of them are about coding style, while others are on the testcases. > > Would you mind addressing my comments in another CL? > > > https://codereview.chromium.org/1044263002/diff/20001/ > appengine/findit/waterfall/analyze_build_failure_pipeline.py > File appengine/findit/waterfall/analyze_build_failure_pipeline.py > (right): > > https://codereview.chromium.org/1044263002/diff/20001/ > appengine/findit/waterfall/analyze_build_failure_pipeline.py#newcode15 > appengine/findit/waterfall/analyze_build_failure_pipeline.py:15: class > AnalyzeBuildFailurePipeline(BasePipeline): > style nit: top-level classes/functions/constants should be separated by > two empty lines. > > https://codereview.chromium.org/1044263002/diff/20001/ > appengine/findit/waterfall/analyze_build_failure_pipeline.py#newcode60 > appengine/findit/waterfall/analyze_build_failure_pipeline.py:60: > nit: remove two tailing empty lines? > > 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 > nit: to be consistent with other imports, may use below instead. > from waterfall.analyze_build_failure_pipeline import > AnalyzeBuildFailurePipeline > > 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#newcode16 > appengine/findit/waterfall/test/analyze_build_failure_pipeline_test.py:16: > > style nit: no tailing whitespace. Same for other occurrences below in > this file. > > 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) > How about adding another case: pipeline is not aborted? > > https://codereview.chromium.org/1044263002/diff/20001/ > appengine/findit/waterfall/test/build_failure_analysis_pipelines_test.py > File > appengine/findit/waterfall/test/build_failure_analysis_pipelines_test.py > (right): > > https://codereview.chromium.org/1044263002/diff/20001/ > appengine/findit/waterfall/test/build_failure_analysis_ > pipelines_test.py#newcode15 > appengine/findit/waterfall/test/build_failure_analysis_ > pipelines_test.py:15: > > style nit: top-level classes should be separated by two empty lines. > > http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Blank_Lines > > https://codereview.chromium.org/1044263002/diff/20001/ > appengine/findit/waterfall/test/build_failure_analysis_ > pipelines_test.py#newcode16 > appengine/findit/waterfall/test/build_failure_analysis_ > pipelines_test.py:16: > class MockRootPipeline(): > nit: make it private (_MockRootPipeline) since it won't be used by other > module. > > https://codereview.chromium.org/1044263002/diff/20001/ > appengine/findit/waterfall/test/build_failure_analysis_ > pipelines_test.py#newcode93 > appengine/findit/waterfall/test/build_failure_analysis_ > pipelines_test.py:93: > self.assertIsNotNone(analysis) > It seems we didn't verify that the pipeline was started. One way to > implement this: > In class MockRootPipeline, add a class-level var: > class MockRootPipeline(object): > STARTED = False > > In the MockRootPipeline.start, update STARTED to True. > > We could also add another case that a pipeline should not be started > when a new analysis is not needed. > > https://codereview.chromium.org/1044263002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
https://codereview.chromium.org/1044263002/diff/20001/appengine/findit/waterf... File appengine/findit/waterfall/build_failure_analysis_pipelines.py (right): https://codereview.chromium.org/1044263002/diff/20001/appengine/findit/waterf... 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: > nit: to be consistent with other imports, may use below instead. > from waterfall.analyze_build_failure_pipeline import AnalyzeBuildFailurePipeline I used this import because in unittest I need to mock the class. 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/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.
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 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.
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. |