Merge into devel : bug-594247-unittests-for-searchtasks-4 : Code : Launchpad itself

Status:

Merged

Approved by:

Gavin Panella on 2010-11-04

Approved revision:

no longer in the source branch.

Merged at revision:

11864

Proposed branch:

lp://staging/~adeuring/launchpad/bug-594247-unittests-for-searchtasks-4

Merge into:

lp://staging/launchpad

Diff against target:

733 lines (+302/-115)

4 files modified

lib/lp/bugs/interfaces/bugtask.py (+2/-2)
lib/lp/bugs/tests/test_bugtask_search.py (+267/-113)
lib/lp/testing/factory.py (+11/-0)
lib/lp/testing/tests/test_factory.py (+22/-0)

To merge this branch:

bzr merge lp://staging/~adeuring/launchpad/bug-594247-unittests-for-searchtasks-4

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Gavin Panella (community)		2010-11-03	Approve on 2010-11-04
Review via email: mp+39991@code.staging.launchpad.net

Commit message

unit tests of BugtaskSet.search() and BugTaskSet.searchBugIds(): new method SearchTestBase.assertSearchFinds(); more tests; more robust lookup of a bug target that is not the main target of the tests.

Description of the change

new attempt, hopefully without merge conflict and an overly huge diff...

This branch adds more unit tests for BugTaskSet.search() and for
BugTaskSet.searchBugIds(), leaving only a few parameters of
BugTaskSearchParams not covered.

Aside for these additional tests, I added a new method
assertSearchFinds() (suggested by Gavin in a previous review
of these tests), which makes reading the tests slightly less
boring and a bit more readable. Working on this change, I
noticed that one tests missed an assert...

Working on tests to find bugs being created or modified after a
given time, I noticed that it was possible to pass the parameters
created_since and modified_since to the constructor of
BugTaskSearchparams, but that the object properties created_since
and modified_since were always set to None. This is now fixed.

One test needed access to a product which is not the main
target of the current test; an already existing test modifies
the bug task of this "other target"
(changeStatusOfBugTaskForOtherProduct()). I moved the code to find
this "other bugtask" into a separate method
(findBugtaskForOtherProduct()). The implementation is less obsucre
ini comparison with the old implementation to find the "other
bugtask".

test: ./bin/test -vvt test_bugtask_search

no lint.

Revision history for this message

Gavin Panella (allenap) wrote on 2010-11-04:

#

Sorry for taking so long to review this. Reading unit tests can be
hard work :-/

A general comment is that the findBugtaskForOtherProduct and its
helper _findBugtaskForOtherProduct started to get a bit convoluted. I
don't have a suggestion for making it better though. I think I
wouldn't have separated it out into two methods; I would have just
overridden the method and called up, but that's a matter of
preference.

It's great to have some clear and accurate definitions of search
behaviour.

+1

[1]

+ # Return the bugtask for the product that not related to the
+ # main bug target.

s/that/that is/

[2]

+ def _findBugtaskForOtherProduct(self, bugtask, main_product):

To summarize this, just to check my understanding:

Return the first bugtask of the given bugtask's bug that is (a)
targeted to an IProduct and (b) not targeted to main_product.

[3]

+ # Search results can be limited to bugs with a bug target to which
+ # a given person has a structural subscription.

Oh my, I was not aware of this. This could get complicated with
filters. For now I'm going to ignore it :)

[4]

+ def changeStatusOfBugTaskForOtherProduct(self, bugtask, new_status):
+ # Change the status of another bugtask of the same bug to the
+ # given status.
...
+ bug = bugtask.bug
+ for other_task in bug.bugtasks:
+ other_target = other_task.target
+ if IProduct.providedBy(other_target):
+ with person_logged_in(other_target.owner):
+ other_task.transitionToStatus(
+ new_status, other_target.owner)

This will change the status of bugtask too, which might be fine but is
not what is implied by the method name and comment.

If you only wish to change the status of other tasks, the
related_tasks property could be useful:

    for other_task in bugtask.related_tasks:
        other_target = other_task.target
        if IProduct.providedBy(other_target):
            with person_logged_in(other_target.owner):
                other_task.transitionToStatus(
                    new_status, other_target.owner)

[5]

+ def makeCVE(self, sequence, description=None,
+ cvestate=CveStatus.CANDIDATE):

This probably ought to have a simple test or two. I only just noticed
that there are tests for the factory methods.

Sorry for taking so long to review this. Reading unit tests can be
hard work :-/

A general comment is that the findBugtaskForOtherProduct and its
helper _findBugtaskForOtherProduct started to get a bit convoluted. I
don't have a suggestion for making it better though. I think I
wouldn't have separated it out into two methods; I would have just
overridden the method and called up, but that's a matter of
preference.

It's great to have some clear and accurate definitions of search
behaviour.

+1

[1]

+        # Return the bugtask for the product that not related to the
+        # main bug target.

s/that/that is/

[2]

+    def _findBugtaskForOtherProduct(self, bugtask, main_product):

To summarize this, just to check my understanding:

Return the first bugtask of the given bugtask's bug that is (a)
  targeted to an IProduct and (b) not targeted to main_product.

[3]

+        # Search results can be limited to bugs with a bug target to which
+        # a given person has a structural subscription.

Oh my, I was not aware of this. This could get complicated with
filters. For now I'm going to ignore it :)

[4]

+    def changeStatusOfBugTaskForOtherProduct(self, bugtask, new_status):
+        # Change the status of another bugtask of the same bug to the
+        # given status.
...
+        bug = bugtask.bug
+        for other_task in bug.bugtasks:
+            other_target = other_task.target
+            if IProduct.providedBy(other_target):
+                with person_logged_in(other_target.owner):
+                    other_task.transitionToStatus(
+                        new_status, other_target.owner)

This will change the status of bugtask too, which might be fine but is
not what is implied by the method name and comment.

If you only wish to change the status of other tasks, the
related_tasks property could be useful:

for other_task in bugtask.related_tasks:
        other_target = other_task.target
        if IProduct.providedBy(other_target):
            with person_logged_in(other_target.owner):
                other_task.transitionToStatus(
                    new_status, other_target.owner)

[5]

+    def makeCVE(self, sequence, description=None,
+                cvestate=CveStatus.CANDIDATE):

This probably ought to have a simple test or two. I only just noticed
that there are tests for the factory methods.

review: Approve

Launchpad itself

Merge lp://staging/~adeuring/launchpad/bug-594247-unittests-for-searchtasks-4 into lp://staging/launchpad

Commit message

Description of the change

Preview Diff

Subscribers