From bb70962bb8f7f4bedb5e31c47fad63f30a9eb952 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Mon, 4 Dec 2017 14:44:22 -0800 Subject: [PATCH 1/4] Stop using new mock functionality in tests (#5295) * Remove assert_called_once from dns-route53 * Remove assert_called_once from main_test.py * Remove assert_called() usage in dns-digitalocean * Remove assert_called() usage in dns-route53 * Downgrade mock version in certbot-auto --- .../certbot_dns_digitalocean/dns_digitalocean_test.py | 2 +- .../certbot_dns_route53/dns_route53_test.py | 7 ++++--- certbot/tests/main_test.py | 10 +++++----- letsencrypt-auto-source/letsencrypt-auto | 7 ++++--- .../pieces/dependency-requirements.txt | 7 ++++--- 5 files changed, 18 insertions(+), 15 deletions(-) diff --git a/certbot-dns-digitalocean/certbot_dns_digitalocean/dns_digitalocean_test.py b/certbot-dns-digitalocean/certbot_dns_digitalocean/dns_digitalocean_test.py index 0fdacf4ad..3b8edce64 100644 --- a/certbot-dns-digitalocean/certbot_dns_digitalocean/dns_digitalocean_test.py +++ b/certbot-dns-digitalocean/certbot_dns_digitalocean/dns_digitalocean_test.py @@ -131,7 +131,7 @@ class DigitalOceanClientTest(unittest.TestCase): self.digitalocean_client.del_txt_record(DOMAIN, self.record_name, self.record_content) - correct_record_mock.destroy.assert_called() + self.assertTrue(correct_record_mock.destroy.called) self.assertFalse(first_record_mock.destroy.call_args_list) self.assertFalse(last_record_mock.destroy.call_args_list) diff --git a/certbot-dns-route53/certbot_dns_route53/dns_route53_test.py b/certbot-dns-route53/certbot_dns_route53/dns_route53_test.py index ff07b6ccd..d5f1b2816 100644 --- a/certbot-dns-route53/certbot_dns_route53/dns_route53_test.py +++ b/certbot-dns-route53/certbot_dns_route53/dns_route53_test.py @@ -31,7 +31,7 @@ class AuthenticatorTest(unittest.TestCase, dns_test_common.BaseAuthenticatorTest self.auth._change_txt_record.assert_called_once_with("UPSERT", '_acme-challenge.' + DOMAIN, mock.ANY) - self.auth._wait_for_change.assert_called_once() + self.assertEqual(self.auth._wait_for_change.call_count, 1) def test_perform_no_credentials_error(self): self.auth._change_txt_record = mock.MagicMock(side_effect=NoCredentialsError) @@ -183,7 +183,8 @@ class ClientTest(unittest.TestCase): self.client._change_txt_record("FOO", DOMAIN, "foo") - self.client.r53.change_resource_record_sets.assert_called_once() + call_count = self.client.r53.change_resource_record_sets.call_count + self.assertEqual(call_count, 1) def test_wait_for_change(self): self.client.r53.get_change = mock.MagicMock( @@ -192,7 +193,7 @@ class ClientTest(unittest.TestCase): self.client._wait_for_change(1) - self.client.r53.get_change.assert_called() + self.assertTrue(self.client.r53.get_change.called) if __name__ == "__main__": diff --git a/certbot/tests/main_test.py b/certbot/tests/main_test.py index 45e5db1df..1f690df26 100644 --- a/certbot/tests/main_test.py +++ b/certbot/tests/main_test.py @@ -356,7 +356,7 @@ class DeleteIfAppropriateTest(unittest.TestCase): mock_cert_path_for_cert_name.return_value = "/some/reasonable/path" mock_overlapping_archive_dirs.return_value = False self._call(config) - mock_delete.assert_called_once() + self.assertEqual(mock_delete.call_count, 1) # pylint: disable=too-many-arguments @mock.patch('certbot.storage.renewal_file_for_certname') @@ -375,7 +375,7 @@ class DeleteIfAppropriateTest(unittest.TestCase): mock_cert_path_to_lineage.return_value = "example.com" mock_overlapping_archive_dirs.return_value = False self._call(config) - mock_delete.assert_called_once() + self.assertEqual(mock_delete.call_count, 1) # pylint: disable=too-many-arguments @mock.patch('certbot.storage.renewal_file_for_certname') @@ -396,7 +396,7 @@ class DeleteIfAppropriateTest(unittest.TestCase): mock_full_archive_dir.return_value = "" mock_match_and_check_overlaps.return_value = "" self._call(config) - mock_delete.assert_called_once() + self.assertEqual(mock_delete.call_count, 1) # pylint: disable=too-many-arguments @mock.patch('certbot.storage.renewal_file_for_certname') @@ -415,7 +415,7 @@ class DeleteIfAppropriateTest(unittest.TestCase): mock_cert_path_to_lineage.return_value = config.certname mock_overlapping_archive_dirs.return_value = False self._call(config) - mock_delete.assert_called_once() + self.assertEqual(mock_delete.call_count, 1) # pylint: disable=too-many-arguments @mock.patch('certbot.cert_manager.match_and_check_overlaps') @@ -442,7 +442,7 @@ class DeleteIfAppropriateTest(unittest.TestCase): util_mock = mock_get_utility() util_mock.menu.return_value = (display_util.OK, 0) self._call(config) - mock_delete.assert_called_once() + self.assertEqual(mock_delete.call_count, 1) # pylint: disable=too-many-arguments @mock.patch('certbot.cert_manager.match_and_check_overlaps') diff --git a/letsencrypt-auto-source/letsencrypt-auto b/letsencrypt-auto-source/letsencrypt-auto index 215b684cf..21e47feb8 100755 --- a/letsencrypt-auto-source/letsencrypt-auto +++ b/letsencrypt-auto-source/letsencrypt-auto @@ -1062,9 +1062,10 @@ zope.interface==4.1.3 \ --hash=sha256:928138365245a0e8869a5999fbcc2a45475a0a6ed52a494d60dbdc540335fedd \ --hash=sha256:0d841ba1bb840eea0e6489dc5ecafa6125554971f53b5acb87764441e61bceba \ --hash=sha256:b09c8c1d47b3531c400e0195697f1414a63221de6ef478598a4f1460f7d9a392 -mock==2.0.0 \ - --hash=sha256:5ce3c71c5545b472da17b72268978914d0252980348636840bd34a00b5cc96c1 \ - --hash=sha256:b158b6df76edd239b8208d481dc46b6afd45a846b7812ff0ce58971cf5bc8bba +# Using an older version of mock here prevents regressions of #5276. +mock==1.3.0 \ + --hash=sha256:3f573a18be94de886d1191f27c168427ef693e8dcfcecf95b170577b2eb69cbb \ + --hash=sha256:1e247dbecc6ce057299eb7ee019ad68314bb93152e81d9a6110d35f4d5eca0f6 # Contains the requirements for the letsencrypt package. # diff --git a/letsencrypt-auto-source/pieces/dependency-requirements.txt b/letsencrypt-auto-source/pieces/dependency-requirements.txt index 4b3da685c..dec7ae7d0 100644 --- a/letsencrypt-auto-source/pieces/dependency-requirements.txt +++ b/letsencrypt-auto-source/pieces/dependency-requirements.txt @@ -184,6 +184,7 @@ zope.interface==4.1.3 \ --hash=sha256:928138365245a0e8869a5999fbcc2a45475a0a6ed52a494d60dbdc540335fedd \ --hash=sha256:0d841ba1bb840eea0e6489dc5ecafa6125554971f53b5acb87764441e61bceba \ --hash=sha256:b09c8c1d47b3531c400e0195697f1414a63221de6ef478598a4f1460f7d9a392 -mock==2.0.0 \ - --hash=sha256:5ce3c71c5545b472da17b72268978914d0252980348636840bd34a00b5cc96c1 \ - --hash=sha256:b158b6df76edd239b8208d481dc46b6afd45a846b7812ff0ce58971cf5bc8bba +# Using an older version of mock here prevents regressions of #5276. +mock==1.3.0 \ + --hash=sha256:3f573a18be94de886d1191f27c168427ef693e8dcfcecf95b170577b2eb69cbb \ + --hash=sha256:1e247dbecc6ce057299eb7ee019ad68314bb93152e81d9a6110d35f4d5eca0f6 From 4db7195e7740fc76c1bf27d8d875ef6bdd70eb9a Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Mon, 4 Dec 2017 17:09:01 -0800 Subject: [PATCH 2/4] Fix coveralls (#5298) --- tox.cover.sh | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/tox.cover.sh b/tox.cover.sh index 3f0a5f72e..2b5a3cf19 100755 --- a/tox.cover.sh +++ b/tox.cover.sh @@ -16,7 +16,7 @@ fi cover () { if [ "$1" = "certbot" ]; then - min=97 + min=98 elif [ "$1" = "acme" ]; then min=100 elif [ "$1" = "certbot_apache" ]; then @@ -24,23 +24,23 @@ cover () { elif [ "$1" = "certbot_dns_cloudflare" ]; then min=98 elif [ "$1" = "certbot_dns_cloudxns" ]; then - min=98 + min=99 elif [ "$1" = "certbot_dns_digitalocean" ]; then min=98 elif [ "$1" = "certbot_dns_dnsimple" ]; then min=98 elif [ "$1" = "certbot_dns_dnsmadeeasy" ]; then - min=98 + min=99 elif [ "$1" = "certbot_dns_google" ]; then min=99 elif [ "$1" = "certbot_dns_luadns" ]; then min=98 elif [ "$1" = "certbot_dns_nsone" ]; then - min=98 + min=99 elif [ "$1" = "certbot_dns_rfc2136" ]; then min=99 elif [ "$1" = "certbot_dns_route53" ]; then - min=91 + min=92 elif [ "$1" = "certbot_nginx" ]; then min=97 elif [ "$1" = "letshelp_certbot" ]; then @@ -50,10 +50,12 @@ cover () { exit 1 fi - pytest --cov "$1" --cov-report term-missing \ - --cov-fail-under "$min" --numprocesses auto --pyargs "$1" + pkg_dir=$(echo "$1" | tr _ -) + pytest --cov "$pkg_dir" --cov-append --cov-report= --numprocesses auto --pyargs "$1" + coverage report --fail-under="$min" --include="$pkg_dir/*" --show-missing } +rm -f .coverage # --cov-append is on, make sure stats are correct for pkg in $pkgs do cover $pkg From 8c4f016b2d4524387ce2ddddf0284118eae455b7 Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Wed, 22 Nov 2017 13:00:29 -0800 Subject: [PATCH 3/4] In ACMEv2, challenges have "url" instead of "uri". To handle this smoothly, Challenge's uri field becomes private (_uri), and is joined by _url. Serialization and deserialization will preserve whichever one was set. The uri name is taken over by an @property that returns whichever of the two is set. I chose not to enforce that they shouldn't both be present because it would just add unnecessary code and brittleness with no stability benefit. * Make url a virtual field. * Add @property annotation. --- acme/acme/client_test.py | 2 +- acme/acme/messages.py | 28 +++++++++++++++++++++++----- 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/acme/acme/client_test.py b/acme/acme/client_test.py index 4bd762865..3aac9c874 100644 --- a/acme/acme/client_test.py +++ b/acme/acme/client_test.py @@ -181,7 +181,7 @@ class ClientTest(unittest.TestCase): # TODO: split here and separate test self.assertRaises(errors.UnexpectedUpdate, self.client.answer_challenge, - self.challr.body.update(uri='foo'), chall_response) + self.challr.body.update(_uri='foo'), chall_response) def test_answer_challenge_missing_next(self): self.assertRaises( diff --git a/acme/acme/messages.py b/acme/acme/messages.py index 4b4fa5003..4dee96c58 100644 --- a/acme/acme/messages.py +++ b/acme/acme/messages.py @@ -325,13 +325,26 @@ class ChallengeBody(ResourceBody): """ __slots__ = ('chall',) - uri = jose.Field('uri') + # ACMEv1 has a "uri" field in challenges. ACMEv2 has a "url" field. This + # challenge object supports either one. In Client.answer_challenge, + # whichever one is set will be used. + _uri = jose.Field('uri', omitempty=True, default=None) + _url = jose.Field('url', omitempty=True, default=None) status = jose.Field('status', decoder=Status.from_json, omitempty=True, default=STATUS_PENDING) validated = fields.RFC3339Field('validated', omitempty=True) error = jose.Field('error', decoder=Error.from_json, omitempty=True, default=None) + def __init__(self, **kwargs): + new_kwargs = {} + for k, v in kwargs.items(): + if k in ('uri', 'url',): + k = '_' + k + new_kwargs[k] = v + # pylint: disable=star-args + super(ChallengeBody, self).__init__(**new_kwargs) + def to_partial_json(self): jobj = super(ChallengeBody, self).to_partial_json() jobj.update(self.chall.to_partial_json()) @@ -343,6 +356,11 @@ class ChallengeBody(ResourceBody): jobj_fields['chall'] = challenges.Challenge.from_json(jobj) return jobj_fields + @property + def uri(self): + """The URL of this challenge.""" + return self._url or self._uri + def __getattr__(self, name): return getattr(self.chall, name) @@ -358,10 +376,10 @@ class ChallengeResource(Resource): authzr_uri = jose.Field('authzr_uri') @property - def uri(self): # pylint: disable=missing-docstring,no-self-argument - # bug? 'method already defined line None' - # pylint: disable=function-redefined - return self.body.uri # pylint: disable=no-member + def uri(self): + """The URL of the challenge body.""" + # pylint: disable=function-redefined,no-member + return self.body.uri class Authorization(ResourceBody): From 62c1112d10927026501ff7c1d6a830d5e4fa9fee Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Mon, 4 Dec 2017 20:50:26 -0800 Subject: [PATCH 4/4] Keep the same behavior with the uri attribute --- acme/acme/client_test.py | 2 +- acme/acme/messages.py | 25 +++++++++++++++++-------- acme/acme/messages_test.py | 3 +++ 3 files changed, 21 insertions(+), 9 deletions(-) diff --git a/acme/acme/client_test.py b/acme/acme/client_test.py index 3aac9c874..4bd762865 100644 --- a/acme/acme/client_test.py +++ b/acme/acme/client_test.py @@ -181,7 +181,7 @@ class ClientTest(unittest.TestCase): # TODO: split here and separate test self.assertRaises(errors.UnexpectedUpdate, self.client.answer_challenge, - self.challr.body.update(_uri='foo'), chall_response) + self.challr.body.update(uri='foo'), chall_response) def test_answer_challenge_missing_next(self): self.assertRaises( diff --git a/acme/acme/messages.py b/acme/acme/messages.py index 4dee96c58..2ac29941e 100644 --- a/acme/acme/messages.py +++ b/acme/acme/messages.py @@ -326,8 +326,9 @@ class ChallengeBody(ResourceBody): """ __slots__ = ('chall',) # ACMEv1 has a "uri" field in challenges. ACMEv2 has a "url" field. This - # challenge object supports either one. In Client.answer_challenge, - # whichever one is set will be used. + # challenge object supports either one, but should be accessed through the + # name "uri". In Client.answer_challenge, whichever one is set will be + # used. _uri = jose.Field('uri', omitempty=True, default=None) _url = jose.Field('url', omitempty=True, default=None) status = jose.Field('status', decoder=Status.from_json, @@ -337,13 +338,12 @@ class ChallengeBody(ResourceBody): omitempty=True, default=None) def __init__(self, **kwargs): - new_kwargs = {} - for k, v in kwargs.items(): - if k in ('uri', 'url',): - k = '_' + k - new_kwargs[k] = v + kwargs = dict((self._internal_name(k), v) for k, v in kwargs.items()) # pylint: disable=star-args - super(ChallengeBody, self).__init__(**new_kwargs) + super(ChallengeBody, self).__init__(**kwargs) + + def encode(self, name): + return super(ChallengeBody, self).encode(self._internal_name(name)) def to_partial_json(self): jobj = super(ChallengeBody, self).to_partial_json() @@ -364,6 +364,15 @@ class ChallengeBody(ResourceBody): def __getattr__(self, name): return getattr(self.chall, name) + def __iter__(self): + # When iterating over fields, use the external name 'uri' instead of + # the internal '_uri'. + for name in super(ChallengeBody, self).__iter__(): + yield name[1:] if name == '_uri' else name + + def _internal_name(self, name): + return '_' + name if name == 'uri' else name + class ChallengeResource(Resource): """Challenge Resource. diff --git a/acme/acme/messages_test.py b/acme/acme/messages_test.py index 631f0ce4d..c9e5c2cf1 100644 --- a/acme/acme/messages_test.py +++ b/acme/acme/messages_test.py @@ -283,6 +283,9 @@ class ChallengeBodyTest(unittest.TestCase): 'detail': 'Unable to communicate with DNS server', } + def test_encode(self): + self.assertEqual(self.challb.encode('uri'), self.challb.uri) + def test_to_partial_json(self): self.assertEqual(self.jobj_to, self.challb.to_partial_json())