launchpad-reviewers team mailing list archive

[Gavin Panella <gavin.panella@xxxxxxxxxxxxx>]

> [9]
>
> 580     +        self.assertEqual(
> 581     +            (legacy_user, legacy_user, legacy_user, legacy_user),
>
> "(legacy_user, legacy_user, legacy_user, legacy_user)" => "[legacy_user] * 4"
> ?
>
> I think it's more readable but this is really a detail.

We'll agree to differ on this one :)

> [10]
>
> 558     +        self.assertEqual(user1, reload_object(node1).owner)
> 559     +        self.assertEqual(user1, reload_object(node2).owner)
>
> Probably better to make that one check.

Done.

>
> [11]
>
> 472     +class TestMigrateToUser(TestCase):
> 473     +
> 474     +    def test_migrate(self):
>
> That test is a bit… huge :) .  Don't you think it could test for behavior
> instead of patching methods… and maybe share some code with
> TestMigrate.test_migrate_ancillary_data_to_legacy_user_when_multiple_users ?

The behaviour of the individual components is tested elsewhere, and
the overall behaviour is tested in TestMigrate, so I sought only to
show that things were wired up correctly. In a way it's also a
reminder not to put more logic into migrate_to_user(); it should be
factored into separate functions.

It was a bit of a what-if experiment too. I've added comments to the
function to explain its rationale, but I'm open to more discussion.
It's interesting to consider the value of this kind of test; perhaps
not very much.

-- 
https://code.launchpad.net/~allenap/maas/shared-to-per-tenant-storage/+merge/151858
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/maas/shared-to-per-tenant-storage into lp:maas.