[Windows] Fix some unit tests (#6865)

This PR is a part of the effort to remove the last broken unit tests in certbot codebase for Windows, as described in #6850.

This PR fixes various unit tests on Windows, whose resolution was only to modify some logic in the tests, or minor changes in certbot codecase impacting Windows only (like handling correctly paths with DOS-style).

* Correct several tests

* Skip test definitively

* Test to be reactivated with #6497

* Mock log system to avoid errors due to multiple calls to main in main_test

* Simplify mock

* Update cli_test.py

* One test to be repaired when windows file permissions PR is merged

4 files changed, 45 insertions, 45 deletions

diff --git a/certbot/plugins/manual_test.py b/certbot/plugins/manual_test.py
index b566f6340..13e79ce6e 100644
--- a/certbot/plugins/manual_test.py
+++ b/certbot/plugins/manual_test.py
@@ -106,10 +106,10 @@ class AuthenticatorTest(test_util.TempDirTestCase):
                 achall.validation(achall.account_key) in args[0])
             self.assertFalse(kwargs['wrap'])
 
-    @test_util.broken_on_windows
     def test_cleanup(self):
         self.config.manual_public_ip_logging_ok = True
-        self.config.manual_auth_hook = 'echo foo;'
+        self.config.manual_auth_hook = ('{0} -c "import sys; sys.stdout.write(\'foo\')"'
+                                        .format(sys.executable))
         self.config.manual_cleanup_hook = '# cleanup'
         self.auth.perform(self.achalls)
 
diff --git a/certbot/tests/cli_test.py b/certbot/tests/cli_test.py
index f6669e7be..60191730f 100644
--- a/certbot/tests/cli_test.py
+++ b/certbot/tests/cli_test.py
@@ -86,26 +86,26 @@ class ParseTest(unittest.TestCase):  # pylint: disable=too-many-public-methods
 
         return output.getvalue()
 
-    @test_util.broken_on_windows
     @mock.patch("certbot.cli.flag_default")
     def test_cli_ini_domains(self, mock_flag_default):
-        tmp_config = tempfile.NamedTemporaryFile()
-        # use a shim to get ConfigArgParse to pick up tmp_config
-        shim = (
-                lambda v: copy.deepcopy(constants.CLI_DEFAULTS[v])
-                if v != "config_files"
-                else [tmp_config.name]
-                )
-        mock_flag_default.side_effect = shim
-
-        namespace = self.parse(["certonly"])
-        self.assertEqual(namespace.domains, [])
-        tmp_config.write(b"domains = example.com")
-        tmp_config.flush()
-        namespace = self.parse(["certonly"])
-        self.assertEqual(namespace.domains, ["example.com"])
-        namespace = self.parse(["renew"])
-        self.assertEqual(namespace.domains, [])
+        with tempfile.NamedTemporaryFile() as tmp_config:
+            tmp_config.close()  # close now because of compatibility issues on Windows
+            # use a shim to get ConfigArgParse to pick up tmp_config
+            shim = (
+                    lambda v: copy.deepcopy(constants.CLI_DEFAULTS[v])
+                    if v != "config_files"
+                    else [tmp_config.name]
+                    )
+            mock_flag_default.side_effect = shim
+
+            namespace = self.parse(["certonly"])
+            self.assertEqual(namespace.domains, [])
+            with open(tmp_config.name, 'w') as file_h:
+                file_h.write("domains = example.com")
+            namespace = self.parse(["certonly"])
+            self.assertEqual(namespace.domains, ["example.com"])
+            namespace = self.parse(["renew"])
+            self.assertEqual(namespace.domains, [])
 
     def test_no_args(self):
         namespace = self.parse([])
diff --git a/certbot/tests/lock_test.py b/certbot/tests/lock_test.py
index 6379693ae..d2e61e386 100644
--- a/certbot/tests/lock_test.py
+++ b/certbot/tests/lock_test.py
@@ -41,7 +41,6 @@ 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,
@@ -59,12 +58,14 @@ 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)
-        self._test_repr_common(lock_file, locked_repr)
-        self.assertTrue('acquired' in locked_repr)
+        try:
+            locked_repr = repr(lock_file)
+            self._test_repr_common(lock_file, locked_repr)
+            self.assertTrue('acquired' in locked_repr)
+        finally:
+            lock_file.release()
 
     def test_released_repr(self):
         lock_file = self._call(self.lock_path)
@@ -94,7 +95,6 @@ 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/main_test.py b/certbot/tests/main_test.py
index 786b91a94..e1be6e023 100644
--- a/certbot/tests/main_test.py
+++ b/certbot/tests/main_test.py
@@ -593,20 +593,20 @@ class MainTest(test_util.ConfigTestCase):  # pylint: disable=too-many-public-met
             self.assertTrue(message in str(exc))
         self.assertTrue(exc is not None)
 
-    @test_util.broken_on_windows
-    def test_noninteractive(self):
+    @mock.patch('certbot.log.post_arg_parse_setup')
+    def test_noninteractive(self, _):
         args = ['-n', 'certonly']
         self._cli_missing_flag(args, "specify a plugin")
         args.extend(['--standalone', '-d', 'eg.is'])
         self._cli_missing_flag(args, "register before running")
 
-    @test_util.broken_on_windows
+    @mock.patch('certbot.log.post_arg_parse_setup')
     @mock.patch('certbot.main._report_new_cert')
     @mock.patch('certbot.main.client.acme_client.Client')
     @mock.patch('certbot.main._determine_account')
     @mock.patch('certbot.main.client.Client.obtain_and_enroll_certificate')
     @mock.patch('certbot.main._get_and_save_cert')
