From eef4c476335a9338dc71509d3d7c21de7d81b486 Mon Sep 17 00:00:00 2001 From: ohemorange Date: Wed, 20 Feb 2019 15:20:44 -0800 Subject: [PATCH 1/4] Add failure message if test farm tests do not run the correct number of tests. (#6771) Fixes #6748. --- tests/letstest/multitester.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/letstest/multitester.py b/tests/letstest/multitester.py index 8babc67b3..b8ae937ad 100644 --- a/tests/letstest/multitester.py +++ b/tests/letstest/multitester.py @@ -564,6 +564,11 @@ try: ii, target, status = outq print('%d %s %s'%(ii, target['name'], status)) results_file.write('%d %s %s\n'%(ii, target['name'], status)) + if len(outputs) != num_processes: + failure_message = 'FAILURE: Some target machines failed to run and were not tested. ' +\ + 'Tests should be rerun.' + print(failure_message) + results_file.write(failure_message + '\n') results_file.close() finally: From eb5c4eca877baf52c0c39778002fce1e9482cff7 Mon Sep 17 00:00:00 2001 From: Adrien Ferrand Date: Thu, 21 Feb 2019 01:20:16 +0100 Subject: [PATCH 2/4] [Windows] Working unit tests for certbot-nginx (#6782) This PR fixes certbot-nginx and relevant tests to make them succeed on Windows. Next step will be to enable integration tests through certbot-ci in a future PR. * Fix tests and incompabilities in certbot-nginx for Windows * Fix lint, fix oldest local dependencies --- certbot-nginx/certbot_nginx/configurator.py | 7 +++---- certbot-nginx/certbot_nginx/parser.py | 4 ++-- .../certbot_nginx/tests/configurator_test.py | 19 ++++++++----------- .../certbot_nginx/tests/http_01_test.py | 6 ------ .../certbot_nginx/tests/parser_test.py | 12 +++++++++--- .../certbot_nginx/tests/tls_sni_01_test.py | 6 ------ certbot-nginx/certbot_nginx/tests/util.py | 18 ++++++++++++++++++ certbot-nginx/local-oldest-requirements.txt | 4 ++-- tools/install_and_test.py | 2 +- tox.cover.py | 5 ++++- 10 files changed, 47 insertions(+), 36 deletions(-) diff --git a/certbot-nginx/certbot_nginx/configurator.py b/certbot-nginx/certbot_nginx/configurator.py index dd0bf9e8b..ffe1ddac7 100644 --- a/certbot-nginx/certbot_nginx/configurator.py +++ b/certbot-nginx/certbot_nginx/configurator.py @@ -13,6 +13,7 @@ import zope.interface from acme import challenges from acme import crypto_util as acme_crypto_util +from certbot import compat from certbot import constants as core_constants from certbot import crypto_util from certbot import errors @@ -164,9 +165,7 @@ class NginxConfigurator(common.Installer): util.lock_dir_until_exit(self.conf('server-root')) except (OSError, errors.LockError): logger.debug('Encountered error:', exc_info=True) - raise errors.PluginError( - 'Unable to lock %s', self.conf('server-root')) - + raise errors.PluginError('Unable to lock {0}'.format(self.conf('server-root'))) # Entry point in main.py for installing cert def deploy_cert(self, domain, cert_path, key_path, @@ -899,7 +898,7 @@ class NginxConfigurator(common.Installer): have permissions of root. """ - uid = os.geteuid() + uid = compat.os_geteuid() util.make_or_verify_dir( self.config.work_dir, core_constants.CONFIG_DIRS_MODE, uid) util.make_or_verify_dir( diff --git a/certbot-nginx/certbot_nginx/parser.py b/certbot-nginx/certbot_nginx/parser.py index 622eb8d55..c5f780d94 100644 --- a/certbot-nginx/certbot_nginx/parser.py +++ b/certbot-nginx/certbot_nginx/parser.py @@ -81,9 +81,9 @@ class NginxParser(object): """ if not os.path.isabs(path): - return os.path.join(self.root, path) + return os.path.normpath(os.path.join(self.root, path)) else: - return path + return os.path.normpath(path) def _build_addr_to_ssl(self): """Builds a map from address to whether it listens on ssl in any server block diff --git a/certbot-nginx/certbot_nginx/tests/configurator_test.py b/certbot-nginx/certbot_nginx/tests/configurator_test.py index 957588e2a..706e2637a 100644 --- a/certbot-nginx/certbot_nginx/tests/configurator_test.py +++ b/certbot-nginx/certbot_nginx/tests/configurator_test.py @@ -1,7 +1,6 @@ # pylint: disable=too-many-public-methods """Test for certbot_nginx.configurator.""" import os -import shutil import unittest import mock @@ -33,12 +32,6 @@ class NginxConfiguratorTest(util.NginxTest): self.config = util.get_nginx_configurator( self.config_path, self.config_dir, self.work_dir, self.logs_dir) - def tearDown(self): - shutil.rmtree(self.temp_dir) - shutil.rmtree(self.config_dir) - shutil.rmtree(self.work_dir) - shutil.rmtree(self.logs_dir) - @mock.patch("certbot_nginx.configurator.util.exe_exists") def test_prepare_no_install(self, mock_exe_exists): mock_exe_exists.return_value = False @@ -69,8 +62,11 @@ class NginxConfiguratorTest(util.NginxTest): def test_prepare_locked(self): server_root = self.config.conf("server-root") + + from certbot import util as certbot_util + certbot_util._LOCKS[server_root].release() # pylint: disable=protected-access + self.config.config_test = mock.Mock() - os.remove(os.path.join(server_root, ".certbot.lock")) certbot_test_util.lock_and_call(self._test_prepare_locked, server_root) @mock.patch("certbot_nginx.configurator.util.exe_exists") @@ -88,11 +84,11 @@ class NginxConfiguratorTest(util.NginxTest): def test_get_all_names(self, mock_gethostbyaddr): mock_gethostbyaddr.return_value = ('155.225.50.69.nephoscale.net', [], []) names = self.config.get_all_names() - self.assertEqual(names, set( - ["155.225.50.69.nephoscale.net", "www.example.org", "another.alias", + self.assertEqual(names, { + "155.225.50.69.nephoscale.net", "www.example.org", "another.alias", "migration.com", "summer.com", "geese.com", "sslon.com", "globalssl.com", "globalsslsetssl.com", "ipv6.com", "ipv6ssl.com", - "headers.com"])) + "headers.com"}) def test_supported_enhancements(self): self.assertEqual(['redirect', 'ensure-http-header', 'staple-ocsp'], @@ -171,6 +167,7 @@ class NginxConfiguratorTest(util.NginxTest): 'abc.www.foo.com': "etc_nginx/foo.conf", 'www.bar.co.uk': "etc_nginx/nginx.conf", 'ipv6.com': "etc_nginx/sites-enabled/ipv6.com"} + conf_path = {key: os.path.normpath(value) for key, value in conf_path.items()} vhost = self.config.choose_vhosts(name)[0] path = os.path.relpath(vhost.filep, self.temp_dir) diff --git a/certbot-nginx/certbot_nginx/tests/http_01_test.py b/certbot-nginx/certbot_nginx/tests/http_01_test.py index ed3c257ee..41c4b95fc 100644 --- a/certbot-nginx/certbot_nginx/tests/http_01_test.py +++ b/certbot-nginx/certbot_nginx/tests/http_01_test.py @@ -1,6 +1,5 @@ """Tests for certbot_nginx.http_01""" import unittest -import shutil import mock import six @@ -54,11 +53,6 @@ class HttpPerformTest(util.NginxTest): from certbot_nginx import http_01 self.http01 = http_01.NginxHttp01(config) - def tearDown(self): - shutil.rmtree(self.temp_dir) - shutil.rmtree(self.config_dir) - shutil.rmtree(self.work_dir) - def test_perform0(self): responses = self.http01.perform() self.assertEqual([], responses) diff --git a/certbot-nginx/certbot_nginx/tests/parser_test.py b/certbot-nginx/certbot_nginx/tests/parser_test.py index f6f28e42b..3a68f7f24 100644 --- a/certbot-nginx/certbot_nginx/tests/parser_test.py +++ b/certbot-nginx/certbot_nginx/tests/parser_test.py @@ -67,9 +67,15 @@ class NginxParserTest(util.NginxTest): #pylint: disable=too-many-public-methods def test_abs_path(self): nparser = parser.NginxParser(self.config_path) - self.assertEqual('/etc/nginx/*', nparser.abs_path('/etc/nginx/*')) - self.assertEqual(os.path.join(self.config_path, 'foo/bar/'), - nparser.abs_path('foo/bar/')) + if os.name != 'nt': + self.assertEqual('/etc/nginx/*', nparser.abs_path('/etc/nginx/*')) + self.assertEqual(os.path.join(self.config_path, 'foo/bar'), + nparser.abs_path('foo/bar')) + else: + self.assertEqual('C:\\etc\\nginx\\*', nparser.abs_path('C:\\etc\\nginx\\*')) + self.assertEqual(os.path.join(self.config_path, 'foo\\bar'), + nparser.abs_path('foo\\bar')) + def test_filedump(self): nparser = parser.NginxParser(self.config_path) diff --git a/certbot-nginx/certbot_nginx/tests/tls_sni_01_test.py b/certbot-nginx/certbot_nginx/tests/tls_sni_01_test.py index 72b65911c..62ca085ef 100644 --- a/certbot-nginx/certbot_nginx/tests/tls_sni_01_test.py +++ b/certbot-nginx/certbot_nginx/tests/tls_sni_01_test.py @@ -1,6 +1,5 @@ """Tests for certbot_nginx.tls_sni_01""" import unittest -import shutil import mock import six @@ -55,11 +54,6 @@ class TlsSniPerformTest(util.NginxTest): from certbot_nginx import tls_sni_01 self.sni = tls_sni_01.NginxTlsSni01(config) - def tearDown(self): - shutil.rmtree(self.temp_dir) - shutil.rmtree(self.config_dir) - shutil.rmtree(self.work_dir) - @mock.patch("certbot_nginx.configurator" ".NginxConfigurator.choose_vhosts") def test_perform(self, mock_choose): diff --git a/certbot-nginx/certbot_nginx/tests/util.py b/certbot-nginx/certbot_nginx/tests/util.py index ad1af2b96..ef669dac0 100644 --- a/certbot-nginx/certbot_nginx/tests/util.py +++ b/certbot-nginx/certbot_nginx/tests/util.py @@ -4,6 +4,8 @@ import os import pkg_resources import tempfile import unittest +import shutil +import warnings import josepy as jose import mock @@ -33,6 +35,22 @@ class NginxTest(unittest.TestCase): # pylint: disable=too-few-public-methods self.rsa512jwk = jose.JWKRSA.load(test_util.load_vector( "rsa512_key.pem")) + def tearDown(self): + # On Windows we have various files which are not correctly closed at the time of tearDown. + # For know, we log them until a proper file close handling is written. + # Useful for development only, so no warning when we are on a CI process. + def onerror_handler(_, path, excinfo): + """On error handler""" + if not os.environ.get('APPVEYOR'): # pragma: no cover + message = ('Following error occurred when deleting path {0}' + 'during tearDown process: {1}'.format(path, str(excinfo))) + warnings.warn(message) + + shutil.rmtree(self.temp_dir, onerror=onerror_handler) + shutil.rmtree(self.config_dir, onerror=onerror_handler) + shutil.rmtree(self.work_dir, onerror=onerror_handler) + shutil.rmtree(self.logs_dir, onerror=onerror_handler) + def get_data_filename(filename): """Gets the filename of a test data file.""" diff --git a/certbot-nginx/local-oldest-requirements.txt b/certbot-nginx/local-oldest-requirements.txt index bcd02d197..db6b261f0 100644 --- a/certbot-nginx/local-oldest-requirements.txt +++ b/certbot-nginx/local-oldest-requirements.txt @@ -1,2 +1,2 @@ -acme[dev]==0.26.0 -certbot[dev]==0.22.0 +acme[dev]==0.29.0 +-e .[dev] diff --git a/tools/install_and_test.py b/tools/install_and_test.py index b15c8eca5..288226527 100755 --- a/tools/install_and_test.py +++ b/tools/install_and_test.py @@ -15,7 +15,7 @@ import subprocess import re SKIP_PROJECTS_ON_WINDOWS = [ - 'certbot-apache', 'certbot-nginx', 'certbot-postfix', 'letshelp-certbot'] + 'certbot-apache', 'certbot-postfix', 'letshelp-certbot'] def call_with_print(command, cwd=None): diff --git a/tox.cover.py b/tox.cover.py index 008424641..e323ba255 100755 --- a/tox.cover.py +++ b/tox.cover.py @@ -35,7 +35,8 @@ COVER_THRESHOLDS = { } SKIP_PROJECTS_ON_WINDOWS = [ - 'certbot-apache', 'certbot-nginx', 'certbot-postfix', 'letshelp-certbot'] + 'certbot-apache', 'certbot-postfix', 'letshelp-certbot'] + def cover(package): threshold = COVER_THRESHOLDS.get(package)['windows' if os.name == 'nt' else 'linux'] @@ -54,6 +55,7 @@ def cover(package): sys.executable, '-m', 'coverage', 'report', '--fail-under', str(threshold), '--include', '{0}/*'.format(pkg_dir), '--show-missing']) + def main(): description = """ This script is used by tox.ini (and thus by Travis CI and AppVeyor) in order @@ -77,5 +79,6 @@ Option -e makes sure we fail fast and don't submit to codecov.""" for package in packages: cover(package) + if __name__ == '__main__': main() From b10ceb7d907cdcc36ec2d745e7802127919ec043 Mon Sep 17 00:00:00 2001 From: Adrien Ferrand Date: Fri, 22 Feb 2019 01:55:08 +0100 Subject: [PATCH 3/4] Fix test sdists with atexit handlers (#6769) So merging the study from @bmw and me, here is what happened. Each invocation of `certbot.logger.post_arg_parse_setup` create a file handler on `letsencrypt.log`. This function also set an atexit handler invoking `logger.shutdown()`, that have the effect to close all logger file handler not already closed at this point. This method is supposed to be called when a python process is close to exit, because it makes all logger unable to write new logs on any handler. Before #6667 and this PR, for tests, the atexit handle would be triggered only at the end of the pytest process. It means that each test that launches `certbot.logger.post_arg_parse_setup` add a new file handler. These tests were typically connecting the file handler on a `letsencrypt.log` located in a temporary directory, and this directory and content was wipped out at each test tearDown. As a consequence, the file handles, not cleared from the logger, were accumulating in the logger, with all of them connected to a deleted file log, except the last one that was just created by the current test. Considering the number of tests concerned, there were ~300 file handler at the end of pytest execution. One can see that, on prior #6667, by calling `print(logger.getLogger().handlers` on the `tearDown` of these tests, and see the array growing at each test execution. Even if this represent a memory leak, this situation was not really a problem on Linux: because a file can be deleted before it is closed, it was only meaning that a given invocation of `logger.debug` for instance, during the tests, was written in 300 log files. The overhead is negligeable. On Windows however, the file handlers were failing because you cannot delete a file before it is closed. It was one of the reason for #6667, that added a call to `logging.shutdown()` at each test tearDown, with the consequence to close all file handlers. At this point, Linux is not happy anymore. Any call to `logger.warn` will generate an error for each closed file handler. As a file handler is added for each test, the number of errors grows on each test, following an arithmetical suite divergence. On `test_sdists.py`, that is using the bare setuptools test suite without output capturing, we can see the damages. The total output takes 216000 lines, and 23000 errors are generated. A decent machine can support this load, but a not a small AWS instance, that is crashing during the execution. Even with pytest, the captured output and the memory leak become so large that segfaults are generated. On the current PR, the problem is solved, by resetting the file handlers array on the logging system on each test tearDown. So each fileHandler is properly closed, and removed from the stack. They do not participate anymore in the logging system, and can be garbage collected. Then we stay on always one file handler opened at any time, and tests can succeed on AWS instances. For the record, here is all the places where the logging system is called and fail if there is still file handlers closed but not cleaned (extracted from the original huge output before correction): ``` Logged from file account.py, line 116 Logged from file account.py, line 178 Logged from file client.py, line 166 Logged from file client.py, line 295 Logged from file client.py, line 415 Logged from file client.py, line 422 Logged from file client.py, line 480 Logged from file client.py, line 503 Logged from file client.py, line 540 Logged from file client.py, line 601 Logged from file client.py, line 622 Logged from file client.py, line 750 Logged from file cli.py, line 220 Logged from file cli.py, line 226 Logged from file crypto_util.py, line 101 Logged from file crypto_util.py, line 127 Logged from file crypto_util.py, line 147 Logged from file crypto_util.py, line 261 Logged from file crypto_util.py, line 283 Logged from file crypto_util.py, line 307 Logged from file crypto_util.py, line 336 Logged from file disco.py, line 116 Logged from file disco.py, line 124 Logged from file disco.py, line 134 Logged from file disco.py, line 138 Logged from file disco.py, line 141 Logged from file dns_common_lexicon.py, line 45 Logged from file dns_common_lexicon.py, line 61 Logged from file dns_common_lexicon.py, line 67 Logged from file dns_common.py, line 316 Logged from file dns_common.py, line 64 Logged from file eff.py, line 60 Logged from file eff.py, line 73 Logged from file error_handler.py, line 105 Logged from file error_handler.py, line 110 Logged from file error_handler.py, line 87 Logged from file hooks.py, line 248 Logged from file main.py, line 1071 Logged from file main.py, line 1075 Logged from file main.py, line 1189 Logged from file ops.py, line 122 Logged from file ops.py, line 325 Logged from file ops.py, line 338 Logged from file reporter.py, line 55 Logged from file selection.py, line 110 Logged from file selection.py, line 118 Logged from file selection.py, line 123 Logged from file selection.py, line 176 Logged from file selection.py, line 231 Logged from file selection.py, line 310 Logged from file selection.py, line 66 Logged from file standalone.py, line 101 Logged from file standalone.py, line 88 Logged from file standalone.py, line 97 Logged from file standalone.py, line 98 Logged from file storage.py, line 52 Logged from file storage.py, line 59 Logged from file storage.py, line 75 Logged from file util.py, line 56 Logged from file webroot.py, line 165 Logged from file webroot.py, line 186 Logged from file webroot.py, line 187 Logged from file webroot.py, line 204 Logged from file webroot.py, line 223 Logged from file webroot.py, line 234 Logged from file webroot.py, line 235 Logged from file webroot.py, line 237 Logged from file webroot.py, line 91 ``` * Reapply #6667 * Make setuptools delegates tests execution to pytest, like in acme module. * Clean handlers at each tearDown to avoid memory leaks. * Update changelog --- CHANGELOG.md | 2 ++ acme/setup.py | 4 +++- certbot-apache/setup.py | 20 ++++++++++++++++++++ certbot-nginx/setup.py | 20 ++++++++++++++++++++ certbot/tests/lock_test.py | 3 +++ certbot/tests/util.py | 33 ++++++++++++++++++--------------- certbot/tests/util_test.py | 24 +++++++++--------------- certbot/util.py | 6 +++--- setup.py | 20 ++++++++++++++++++++ 9 files changed, 98 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 57a895e07..7744e3745 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ Certbot adheres to [Semantic Versioning](https://semver.org/). warnings described at https://github.com/certbot/josepy/issues/13. * Apache plugin now respects CERTBOT_DOCS environment variable when adding command line defaults. +* Tests execution for certbot, certbot-apache and certbot-nginx packages now relies on pytest. ### Fixed @@ -26,6 +27,7 @@ package with changes other than its version number was: * acme * certbot * certbot-apache +* certbot-nginx More details about these changes can be found on our GitHub repo. diff --git a/acme/setup.py b/acme/setup.py index 79d6d3389..6cb5c3f92 100644 --- a/acme/setup.py +++ b/acme/setup.py @@ -36,6 +36,7 @@ docs_extras = [ 'sphinx_rtd_theme', ] + class PyTest(TestCommand): user_options = [] @@ -50,6 +51,7 @@ class PyTest(TestCommand): errno = pytest.main(shlex.split(self.pytest_args)) sys.exit(errno) + setup( name='acme', version=version, @@ -82,7 +84,7 @@ setup( 'dev': dev_extras, 'docs': docs_extras, }, - tests_require=["pytest"], test_suite='acme', + tests_require=["pytest"], cmdclass={"test": PyTest}, ) diff --git a/certbot-apache/setup.py b/certbot-apache/setup.py index 52528a536..5d15611dd 100644 --- a/certbot-apache/setup.py +++ b/certbot-apache/setup.py @@ -1,5 +1,7 @@ from setuptools import setup from setuptools import find_packages +from setuptools.command.test import test as TestCommand +import sys version = '0.32.0.dev0' @@ -21,6 +23,22 @@ docs_extras = [ 'sphinx_rtd_theme', ] + +class PyTest(TestCommand): + user_options = [] + + def initialize_options(self): + TestCommand.initialize_options(self) + self.pytest_args = '' + + def run_tests(self): + import shlex + # import here, cause outside the eggs aren't loaded + import pytest + errno = pytest.main(shlex.split(self.pytest_args)) + sys.exit(errno) + + setup( name='certbot-apache', version=version, @@ -64,4 +82,6 @@ setup( ], }, test_suite='certbot_apache', + tests_require=["pytest"], + cmdclass={"test": PyTest}, ) diff --git a/certbot-nginx/setup.py b/certbot-nginx/setup.py index f78473ada..8984704a6 100644 --- a/certbot-nginx/setup.py +++ b/certbot-nginx/setup.py @@ -1,5 +1,7 @@ from setuptools import setup from setuptools import find_packages +from setuptools.command.test import test as TestCommand +import sys version = '0.32.0.dev0' @@ -21,6 +23,22 @@ docs_extras = [ 'sphinx_rtd_theme', ] + +class PyTest(TestCommand): + user_options = [] + + def initialize_options(self): + TestCommand.initialize_options(self) + self.pytest_args = '' + + def run_tests(self): + import shlex + # import here, cause outside the eggs aren't loaded + import pytest + errno = pytest.main(shlex.split(self.pytest_args)) + sys.exit(errno) + + setup( name='certbot-nginx', version=version, @@ -64,4 +82,6 @@ setup( ], }, test_suite='certbot_nginx', + tests_require=["pytest"], + cmdclass={"test": PyTest}, ) diff --git a/certbot/tests/lock_test.py b/certbot/tests/lock_test.py index 8658443d0..6379693ae 100644 --- a/certbot/tests/lock_test.py +++ b/certbot/tests/lock_test.py @@ -41,6 +41,7 @@ class LockFileTest(test_util.TempDirTestCase): super(LockFileTest, self).setUp() self.lock_path = os.path.join(self.tempdir, 'test.lock') + @test_util.broken_on_windows def test_acquire_without_deletion(self): # acquire the lock in another process but don't delete the file child = multiprocessing.Process(target=self._call, @@ -58,6 +59,7 @@ class LockFileTest(test_util.TempDirTestCase): self.assertRaises, errors.LockError, self._call, self.lock_path) test_util.lock_and_call(assert_raises, self.lock_path) + @test_util.broken_on_windows def test_locked_repr(self): lock_file = self._call(self.lock_path) locked_repr = repr(lock_file) @@ -92,6 +94,7 @@ class LockFileTest(test_util.TempDirTestCase): self._call(self.lock_path) self.assertFalse(should_delete) + @test_util.broken_on_windows def test_removed(self): lock_file = self._call(self.lock_path) lock_file.release() diff --git a/certbot/tests/util.py b/certbot/tests/util.py index 289f2dd9b..4c0c66a42 100644 --- a/certbot/tests/util.py +++ b/certbot/tests/util.py @@ -3,13 +3,14 @@ .. warning:: This module is not part of the public API. """ +import logging import os import pkg_resources import shutil +import stat import tempfile import unittest import sys -import warnings from multiprocessing import Process, Event from cryptography.hazmat.backends import default_backend @@ -329,23 +330,25 @@ class TempDirTestCase(unittest.TestCase): def tearDown(self): """Execute after test""" - # On Windows we have various files which are not correctly closed at the time of tearDown. - # For know, we log them until a proper file close handling is written. - # Useful for development only, so no warning when we are on a CI process. - def onerror_handler(_, path, excinfo): - """On error handler""" - if not os.environ.get('APPVEYOR'): # pragma: no cover - message = ('Following error occurred when deleting the tempdir {0}' - ' for path {1} during tearDown process: {2}' - .format(self.tempdir, path, str(excinfo))) - warnings.warn(message) - shutil.rmtree(self.tempdir, onerror=onerror_handler) + # Cleanup opened resources after a test. This is usually done through atexit handlers in + # Certbot, but during tests, atexit will not run registered functions before tearDown is + # called and instead will run them right before the entire test process exits. + # It is a problem on Windows, that does not accept to clean resources before closing them. + logging.shutdown() + # Remove logging handlers that have been closed so they won't be + # accidentally used in future tests. + logging.getLogger().handlers = [] + util._release_locks() # pylint: disable=protected-access + + def handle_rw_files(_, path, __): + """Handle read-only files, that will fail to be removed on Windows.""" + os.chmod(path, stat.S_IWRITE) + os.remove(path) + shutil.rmtree(self.tempdir, onerror=handle_rw_files) class ConfigTestCase(TempDirTestCase): - """Test class which sets up a NamespaceConfig object. - - """ + """Test class which sets up a NamespaceConfig object.""" def setUp(self): super(ConfigTestCase, self).setUp() self.config = configuration.NamespaceConfig( diff --git a/certbot/tests/util_test.py b/certbot/tests/util_test.py index c85009c62..cac587995 100644 --- a/certbot/tests/util_test.py +++ b/certbot/tests/util_test.py @@ -193,7 +193,12 @@ class CheckPermissionsTest(test_util.TempDirTestCase): def test_wrong_mode(self): os.chmod(self.tempdir, 0o400) - self.assertFalse(self._call(0o600)) + try: + self.assertFalse(self._call(0o600)) + finally: + # Without proper write permissions, Windows is unable to delete a folder, + # even with admin permissions. Write access must be explicitly set first. + os.chmod(self.tempdir, 0o700) class UniqueFileTest(test_util.TempDirTestCase): @@ -279,20 +284,9 @@ class UniqueLineageNameTest(test_util.TempDirTestCase): for f, _ in items: f.close() - @mock.patch("certbot.util.os.fdopen") - def test_failure(self, mock_fdopen): - err = OSError("whoops") - err.errno = errno.EIO - mock_fdopen.side_effect = err - self.assertRaises(OSError, self._call, "wow") - - @mock.patch("certbot.util.os.fdopen") - def test_subsequent_failure(self, mock_fdopen): - self._call("wow") - err = OSError("whoops") - err.errno = errno.EIO - mock_fdopen.side_effect = err - self.assertRaises(OSError, self._call, "wow") + def test_failure(self): + with mock.patch("certbot.util.os.open", side_effect=OSError(errno.EIO)): + self.assertRaises(OSError, self._call, "wow") class SafelyRemoveTest(test_util.TempDirTestCase): diff --git a/certbot/util.py b/certbot/util.py index d7c542465..416075ce8 100644 --- a/certbot/util.py +++ b/certbot/util.py @@ -142,6 +142,7 @@ def _release_locks(): except: # pylint: disable=bare-except msg = 'Exception occurred releasing lock: {0!r}'.format(dir_lock) logger.debug(msg, exc_info=True) + _LOCKS.clear() def set_up_core_dir(directory, mode, uid, strict): @@ -225,9 +226,8 @@ def safe_open(path, mode="w", chmod=None, buffering=None): fdopen_args = () # type: Union[Tuple[()], Tuple[int]] if buffering is not None: fdopen_args = (buffering,) - return os.fdopen( - os.open(path, os.O_CREAT | os.O_EXCL | os.O_RDWR, *open_args), - mode, *fdopen_args) + fd = os.open(path, os.O_CREAT | os.O_EXCL | os.O_RDWR, *open_args) + return os.fdopen(fd, mode, *fdopen_args) def _unique_file(path, filename_pat, count, chmod, mode): diff --git a/setup.py b/setup.py index 14fef37f3..26a1c4293 100644 --- a/setup.py +++ b/setup.py @@ -1,8 +1,10 @@ import codecs import os import re +import sys from setuptools import find_packages, setup +from setuptools.command.test import test as TestCommand # Workaround for http://bugs.python.org/issue8876, see # http://bugs.python.org/issue8876#msg208792 @@ -77,6 +79,22 @@ docs_extras = [ 'sphinx_rtd_theme', ] + +class PyTest(TestCommand): + user_options = [] + + def initialize_options(self): + TestCommand.initialize_options(self) + self.pytest_args = '' + + def run_tests(self): + import shlex + # import here, cause outside the eggs aren't loaded + import pytest + errno = pytest.main(shlex.split(self.pytest_args)) + sys.exit(errno) + + setup( name='certbot', version=version, @@ -123,6 +141,8 @@ setup( # to test all packages run "python setup.py test -s # {acme,certbot_apache,certbot_nginx}" test_suite='certbot', + tests_require=["pytest"], + cmdclass={"test": PyTest}, entry_points={ 'console_scripts': [ From 31b4b8e57c542577d35a20f87eaad60d5ce37193 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Fri, 22 Feb 2019 06:42:01 -0800 Subject: [PATCH 4/4] Log the execution of manual hooks (#6788) * Move logging to execute and fix tests. * update changelog --- CHANGELOG.md | 2 ++ certbot/hooks.py | 36 ++++++++++++++++++++---------------- certbot/plugins/manual.py | 7 +++++-- certbot/tests/hook_test.py | 34 +++++++++++++++++++--------------- 4 files changed, 46 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7744e3745..ce7eac2e9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,8 @@ Certbot adheres to [Semantic Versioning](https://semver.org/). warnings described at https://github.com/certbot/josepy/issues/13. * Apache plugin now respects CERTBOT_DOCS environment variable when adding command line defaults. +* The running of manual plugin hooks is now always included in Certbot's log + output. * Tests execution for certbot, certbot-apache and certbot-nginx packages now relies on pytest. ### Fixed diff --git a/certbot/hooks.py b/certbot/hooks.py index d5239a437..7d2e42fcd 100644 --- a/certbot/hooks.py +++ b/certbot/hooks.py @@ -93,8 +93,7 @@ def _run_pre_hook_if_necessary(command): if command in executed_pre_hooks: logger.info("Pre-hook command already run, skipping: %s", command) else: - logger.info("Running pre-hook command: %s", command) - _run_hook(command) + _run_hook("pre-hook", command) executed_pre_hooks.add(command) @@ -126,8 +125,7 @@ def post_hook(config): _run_eventually(cmd) # certonly / run elif cmd: - logger.info("Running post-hook command: %s", cmd) - _run_hook(cmd) + _run_hook("post-hook", cmd) post_hooks = [] # type: List[str] @@ -149,8 +147,7 @@ def _run_eventually(command): def run_saved_post_hooks(): """Run any post hooks that were saved up in the course of the 'renew' verb""" for cmd in post_hooks: - logger.info("Running post-hook command: %s", cmd) - _run_hook(cmd) + _run_hook("post-hook", cmd) def deploy_hook(config, domains, lineage_path): @@ -220,23 +217,30 @@ def _run_deploy_hook(command, domains, lineage_path, dry_run): os.environ["RENEWED_DOMAINS"] = " ".join(domains) os.environ["RENEWED_LINEAGE"] = lineage_path - logger.info("Running deploy-hook command: %s", command) - _run_hook(command) + _run_hook("deploy-hook", command) -def _run_hook(shell_cmd): +def _run_hook(cmd_name, shell_cmd): """Run a hook command. - :returns: stderr if there was any""" + :param str cmd_name: the user facing name of the hook being run + :param shell_cmd: shell command to execute + :type shell_cmd: `list` of `str` or `str` - err, _ = execute(shell_cmd) + :returns: stderr if there was any""" + err, _ = execute(cmd_name, shell_cmd) return err -def execute(shell_cmd): +def execute(cmd_name, shell_cmd): """Run a command. + :param str cmd_name: the user facing name of the hook being run + :param shell_cmd: shell command to execute + :type shell_cmd: `list` of `str` or `str` + :returns: `tuple` (`str` stderr, `str` stdout)""" + logger.info("Running %s command: %s", cmd_name, shell_cmd) # universal_newlines causes Popen.communicate() # to return str objects instead of bytes in Python 3 @@ -245,12 +249,12 @@ def execute(shell_cmd): out, err = cmd.communicate() base_cmd = os.path.basename(shell_cmd.split(None, 1)[0]) if out: - logger.info('Output from %s:\n%s', base_cmd, out) + logger.info('Output from %s command %s:\n%s', cmd_name, base_cmd, out) if cmd.returncode != 0: - logger.error('Hook command "%s" returned error code %d', - shell_cmd, cmd.returncode) + logger.error('%s command "%s" returned error code %d', + cmd_name, shell_cmd, cmd.returncode) if err: - logger.error('Error output from %s:\n%s', base_cmd, err) + logger.error('Error output from %s command %s:\n%s', cmd_name, base_cmd, err) return (err, out) diff --git a/certbot/plugins/manual.py b/certbot/plugins/manual.py index 8723a1c62..123a5bfea 100644 --- a/certbot/plugins/manual.py +++ b/certbot/plugins/manual.py @@ -202,7 +202,7 @@ permitted by DNS standards.) os.environ.pop('CERTBOT_KEY_PATH', None) os.environ.pop('CERTBOT_SNI_DOMAIN', None) os.environ.update(env) - _, out = hooks.execute(self.conf('auth-hook')) + _, out = self._execute_hook('auth-hook') env['CERTBOT_AUTH_OUTPUT'] = out.strip() self.env[achall] = env @@ -243,5 +243,8 @@ permitted by DNS standards.) if 'CERTBOT_TOKEN' not in env: os.environ.pop('CERTBOT_TOKEN', None) os.environ.update(env) - hooks.execute(self.conf('cleanup-hook')) + self._execute_hook('cleanup-hook') self.reverter.recovery_routine() + + def _execute_hook(self, hook_name): + return hooks.execute(self.option_name(hook_name), self.conf(hook_name)) diff --git a/certbot/tests/hook_test.py b/certbot/tests/hook_test.py index f5bb0c8b5..90f639958 100644 --- a/certbot/tests/hook_test.py +++ b/certbot/tests/hook_test.py @@ -121,7 +121,7 @@ class PreHookTest(HookTest): def _test_nonrenew_common(self): mock_execute = self._call_with_mock_execute(self.config) - mock_execute.assert_called_once_with(self.config.pre_hook) + mock_execute.assert_called_once_with("pre-hook", self.config.pre_hook) self._test_no_executions_common() def test_no_hooks(self): @@ -137,21 +137,21 @@ class PreHookTest(HookTest): def test_renew_disabled_dir_hooks(self): self.config.directory_hooks = False mock_execute = self._call_with_mock_execute(self.config) - mock_execute.assert_called_once_with(self.config.pre_hook) + mock_execute.assert_called_once_with("pre-hook", self.config.pre_hook) self._test_no_executions_common() def test_renew_no_overlap(self): self.config.verb = "renew" mock_execute = self._call_with_mock_execute(self.config) - mock_execute.assert_any_call(self.dir_hook) - mock_execute.assert_called_with(self.config.pre_hook) + mock_execute.assert_any_call("pre-hook", self.dir_hook) + mock_execute.assert_called_with("pre-hook", self.config.pre_hook) self._test_no_executions_common() def test_renew_with_overlap(self): self.config.pre_hook = self.dir_hook self.config.verb = "renew" mock_execute = self._call_with_mock_execute(self.config) - mock_execute.assert_called_once_with(self.dir_hook) + mock_execute.assert_called_once_with("pre-hook", self.dir_hook) self._test_no_executions_common() def _test_no_executions_common(self): @@ -193,7 +193,7 @@ class PostHookTest(HookTest): for verb in ("certonly", "run",): self.config.verb = verb mock_execute = self._call_with_mock_execute(self.config) - mock_execute.assert_called_once_with(self.config.post_hook) + mock_execute.assert_called_once_with("post-hook", self.config.post_hook) self.assertFalse(self._get_eventually()) def test_cert_only_and_run_without_hook(self): @@ -277,12 +277,12 @@ class RunSavedPostHooksTest(HookTest): calls = mock_execute.call_args_list for actual_call, expected_arg in zip(calls, self.eventually): - self.assertEqual(actual_call[0][0], expected_arg) + self.assertEqual(actual_call[0][1], expected_arg) def test_single(self): self.eventually = ["foo"] mock_execute = self._call_with_mock_execute_and_eventually() - mock_execute.assert_called_once_with(self.eventually[0]) + mock_execute.assert_called_once_with("post-hook", self.eventually[0]) class RenewalHookTest(HookTest): @@ -360,7 +360,7 @@ class DeployHookTest(RenewalHookTest): self.config.deploy_hook = "foo" mock_execute = self._call_with_mock_execute( self.config, domains, lineage) - mock_execute.assert_called_once_with(self.config.deploy_hook) + mock_execute.assert_called_once_with("deploy-hook", self.config.deploy_hook) class RenewHookTest(RenewalHookTest): @@ -384,7 +384,7 @@ class RenewHookTest(RenewalHookTest): self.config.directory_hooks = False mock_execute = self._call_with_mock_execute( self.config, ["example.org"], "/foo/bar") - mock_execute.assert_called_once_with(self.config.renew_hook) + mock_execute.assert_called_once_with("deploy-hook", self.config.renew_hook) @mock.patch("certbot.hooks.logger") def test_dry_run(self, mock_logger): @@ -408,13 +408,13 @@ class RenewHookTest(RenewalHookTest): self.config.renew_hook = self.dir_hook mock_execute = self._call_with_mock_execute( self.config, ["example.net", "example.org"], "/foo/bar") - mock_execute.assert_called_once_with(self.dir_hook) + mock_execute.assert_called_once_with("deploy-hook", self.dir_hook) def test_no_overlap(self): mock_execute = self._call_with_mock_execute( self.config, ["example.org"], "/foo/bar") - mock_execute.assert_any_call(self.dir_hook) - mock_execute.assert_called_with(self.config.renew_hook) + mock_execute.assert_any_call("deploy-hook", self.dir_hook) + mock_execute.assert_called_with("deploy-hook", self.config.renew_hook) class ExecuteTest(unittest.TestCase): @@ -433,18 +433,22 @@ class ExecuteTest(unittest.TestCase): def _test_common(self, returncode, stdout, stderr): given_command = "foo" + given_name = "foo-hook" with mock.patch("certbot.hooks.Popen") as mock_popen: mock_popen.return_value.communicate.return_value = (stdout, stderr) mock_popen.return_value.returncode = returncode with mock.patch("certbot.hooks.logger") as mock_logger: - self.assertEqual(self._call(given_command), (stderr, stdout)) + self.assertEqual(self._call(given_name, given_command), (stderr, stdout)) executed_command = mock_popen.call_args[1].get( "args", mock_popen.call_args[0][0]) self.assertEqual(executed_command, given_command) + mock_logger.info.assert_any_call("Running %s command: %s", + given_name, given_command) if stdout: - self.assertTrue(mock_logger.info.called) + mock_logger.info.assert_any_call(mock.ANY, mock.ANY, + mock.ANY, stdout) if stderr or returncode: self.assertTrue(mock_logger.error.called)