curtin-dev team mailing list archive

[Ryan Harper <rharper@xxxxxxxxxxx>]

Review: Needs Fixing

Can you drop the missing Bionic/Focal ones and add them in a separate PR?

I added a few spots where I asked if we should move a unittest into a base class and iterate skip any releases that don't support it so we don't have to duplicate the unittest in each release class.

There are a few places I didn't mention that; some of them are class settings; like disk_block_size = 4096, I think those are fine to duplicate though if we could make a 4k base class and subclass the users of that; it'd be a nice cleanup.


Diff comments:

> diff --git a/tests/vmtests/test_lvm_raid.py b/tests/vmtests/test_lvm_raid.py
> index cc1afa1..5fe7993 100644
> --- a/tests/vmtests/test_lvm_raid.py
> +++ b/tests/vmtests/test_lvm_raid.py
> @@ -47,7 +47,7 @@ class TestLvmOverRaidAbs(TestMdadmAbs, TestLvmAbs):
>          return self._test_pvs(dname_to_vg)
>  
>  
> -class FocalTestLvmOverRaid(relbase.focal, TestLvmOverRaidAbs):
> +class XenialGATestLvmOverRaid(relbase.xenial_ga, TestLvmOverRaidAbs):

This is a strange re-ordering; was this manual?

>      __test__ = True
>  
>  
> diff --git a/tests/vmtests/test_mdadm_bcache.py b/tests/vmtests/test_mdadm_bcache.py
> index 4f38e7a..ac8af3f 100644
> --- a/tests/vmtests/test_mdadm_bcache.py
> +++ b/tests/vmtests/test_mdadm_bcache.py
> @@ -333,6 +346,16 @@ class FocalTestMirrorbootPartitionsUEFI(relbase.focal,
>          self.assertEqual(primary_esp, backup_esp)
>  
>  
> +class GroovyTestMirrorbootPartitionsUEFI(relbase.groovy,
> +                                         TestMirrorbootPartitionsUEFIAbs):
> +    __test__ = True
> +
> +    def test_backup_esp_matches_primary(self):
> +        primary_esp = self.load_collect_file("diska-part1-efi.out")
> +        backup_esp = self.load_collect_file("diskb-part1-efi.out")
> +        self.assertEqual(primary_esp, backup_esp)

I wonder if we should move this to the TestMirrorbootPartitionsUEFIAbs class
and have it SkipTest if not release in ['focal', 'groovy'];

I much prefer to have a single unittest than a copy-paste from 
release to release.

> +
> +
>  class TestRaid5bootAbs(TestMdadmAbs):
>      # alternative config for more complex setup
>      conf_file = "examples/tests/raid5boot.yaml"
> diff --git a/tests/vmtests/test_network_mtu.py b/tests/vmtests/test_network_mtu.py
> index 71f87ca..4bed78a 100644
> --- a/tests/vmtests/test_network_mtu.py
> +++ b/tests/vmtests/test_network_mtu.py
> @@ -191,6 +191,11 @@ class FocalTestNetworkMtu(relbase.focal, TestNetworkMtuAbs):
>      __test__ = True
>  
>  
> +class GroovyTestNetworkMtu(relbase.groovy, TestNetworkMtuAbs):
> +    conf_file = "examples/tests/network_mtu_networkd.yaml"

Hrm, do we not have a class for just network_mtu_networkd that multiple release classes can subclass from?

> +    __test__ = True
> +
> +
>  class Centos66TestNetworkMtu(centos_relbase.centos66_xenial,
>                               CentosTestNetworkMtuAbs):
>      __test__ = True


-- 
https://code.launchpad.net/~paride/curtin/+git/curtin/+merge/387865
Your team curtin developers is requested to review the proposed merge of ~paride/curtin:vmtests-add-groovy into curtin:master.