-    def test_user_agent(self, gsc, _obt, det, _client, unused_report):
+    def test_user_agent(self, gsc, _obt, det, _client, _, __):
         # Normally the client is totally mocked out, but here we need more
         # arguments to automate it...
         args = ["--standalone", "certonly", "-m", "none@none.com",
@@ -655,11 +655,11 @@ class MainTest(test_util.ConfigTestCase):  # pylint: disable=too-many-public-met
             self.assertEqual(call_config.fullchain_path, test_util.temp_join('chain'))
             self.assertEqual(call_config.key_path, test_util.temp_join('privkey'))
 
-    @test_util.broken_on_windows
+    @mock.patch('certbot.log.post_arg_parse_setup')
     @mock.patch('certbot.main._install_cert')
     @mock.patch('certbot.main.plug_sel.record_chosen_plugins')
     @mock.patch('certbot.main.plug_sel.pick_installer')
-    def test_installer_param_override(self, _inst, _rec, mock_install):
+    def test_installer_param_override(self, _inst, _rec, mock_install, _):
         mock_lineage = mock.MagicMock(cert_path=test_util.temp_join('cert'),
                                       chain_path=test_util.temp_join('chain'),
                                       fullchain_path=test_util.temp_join('chain'),
@@ -706,10 +706,10 @@ class MainTest(test_util.ConfigTestCase):  # pylint: disable=too-many-public-met
         self.assertTrue(mock_getcert.called)
         self.assertTrue(mock_inst.called)
 
-    @test_util.broken_on_windows
+    @mock.patch('certbot.log.post_arg_parse_setup')
     @mock.patch('certbot.main._report_new_cert')
     @mock.patch('certbot.util.exe_exists')
-    def test_configurator_selection(self, mock_exe_exists, unused_report):
+    def test_configurator_selection(self, mock_exe_exists, _, __):
         mock_exe_exists.return_value = True
         real_plugins = disco.PluginsRegistry.find_all()
         args = ['--apache', '--authenticator', 'standalone']
@@ -746,8 +746,8 @@ class MainTest(test_util.ConfigTestCase):  # pylint: disable=too-many-public-met
             self._call(["auth", "--standalone"])
             self.assertEqual(1, mock_certonly.call_count)
 
-    @test_util.broken_on_windows
-    def test_rollback(self):
+    @mock.patch('certbot.log.post_arg_parse_setup')
+    def test_rollback(self, _):
         _, _, _, client = self._call(['rollback'])
         self.assertEqual(1, client.rollback.call_count)
 
@@ -774,8 +774,8 @@ class MainTest(test_util.ConfigTestCase):  # pylint: disable=too-many-public-met
         self._call_no_clientmock(['delete'])
         self.assertEqual(1, mock_cert_manager.call_count)
 
-    @test_util.broken_on_windows
-    def test_plugins(self):
+    @mock.patch('certbot.log.post_arg_parse_setup')
+    def test_plugins(self, _):
         flags = ['--init', '--prepare', '--authenticators', '--installers']
         for args in itertools.chain(
                 *(itertools.combinations(flags, r)
@@ -1047,7 +1047,7 @@ class MainTest(test_util.ConfigTestCase):  # pylint: disable=too-many-public-met
         return mock_lineage, mock_get_utility, stdout
 
     @mock.patch('certbot.crypto_util.notAfter')
-    def test_certonly_renewal(self, unused_notafter):
+    def test_certonly_renewal(self, _):
         lineage, get_utility, _ = self._test_renewal_common(True, [])
         self.assertEqual(lineage.save_successor.call_count, 1)
         lineage.update_all_links_to.assert_called_once_with(
@@ -1056,9 +1056,9 @@ class MainTest(test_util.ConfigTestCase):  # pylint: disable=too-many-public-met
         self.assertTrue('fullchain.pem' in cert_msg)
         self.assertTrue('donate' in get_utility().add_message.call_args[0][0])
 
-    @test_util.broken_on_windows
+    @mock.patch('certbot.log.logging.handlers.RotatingFileHandler.doRollover')
     @mock.patch('certbot.crypto_util.notAfter')
-    def test_certonly_renewal_triggers(self, unused_notafter):
+    def test_certonly_renewal_triggers(self, _, __):
         # --dry-run should force renewal
         _, get_utility, _ = self._test_renewal_common(False, ['--dry-run', '--keep'],
                                                       log_out="simulating renewal")
@@ -1125,8 +1125,8 @@ class MainTest(test_util.ConfigTestCase):  # pylint: disable=too-many-public-met
         self.assertTrue('No renewals were attempted.' in stdout.getvalue())
         self.assertTrue('The following certs are not due for renewal yet:' in stdout.getvalue())
 
-    @test_util.broken_on_windows
-    def test_quiet_renew(self):
+    @mock.patch('certbot.log.post_arg_parse_setup')
+    def test_quiet_renew(self, _):
         test_util.make_lineage(self.config.config_dir, 'sample-renewal.conf')
         args = ["renew", "--dry-run"]
         _, _, stdout = self._test_renewal_common(True, [], args=args, should_renew=True)
@@ -1381,8 +1381,8 @@ class MainTest(test_util.ConfigTestCase):  # pylint: disable=too-many-public-met
             self._call(['-c', test_util.vector_path('cli.ini')])
         self.assertTrue(mocked_run.called)
 
-    @test_util.broken_on_windows
-    def test_register(self):
+    @mock.patch('certbot.log.post_arg_parse_setup')
+    def test_register(self, _):
         with mock.patch('certbot.main.client') as mocked_client:
             acc = mock.MagicMock()
             acc.id = "imaginary_account"