curtin-dev team mailing list archive

[Michael Hudson-Doyle <mwhudsonlp@xxxxxxxxxxx>]

Thanks the tests are much nicer but I still have a question I'm afraid. Hopefully close now :)

Diff comments:

> diff --git a/tests/unittests/test_apt_source.py b/tests/unittests/test_apt_source.py
> index 6556399..e40d05d 100644
> --- a/tests/unittests/test_apt_source.py
> +++ b/tests/unittests/test_apt_source.py
> @@ -33,6 +33,21 @@ S0ORP6HXET3+jC8BMG4tBWCTK/XEZw==
>  =ACB2
>  -----END PGP PUBLIC KEY BLOCK-----"""
>  
> +EXPECTEDKEY_NOVER = u"""-----BEGIN PGP PUBLIC KEY BLOCK-----
> +
> +mI0ESuZLUgEEAKkqq3idtFP7g9hzOu1a8+v8ImawQN4TrvlygfScMU1TIS1eC7UQ
> +NUA8Qqgr9iUaGnejb0VciqftLrU9D6WYHSKz+EITefgdyJ6SoQxjoJdsCpJ7o9Jy
> +8PQnpRttiFm4qHu6BVnKnBNxw/z3ST9YMqW5kbMQpfxbGe+obRox59NpABEBAAG0
> +HUxhdW5jaHBhZCBQUEEgZm9yIFNjb3R0IE1vc2VyiLYEEwECACAFAkrmS1ICGwMG
> +CwkIBwMCBBUCCAMEFgIDAQIeAQIXgAAKCRAGILvPA2g/d3aEA/9tVjc10HOZwV29
> +OatVuTeERjjrIbxflO586GLA8cp0C9RQCwgod/R+cKYdQcHjbqVcP0HqxveLg0RZ
> +FJpWLmWKamwkABErwQLGlM/Hwhjfade8VvEQutH5/0JgKHmzRsoqfR+LMO6OS+Sm
> +S0ORP6HXET3+jC8BMG4tBWCTK/XEZw==
> +=ACB2
> +-----END PGP PUBLIC KEY BLOCK-----"""
> +
> +EXPECTED_BINKEY = u"s390x"

So this value appears as the expected value in some asserts, but nothing that I can see would make it the seen value in any of these asserts, so is there some missing coverage? In any case, can you make this something more realistic please :)

> +
>  ADD_APT_REPO_MATCH = r"^[\w-]+:\w"
>  
>  TARGET = "/"
> @@ -215,23 +228,30 @@ class TestAptSourceConfig(CiTestCase):
>                 self.aptlistfile3: {'source': 'deb $MIRROR $RELEASE universe'}}
>          self._apt_src_replace_tri(cfg)
>  
> -    def _apt_src_keyid(self, filename, cfg, keynum):
> +    def _apt_src_keyid(self, filename, cfg):
>          """ _apt_src_keyid
>          Test specification of a source + keyid
>          """
>          params = self._get_default_params()
>  
> -        with mock.patch("curtin.util.subp",
> -                        return_value=('fakekey 1234', '')) as mockobj:
> +        with mock.patch.object(gpg, 'getkeybyid',
> +                               return_value=EXPECTEDKEY_NOVER):
>              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):
> -            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].get('filename', ent)
> +            # 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)

Further to the above, I'm pretty sure this branch is not being taken (I can put 1/0 here and all the tests pass)

>  
>          self.assertTrue(os.path.isfile(filename))
>  
> @@ -338,13 +363,22 @@ class TestAptSourceConfig(CiTestCase):
>          params = self._get_default_params()
>          cfg = {self.aptlistfile: {'keyid': "03683F77"}}
>  
> -        with mock.patch.object(util, 'subp',
> -                               return_value=('fakekey 1212', '')) as mockobj:
> +        with mock.patch.object(gpg, 'getkeybyid',
> +                               return_value=EXPECTEDKEY_NOVER):
>              self._add_apt_sources(cfg, TARGET, template_params=params,
>                                    aa_repo_match=self.matcher)
>  
> -        mockobj.assert_any_call(['apt-key', 'add', '-'], data=b'fakekey 1212',
> -                                target=TARGET)
> +        key_filename = self.aptlistfile
> +        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)

Ditto here. No opinion from me as to whether this indicates missing coverage or test code that can be deleted -- what do you think? :)

>  
>          # filename should be ignored on key only
>          self.assertFalse(os.path.isfile(self.aptlistfile))


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