launchpad-reviewers team mailing list archive

[Raphaël Badin <raphael.badin@xxxxxxxxxxxxx>]

Review: Approve

Great work!  As Francesco said, congrats for turning the doc tests into proper unit tests.

[0]

551	+        for sourcename in (
552	+            "gedit",
553	+            "firefox",
554	+            "cobblers",
555	+            "thunderpants",
556	+            "apg",
557	+            "vim",
558	+            "gcc",
559	+            "bison",
560	+            "flex",
561	+            "postgres",
562	+            ):

You've made this code much more compact and readable but maybe you can go one step further and avoid having the list defined in the loop statement:  please consider using this construct:
sourcenames = [
    "gedit",
    "firefox",
    [...]
    "cobblers",
    "thunderpants"
    ]
for sourcename in sourcenames:
    [...]

[1]

614	+        # 1500 (RELEASE) + 1000 (main) + 5 (low) = 2505.
615	+        job = self.makeBuildJob(component="main", urgency="low")
616	+        self.assertEqual(2505, job.score())

Maybe you could use:
self.assertEqual(
    SCORE_BY_POCKET[PackagePublishingPocket.RELEASE] + SCORE_BY_COMPONENT['main'] +   
        SCORE_BY_URGENCY[SourcePackageUrgency.LOW],
    job.score())
instead of putting the integer value (2505).  This would help document the code in the other tests similar to this one where no comment is present and it also would avoid hardcoding the integer values in the tests.  I know these values are probably not gonna change soon but I think it's still a healthy way to write more explicit tests

[2]

I think you're missing a test to make sure that 'score' can be read through the api.  Strickly speaking, it's tested in test_score_packageset_allows_buildd_admin but please consider adding a simple test for that.  My idea is that if by any chance someone removes that field on the API, the name of the failing tests should make the problem obvious to solve.  Right now, the failure will sort of imply that something is wrong with the permissions.
-- 
https://code.launchpad.net/~cjwatson/launchpad/packageset-score/+merge/105915
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.