[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

@@ -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(

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

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)

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)

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)

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):

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."""

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]

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):

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()