Code review comment for lp:~wallyworld/launchpad/rename-private-team-795771

Revision history for this message
Curtis Hovey (sinzui) wrote :

I am very wary of converting stories into unittests. Most are predicated on issues
that are relevant. A story only needs to show the happy path that demonstrates
that the user can perform a goal. In this case I believe only one browser test
is needs to show that a user can navigate from the team page to the edit page
and submit a change to be returned to the team page.

Do not use TestBrowser (an integration test) to test what a view
does (unittest). Browser tests do not document form contracts or directly
exercise the lines in the view it claims to be testing.

test_team_name_already_used is not testing the view's error message.
It is testing that base-layout-macros, which I am pretty sure is not telling
us anything more than the 100 other tests like this. I image a test that
realley exercises that the view does soemthing like this:

    def test_team_name_already_used(self):
        # If we try to use a name which is already in use, we'll get an error
        # message explaining it.

        self.factory.makeTeam(name="existing")
        owner = self.factory.makePerson()
        team = self.factory.makeTeam(name="team", owner=owner)

        form = {
            'field.name': 'existing'
            # May need more field to document that actual contract the
            # for requires.
            'field.actions.save': 'Save changes',
            }
        login_person(owner)
        view = create_initialized_view(team, '+edit' form=form)
        self.assertEqual(1, len(view.errors))
        notifications = view.request.response.notifications
        self.assertEqual(1, len(notifications))
        self.assertEqual(
            'existing is already in use by another person or team.',
            notifications[0].message)

The page rendering has nothing to do with permission. This is a zope
secuirty rule on a view. Never test with "mark" you do not know why
permission is given. Test that the team owner has permission and other
users do not. Also this ancient test is unaware that registry-experts
is the celebrity that actually does most of the administration today.

from canonical.launchpad.webapp.authorization import check_permission

    def test_edit_team_view_permission(self):
        # Only an administrator (as well as the team owner) of a team can
        # change the details of that team.
        person = self.factory.makePerson()
        team = self.factory.makeTeam()
        view = create_view(team, '+edit')
        login_person(person)
        self.assertFalse(check_permission('launchpad.Edit', view))
        login.person(team.owner)
        self.assertTrue(check_permission('launchpad.Edit', view))

review: Needs Fixing (test)

« Back to merge proposal