On Thu, Feb 2, 2017 at 8:27 PM, Scott Moser <email address hidden> wrote:
> Ryan, this looks good. > Some comments in line, possibly me just misunderstanding. > > > Diff comments: > > > > > === added file 'tests/unittests/test_gpg.py' > > --- tests/unittests/test_gpg.py 1970-01-01 00:00:00 +0000 > > +++ tests/unittests/test_gpg.py 2017-02-02 22:52:32 +0000 > > @@ -0,0 +1,174 @@ > > +from unittest import TestCase > > +from mock import call, patch > > +import textwrap > > + > > +from curtin import gpg > > +from curtin import util > > + > > + > > +class TestCurtinGpg(TestCase): > > + > > + @patch('curtin.util.subp') > > + def test_export_armour(self, mock_subp): > > + key = 'DEADBEEF' > > + expected_armour = textwrap.dedent(""" > > + -----BEGIN PGP PUBLIC KEY BLOCK----- > > + Version: GnuPG v1 > > + > > + deadbeef > > + -----END PGP PUBLIC KEY BLOCK---- > > + """) > > + mock_subp.side_effect = iter([(expected_armour, "")]) > > what is the second side affect for ? the ""? > I dont think subp should be called twice for this, is it ? >
subp usually returns an (out, err) tuple; It's a habit in case we need to do something with the error in the calling code.
> > > + > > + armour = gpg.export_armour(key) > > + mock_subp.assert_called_with(["gpg", "--export", "--armour", > key], > > + capture=True) > > + self.assertEqual(expected_armour, armour) > > + > > + @patch('curtin.util.subp') > > + def test_export_armour_missingkey(self, mock_subp): > > + key = 'DEADBEEF' > > + mock_subp.side_effect = iter([util.ProcessExecutionError()]) > > + > > + expected_armour = gpg.export_armour(key) > > + mock_subp.assert_called_with(["gpg", "--export", "--armour", > key], > > + capture=True) > > + self.assertEqual(None, expected_armour) > > + > > + @patch('curtin.util.subp') > > + def test_recv_key(self, mock_subp): > > + key = 'DEADBEEF' > > + keyserver = 'keyserver.ubuntu.com' > > + mock_subp.side_effect = iter([("", "")]) > > + > > + gpg.recv_key(key, keyserver, ) > > calling here is wierd with the following , > > And again, why the 2 side_affects ? shouldn't subp just be called once? >
It's a single side-effect with a tuple return.
the iter() is for python mock on trusty which doesn't convert the list of side-effects into an iterable directly.
> > > + mock_subp.assert_called_with(["gpg", "--keyserver", keyserver, > > + "--recv", key], capture=True, > > + retries=None) > > + > > + @patch('time.sleep') > > + @patch('curtin.util._subp') > > + def test_recv_key_retry_raises(self, mock_under_subp, mock_sleep): > > + key = 'DEADBEEF' > > + keyserver = 'keyserver.ubuntu.com' > > + retries = (1, 2, 5, 10) > > + mock_under_subp.side_effect = iter([ > > + util.ProcessExecutionError(), # 1 > > + util.ProcessExecutionError(), # 2 > > + util.ProcessExecutionError(), # 3 > > + util.ProcessExecutionError(), # 4 > > + util.ProcessExecutionError(), # Final call > > + ]) > > + > > + with self.assertRaises(ValueError): > > + gpg.recv_key(key, keyserver, retries=retries) > > + > > + print("_subp calls: %s" % mock_under_subp.call_args_list) > > + print("sleep calls: %s" % mock_sleep.call_args_list) > > + expected_calls = [ > > + call(["gpg", "--keyserver", keyserver, "--recv", key], > > + capture=True), > > + call(["gpg", "--keyserver", keyserver, "--recv", key], > > + capture=True), > > + call(["gpg", "--keyserver", keyserver, "--recv", key], > > + capture=True), > > + call(["gpg", "--keyserver", keyserver, "--recv", key], > > + capture=True), > > + call(["gpg", "--keyserver", keyserver, "--recv", key], > > + capture=True), > > since these are all the same it is shorter and more obvious (i think) with: > expected_calls = [mock.call(["gpg", "--keyserver" ... ])] * 5 >
Nice! I'll do that.
> > > + ] > > + mock_under_subp.assert_has_calls(expected_calls) > > + > > + expected_calls = [call(1), call(2), call(5), call(10)] > > + mock_sleep.assert_has_calls(expected_calls) > > + > > + @patch('time.sleep') > > + @patch('curtin.util._subp') > > + def test_recv_key_retry_works(self, mock_under_subp, mock_sleep): > > + key = 'DEADBEEF' > > you dont' use key in this test. >
? it's passed to gpg.recv_key) and checked in the expected_calls.
> > > + keyserver = 'keyserver.ubuntu.com' > > + mock_under_subp.side_effect = iter([ > > + util.ProcessExecutionError(), # 1 > > + ("", ""), > > + ]) > > + > > + gpg.recv_key(key, keyserver, retries=[1]) > > + > > + print("_subp calls: %s" % mock_under_subp.call_args_list) > > + print("sleep calls: %s" % mock_sleep.call_args_list) > > + expected_calls = [ > > + call(["gpg", "--keyserver", keyserver, "--recv", key], > > + capture=True), > > + call(["gpg", "--keyserver", keyserver, "--recv", key], > > + capture=True), > > + ] > > + mock_under_subp.assert_has_calls(expected_calls) > > + mock_sleep.assert_has_calls([call(1)]) > > + > > + @patch('curtin.util.subp') > > + def test_delete_key(self, mock_subp): > > + key = 'DEADBEEF' > > + mock_subp.side_effect = iter([("", "")]) > > + > > + gpg.delete_key(key) > > + mock_subp.assert_called_with(["gpg", "--batch", "--yes", > > + "--delete-keys", key], > capture=True) > > + > > + @patch('curtin.gpg.delete_key') > > + @patch('curtin.gpg.recv_key') > > + @patch('curtin.gpg.export_armour') > > + def test_getkeybyid(self, mock_export, mock_recv, mock_del): > > + key = 'DEADBEEF' > > + keyserver = 'my.keyserver.xyz.co.uk' > > + > > + mock_export.side_effect = iter([ > > + None, > > + "-----BEGIN PGP PUBLIC KEY BLOCK-----", > > + ]) > > + > > + gpg.getkeybyid(key, keyserver=keyserver) > > + > > + mock_export.assert_has_calls([call(key), call(key)]) > > + mock_recv.assert_has_calls([ > > + call(key, keyserver=keyserver, retries=None)]) > > + mock_del.assert_has_calls([call(key)]) > > + > > + @patch('curtin.gpg.delete_key') > > + @patch('curtin.gpg.recv_key') > > + @patch('curtin.gpg.export_armour') > > + def test_getkeybyid_exists(self, mock_export, mock_recv, mock_del): > > + key = 'DEADBEEF' > > + > > + mock_export.side_effect = iter([ > > + "-----BEGIN PGP PUBLIC KEY BLOCK-----", > > + ]) > > + > > + gpg.getkeybyid(key) > > + > > + mock_export.assert_has_calls([call(key)]) > > + self.assertEqual([], mock_recv.call_args_list) > > + self.assertEqual([], mock_del.call_args_list) > > + > > + @patch('curtin.gpg.delete_key') > > + @patch('curtin.gpg.recv_key') > > + @patch('curtin.gpg.export_armour') > > + def test_getkeybyid_raises(self, mock_export, mock_recv, mock_del): > > + key = 'DEADBEEF' > > + keyserver = 'my.keyserver.xyz.co.uk' > > + > > + mock_export.side_effect = iter([ > > + None, > > + "-----BEGIN PGP PUBLIC KEY BLOCK-----", > > + ]) > > + mock_recv.side_effect = iter([ > > + ValueError("Failed to import key %s from server %s" % > > + (key, keyserver)), > > + ]) > > + > > + with self.assertRaises(ValueError): > > + gpg.getkeybyid(key, keyserver=keyserver) > > + > > + mock_export.assert_has_calls([call(key)]) > > + mock_recv.assert_has_calls([ > > + call(key, keyserver=keyserver, retries=None)]) > > + mock_del.assert_has_calls([call(key)]) > > > -- > https://code.launchpad.net/~raharper/curtin/trunk.add-gpg- > retry/+merge/316264 > You are the owner of lp:~raharper/curtin/trunk.add-gpg-retry. >
« Back to merge proposal
On Thu, Feb 2, 2017 at 8:27 PM, Scott Moser <email address hidden> wrote:
> Ryan, this looks good. unittests/ test_gpg. py' /test_gpg. py 1970-01-01 00:00:00 +0000 /test_gpg. py 2017-02-02 22:52:32 +0000 TestCase) : 'curtin. util.subp' ) armour( self, mock_subp): side_effect = iter([( expected_ armour, "")])
> Some comments in line, possibly me just misunderstanding.
>
>
> Diff comments:
>
> >
> > === added file 'tests/
> > --- tests/unittests
> > +++ tests/unittests
> > @@ -0,0 +1,174 @@
> > +from unittest import TestCase
> > +from mock import call, patch
> > +import textwrap
> > +
> > +from curtin import gpg
> > +from curtin import util
> > +
> > +
> > +class TestCurtinGpg(
> > +
> > + @patch(
> > + def test_export_
> > + key = 'DEADBEEF'
> > + expected_armour = textwrap.dedent("""
> > + -----BEGIN PGP PUBLIC KEY BLOCK-----
> > + Version: GnuPG v1
> > +
> > + deadbeef
> > + -----END PGP PUBLIC KEY BLOCK----
> > + """)
> > + mock_subp.
>
> what is the second side affect for ? the ""?
> I dont think subp should be called twice for this, is it ?
>
subp usually returns an (out, err) tuple; It's a habit in case we need to
do something with the error in the calling code.
> armour( key) assert_ called_ with([" gpg", "--export", "--armour", l(expected_ armour, armour) 'curtin. util.subp' ) armour_ missingkey( self, mock_subp): side_effect = iter([util. ProcessExecutio nError( )]) armour( key) assert_ called_ with([" gpg", "--export", "--armour", l(None, expected_armour) 'curtin. util.subp' ) ubuntu. com' side_effect = iter([("", "")])
> > +
> > + armour = gpg.export_
> > + mock_subp.
> key],
> > + capture=True)
> > + self.assertEqua
> > +
> > + @patch(
> > + def test_export_
> > + key = 'DEADBEEF'
> > + mock_subp.
> > +
> > + expected_armour = gpg.export_
> > + mock_subp.
> key],
> > + capture=True)
> > + self.assertEqua
> > +
> > + @patch(
> > + def test_recv_key(self, mock_subp):
> > + key = 'DEADBEEF'
> > + keyserver = 'keyserver.
> > + mock_subp.
> > +
> > + gpg.recv_key(key, keyserver, )
>
> calling here is wierd with the following ,
>
> And again, why the 2 side_affects ? shouldn't subp just be called once?
>
It's a single side-effect with a tuple return.
the iter() is for python mock on trusty which doesn't convert the list of
side-effects into an
iterable directly.
> assert_ called_ with([" gpg", "--keyserver", keyserver, 'time.sleep' ) 'curtin. util._subp' ) key_retry_ raises( self, mock_under_subp, mock_sleep): ubuntu. com' subp.side_ effect = iter([ cutionError( ), # 1 cutionError( ), # 2 cutionError( ), # 3 cutionError( ), # 4 cutionError( ), # Final call es(ValueError) : subp.call_ args_list) call_args_ list)
> > + mock_subp.
> > + "--recv", key], capture=True,
> > + retries=None)
> > +
> > + @patch(
> > + @patch(
> > + def test_recv_
> > + key = 'DEADBEEF'
> > + keyserver = 'keyserver.
> > + retries = (1, 2, 5, 10)
> > + mock_under_
> > + util.ProcessExe
> > + util.ProcessExe
> > + util.ProcessExe
> > + util.ProcessExe
> > + util.ProcessExe
> > + ])
> > +
> > + with self.assertRais
> > + gpg.recv_key(key, keyserver, retries=retries)
> > +
> > + print("_subp calls: %s" % mock_under_
> > + print("sleep calls: %s" % mock_sleep.
> > + expected_calls = [
> > + call(["gpg", "--keyserver", keyserver, "--recv", key],
> > + capture=True),
> > + call(["gpg", "--keyserver", keyserver, "--recv", key],
> > + capture=True),
> > + call(["gpg", "--keyserver", keyserver, "--recv", key],
> > + capture=True),
> > + call(["gpg", "--keyserver", keyserver, "--recv", key],
> > + capture=True),
> > + call(["gpg", "--keyserver", keyserver, "--recv", key],
> > + capture=True),
>
> since these are all the same it is shorter and more obvious (i think) with:
> expected_calls = [mock.call(["gpg", "--keyserver" ... ])] * 5
>
Nice! I'll do that.
> subp.assert_ has_calls( expected_ calls) assert_ has_calls( expected_ calls) 'time.sleep' ) 'curtin. util._subp' ) key_retry_ works(self, mock_under_subp, mock_sleep):
> > + ]
> > + mock_under_
> > +
> > + expected_calls = [call(1), call(2), call(5), call(10)]
> > + mock_sleep.
> > +
> > + @patch(
> > + @patch(
> > + def test_recv_
> > + key = 'DEADBEEF'
>
> you dont' use key in this test.
>
? it's passed to gpg.recv_key) and checked in the expected_calls.
> ubuntu. com' subp.side_ effect = iter([ cutionError( ), # 1 subp.call_ args_list) call_args_ list) subp.assert_ has_calls( expected_ calls) assert_ has_calls( [call(1) ]) 'curtin. util.subp' ) key(self, mock_subp): side_effect = iter([("", "")]) assert_ called_ with([" gpg", "--batch", "--yes", 'curtin. gpg.delete_ key') 'curtin. gpg.recv_ key') 'curtin. gpg.export_ armour' ) (self, mock_export, mock_recv, mock_del): xyz.co. uk' side_effect = iter([ keyserver) assert_ has_calls( [call(key) , call(key)]) assert_ has_calls( [ keyserver, retries=None)]) assert_ has_calls( [call(key) ]) 'curtin. gpg.delete_ key') 'curtin. gpg.recv_ key') 'curtin. gpg.export_ armour' ) _exists( self, mock_export, mock_recv, mock_del): side_effect = iter([ assert_ has_calls( [call(key) ]) l([], mock_recv. call_args_ list) l([], mock_del. call_args_ list) 'curtin. gpg.delete_ key') 'curtin. gpg.recv_ key') 'curtin. gpg.export_ armour' ) _raises( self, mock_export, mock_recv, mock_del): xyz.co. uk' side_effect = iter([ side_effect = iter([ es(ValueError) : keyserver) assert_ has_calls( [call(key) ]) assert_ has_calls( [ keyserver, retries=None)]) assert_ has_calls( [call(key) ]) /code.launchpad .net/~raharper/ curtin/ trunk.add- gpg-
> > + keyserver = 'keyserver.
> > + mock_under_
> > + util.ProcessExe
> > + ("", ""),
> > + ])
> > +
> > + gpg.recv_key(key, keyserver, retries=[1])
> > +
> > + print("_subp calls: %s" % mock_under_
> > + print("sleep calls: %s" % mock_sleep.
> > + expected_calls = [
> > + call(["gpg", "--keyserver", keyserver, "--recv", key],
> > + capture=True),
> > + call(["gpg", "--keyserver", keyserver, "--recv", key],
> > + capture=True),
> > + ]
> > + mock_under_
> > + mock_sleep.
> > +
> > + @patch(
> > + def test_delete_
> > + key = 'DEADBEEF'
> > + mock_subp.
> > +
> > + gpg.delete_key(key)
> > + mock_subp.
> > + "--delete-keys", key],
> capture=True)
> > +
> > + @patch(
> > + @patch(
> > + @patch(
> > + def test_getkeybyid
> > + key = 'DEADBEEF'
> > + keyserver = 'my.keyserver.
> > +
> > + mock_export.
> > + None,
> > + "-----BEGIN PGP PUBLIC KEY BLOCK-----",
> > + ])
> > +
> > + gpg.getkeybyid(key, keyserver=
> > +
> > + mock_export.
> > + mock_recv.
> > + call(key, keyserver=
> > + mock_del.
> > +
> > + @patch(
> > + @patch(
> > + @patch(
> > + def test_getkeybyid
> > + key = 'DEADBEEF'
> > +
> > + mock_export.
> > + "-----BEGIN PGP PUBLIC KEY BLOCK-----",
> > + ])
> > +
> > + gpg.getkeybyid(key)
> > +
> > + mock_export.
> > + self.assertEqua
> > + self.assertEqua
> > +
> > + @patch(
> > + @patch(
> > + @patch(
> > + def test_getkeybyid
> > + key = 'DEADBEEF'
> > + keyserver = 'my.keyserver.
> > +
> > + mock_export.
> > + None,
> > + "-----BEGIN PGP PUBLIC KEY BLOCK-----",
> > + ])
> > + mock_recv.
> > + ValueError("Failed to import key %s from server %s" %
> > + (key, keyserver)),
> > + ])
> > +
> > + with self.assertRais
> > + gpg.getkeybyid(key, keyserver=
> > +
> > + mock_export.
> > + mock_recv.
> > + call(key, keyserver=
> > + mock_del.
>
>
> --
> https:/
> retry/+merge/316264
> You are the owner of lp:~raharper/curtin/trunk.add-gpg-retry.
>