curtin-dev team mailing list archive

[Michael Hudson-Doyle <mwhudsonlp@xxxxxxxxxxx>]

I have to say I find the test_apt_source tests a bit incomprehensible. Your changes don't really make them worse but I don't think they really make them better either :/ I have a couple of concrete suggestions in the comments, do they sound reasonable?

Diff comments:

> diff --git a/tests/unittests/test_apt_source.py b/tests/unittests/test_apt_source.py
> index 6556399..c89855f 100644
> --- a/tests/unittests/test_apt_source.py
> +++ b/tests/unittests/test_apt_source.py
> @@ -75,6 +90,9 @@ class TestAptSourceConfig(CiTestCase):
>          self.aptlistfile3 = os.path.join(self.tmp, "single-deb3.list")
>          self.join = os.path.join
>          self.matcher = re.compile(ADD_APT_REPO_MATCH).search
> +        # in case this is not obvious to anyone else, this causes GPG
> +        # armored keys to be 's390x', because the underlying gpg code
> +        # calls util.subp.

It seems like nothing depends on this mocking (any more?) so let's not make the tests awful because of it :)  Is it possible to change the tests so that it's the functions from the gpg module that are mocked?

>          self.add_patch('curtin.util.subp', 'm_subp')
>          self.m_subp.return_value = ('s390x', '')
>  
> @@ -221,17 +239,24 @@ class TestAptSourceConfig(CiTestCase):
>          """
>          params = self._get_default_params()
>  
> -        with mock.patch("curtin.util.subp",
> -                        return_value=('fakekey 1234', '')) as mockobj:
> -            self._add_apt_sources(cfg, TARGET, template_params=params,
> -                                  aa_repo_match=self.matcher)
> +        self._add_apt_sources(cfg, TARGET, template_params=params,
> +                              aa_repo_match=self.matcher)
>  
> -        # check if it added the right ammount of keys
> -        calls = []
> -        for _ in range(keynum):

This method (_apt_src_keyid) no longer uses the keynum argument, so drop it?

> -            calls.append(call(['apt-key', 'add', '-'], data=b'fakekey 1234',
> -                              target=TARGET))
> -        mockobj.assert_has_calls(calls, any_order=True)
> +        for ent in cfg:
> +            key_filename = (
> +                cfg[ent]['filename'] if 'filename' in cfg[ent] else ent
> +            )

key_filename = cfg[ent].get('filename', ent) is clearer, surely?

> +            # without knowing the key, do not know the suffix
> +            self.assertTrue(
> +                os.path.isfile(key_filename + '.asc') or
> +                os.path.isfile(key_filename + '.gpg')
> +            )
> +            if os.path.isfile(key_filename + '.asc'):
> +                contents = load_tfile(key_filename + '.asc')
> +                self.assertMultiLineEqual(EXPECTEDKEY_NOVER, contents)
> +            elif os.path.isfile(key_filename + '.gpg'):
> +                contents = load_tfile(key_filename + '.gpg')
> +                self.assertMultiLineEqual(EXPECTED_BINKEY, contents)
>  
>          self.assertTrue(os.path.isfile(filename))
>  


-- 
https://code.launchpad.net/~nacc/curtin/+git/curtin/+merge/393661
Your team curtin developers is subscribed to branch curtin:master.