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.
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
