Code review comment for lp://staging/~stevenk/launchpad/dsd-hide-child-diff

Revision history for this message
Gavin Panella (allenap) wrote :

Nice :)

[1]

+ tags = soup.findAll('span')
+ self.assertEqual(2, len(tags))

This is a bit fragile because it's so non-specific.

Consider changing it to:

        tags = soup.find('ul', 'package-diff-status').findAll('span')

[2]

There are no unit tests for the view's display_child_diff
property. It's pretty simple and I think it's okay how you've done
it. I'm just wondering whether or not it would be better to test
display_child_diff in isolation (and the template in isolation, by
patching the view for example), and then having these integration
tests on top. That's more typing, would probably be more "pure", but I
wonder if it would buy us anything? I'm thinking out loud, I'm not
asking you to make any changes, just interested if you have an
opinion.

review: Approve

« Back to merge proposal