From 975216481154b061fbc865f7bf28d01f20b68929 Mon Sep 17 00:00:00 2001 From: Dave Dalcino Date: Sun, 27 Aug 2023 12:58:08 -0700 Subject: [PATCH 1/8] Add test for refactored _set_sevenzip --- tests/test_cli.py | 58 +++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 56 insertions(+), 2 deletions(-) diff --git a/tests/test_cli.py b/tests/test_cli.py index 91a568bd..cb88fae7 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -2,7 +2,7 @@ import sys from pathlib import Path from tempfile import TemporaryDirectory -from typing import Dict, List, Optional +from typing import Dict, List, Optional, Union import pytest @@ -382,7 +382,7 @@ def _mocked_run(*args): ) -def test_cli_set_7zip(monkeypatch): +def test_cli_set_7zip_nonexistent(monkeypatch): cli = Cli() cli._setup_settings() with pytest.raises(CliInputError) as err: @@ -391,6 +391,60 @@ def test_cli_set_7zip(monkeypatch): assert format(err.value) == "Specified 7zip command executable does not exist: 'some_nonexistent_binary'" +@pytest.mark.parametrize("external_tool_exists", (True, False)) +def test_set_7zip_checks_external_tool_when_specified(monkeypatch, capsys, external_tool_exists: bool): + cli = Cli() + cli._setup_settings() + external, fallback = "my_7z_extractor", "7zipper" + def mock_subprocess_run(args, **kwargs): + assert args[0] == external + if not external_tool_exists: + raise FileNotFoundError() + + monkeypatch.setattr("aqt.installer.subprocess.run", mock_subprocess_run) + if external_tool_exists: + assert external == cli._set_sevenzip(external, fallback, is_p7zr_missing=False) + else: + with pytest.raises(CliInputError) as err: + cli._set_sevenzip(external, fallback, is_p7zr_missing=False) + assert format(err.value) == format(f"Specified 7zip command executable does not exist: '{external}'") + assert capsys.readouterr()[1] == '' + + +@pytest.mark.parametrize("fallback_exists", (True, False)) +def test_set_7zip_uses_fallback_when_py7zr_missing(monkeypatch, capsys, fallback_exists: bool): + cli = Cli() + cli._setup_settings() + external, fallback = None, "7zipper" + def mock_subprocess_run(args, **kwargs): + assert args[0] == fallback + if not fallback_exists: + raise FileNotFoundError() + + monkeypatch.setattr("aqt.installer.subprocess.run", mock_subprocess_run) + if fallback_exists: + assert fallback == cli._set_sevenzip(external, fallback, is_p7zr_missing=True) + else: + with pytest.raises(CliInputError) as err: + cli._set_sevenzip(external, fallback, is_p7zr_missing=True) + assert format(err.value) == format(f"Fallback 7zip command executable does not exist: '{fallback}'") + assert f"Falling back to '{fallback}'" in capsys.readouterr()[1] + + +@pytest.mark.parametrize("fallback_exists", (True, False)) +def test_set_7zip_chooses_p7zr_when_ext_missing(monkeypatch, capsys, fallback_exists: bool): + cli = Cli() + cli._setup_settings() + external, fallback = None, "7zipper" + + def mock_subprocess_run(args, **kwargs): + assert False, "Should not try to run anything" + + monkeypatch.setattr("aqt.installer.subprocess.run", mock_subprocess_run) + assert cli._set_sevenzip(external, fallback, is_p7zr_missing=False) is None + assert capsys.readouterr()[1] == '' + + @pytest.mark.parametrize( "archive_dest, keep, temp_dir, expect, should_make_dir", ( From 1eeebfb44dfe11bc5dc932752e25d6bc02874ca4 Mon Sep 17 00:00:00 2001 From: Dave Dalcino Date: Sun, 27 Aug 2023 14:48:53 -0700 Subject: [PATCH 2/8] Add passing impl for refactored _set_sevenzip --- aqt/installer.py | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/aqt/installer.py b/aqt/installer.py index 2f9e7912..cbc7e242 100644 --- a/aqt/installer.py +++ b/aqt/installer.py @@ -233,21 +233,26 @@ def _warning_on_bad_combination(self, combo_message: str) -> str: "This may not install properly, but we will try our best." ) - def _set_sevenzip(self, external): + def _set_sevenzip(self, external: Optional[str], fallback: str, is_p7zr_missing: bool) -> Optional[str]: sevenzip = external - if sevenzip is None: - return None - + if not sevenzip: + if is_p7zr_missing: + self.logger.warning(f"The py7zr module failed to load. Falling back to '{fallback}' for .7z extraction.") + self.logger.warning(f"You can use the '--external | -E' flags to select your own extraction tool.") + sevenzip = fallback + else: + # Just use py7zr + return None try: subprocess.run( [sevenzip, "--help"], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, ) + return sevenzip except FileNotFoundError as e: - raise CliInputError("Specified 7zip command executable does not exist: {!r}".format(sevenzip)) from e - - return sevenzip + qualifier = "Specified" if sevenzip == external else "Fallback" + raise CliInputError(f"{qualifier} 7zip command executable does not exist: '{sevenzip}'") from e @staticmethod def _set_arch(arch: Optional[str], os_name: str, target: str, qt_version_or_spec: str) -> str: From 85037194fee380056f21d0859641d31ab59c85e5 Mon Sep 17 00:00:00 2001 From: Dave Dalcino Date: Sun, 27 Aug 2023 15:07:47 -0700 Subject: [PATCH 3/8] Remove fallback parameter --- aqt/installer.py | 14 +++----------- tests/test_cli.py | 20 ++++++++++++-------- 2 files changed, 15 insertions(+), 19 deletions(-) diff --git a/aqt/installer.py b/aqt/installer.py index cbc7e242..c81c6c0f 100644 --- a/aqt/installer.py +++ b/aqt/installer.py @@ -233,10 +233,11 @@ def _warning_on_bad_combination(self, combo_message: str) -> str: "This may not install properly, but we will try our best." ) - def _set_sevenzip(self, external: Optional[str], fallback: str, is_p7zr_missing: bool) -> Optional[str]: + def _set_sevenzip(self, external: Optional[str]) -> Optional[str]: sevenzip = external + fallback = Settings.zipcmd if not sevenzip: - if is_p7zr_missing: + if EXT7Z: self.logger.warning(f"The py7zr module failed to load. Falling back to '{fallback}' for .7z extraction.") self.logger.warning(f"You can use the '--external | -E' flags to select your own extraction tool.") sevenzip = fallback @@ -363,9 +364,6 @@ def run_install_qt(self, args: InstallArgParser): timeout = (Settings.connection_timeout, Settings.response_timeout) modules = args.modules sevenzip = self._set_sevenzip(args.external) - if EXT7Z and sevenzip is None: - # override when py7zr is not exist - sevenzip = self._set_sevenzip("7z") if args.base is not None: if not self._check_mirror(args.base): raise CliInputError( @@ -480,9 +478,6 @@ def _run_src_doc_examples(self, flavor, args, cmd_name: Optional[str] = None): else: timeout = (Settings.connection_timeout, Settings.response_timeout) sevenzip = self._set_sevenzip(args.external) - if EXT7Z and sevenzip is None: - # override when py7zr is not exist - sevenzip = self._set_sevenzip(Settings.zipcmd) modules = getattr(args, "modules", None) # `--modules` is invalid for `install-src` archives = args.archives all_extra = True if modules is not None and "all" in modules else False @@ -554,9 +549,6 @@ def run_install_tool(self, args: InstallToolArgParser): else: base_dir = output_dir sevenzip = self._set_sevenzip(args.external) - if EXT7Z and sevenzip is None: - # override when py7zr is not exist - sevenzip = self._set_sevenzip(Settings.zipcmd) version = getattr(args, "version", None) if version is not None: Cli._validate_version_str(version, allow_minus=True) diff --git a/tests/test_cli.py b/tests/test_cli.py index cb88fae7..acaf4c01 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -7,6 +7,7 @@ import pytest from aqt.exceptions import CliInputError +from aqt.helper import Settings from aqt.installer import Cli from aqt.metadata import MetadataFactory, SimpleSpec, Version @@ -395,18 +396,19 @@ def test_cli_set_7zip_nonexistent(monkeypatch): def test_set_7zip_checks_external_tool_when_specified(monkeypatch, capsys, external_tool_exists: bool): cli = Cli() cli._setup_settings() - external, fallback = "my_7z_extractor", "7zipper" + external = "my_7z_extractor" def mock_subprocess_run(args, **kwargs): assert args[0] == external if not external_tool_exists: raise FileNotFoundError() monkeypatch.setattr("aqt.installer.subprocess.run", mock_subprocess_run) + monkeypatch.setattr("aqt.installer.EXT7Z", False) if external_tool_exists: - assert external == cli._set_sevenzip(external, fallback, is_p7zr_missing=False) + assert external == cli._set_sevenzip(external) else: with pytest.raises(CliInputError) as err: - cli._set_sevenzip(external, fallback, is_p7zr_missing=False) + cli._set_sevenzip(external) assert format(err.value) == format(f"Specified 7zip command executable does not exist: '{external}'") assert capsys.readouterr()[1] == '' @@ -415,18 +417,19 @@ def mock_subprocess_run(args, **kwargs): def test_set_7zip_uses_fallback_when_py7zr_missing(monkeypatch, capsys, fallback_exists: bool): cli = Cli() cli._setup_settings() - external, fallback = None, "7zipper" + external, fallback = None, Settings.zipcmd def mock_subprocess_run(args, **kwargs): assert args[0] == fallback if not fallback_exists: raise FileNotFoundError() monkeypatch.setattr("aqt.installer.subprocess.run", mock_subprocess_run) + monkeypatch.setattr("aqt.installer.EXT7Z", True) if fallback_exists: - assert fallback == cli._set_sevenzip(external, fallback, is_p7zr_missing=True) + assert fallback == cli._set_sevenzip(external) else: with pytest.raises(CliInputError) as err: - cli._set_sevenzip(external, fallback, is_p7zr_missing=True) + cli._set_sevenzip(external) assert format(err.value) == format(f"Fallback 7zip command executable does not exist: '{fallback}'") assert f"Falling back to '{fallback}'" in capsys.readouterr()[1] @@ -435,13 +438,14 @@ def mock_subprocess_run(args, **kwargs): def test_set_7zip_chooses_p7zr_when_ext_missing(monkeypatch, capsys, fallback_exists: bool): cli = Cli() cli._setup_settings() - external, fallback = None, "7zipper" + external = None def mock_subprocess_run(args, **kwargs): assert False, "Should not try to run anything" monkeypatch.setattr("aqt.installer.subprocess.run", mock_subprocess_run) - assert cli._set_sevenzip(external, fallback, is_p7zr_missing=False) is None + monkeypatch.setattr("aqt.installer.EXT7Z", False) + assert cli._set_sevenzip(external) is None assert capsys.readouterr()[1] == '' From 772c20aa4d282d22c46ee1c2b16c27a6d1c8abed Mon Sep 17 00:00:00 2001 From: Dave Dalcino Date: Sun, 27 Aug 2023 15:14:02 -0700 Subject: [PATCH 4/8] Remove duplicated test --- tests/test_cli.py | 9 --------- 1 file changed, 9 deletions(-) diff --git a/tests/test_cli.py b/tests/test_cli.py index acaf4c01..6a84fa00 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -383,15 +383,6 @@ def _mocked_run(*args): ) -def test_cli_set_7zip_nonexistent(monkeypatch): - cli = Cli() - cli._setup_settings() - with pytest.raises(CliInputError) as err: - cli._set_sevenzip("some_nonexistent_binary") - assert err.type == CliInputError - assert format(err.value) == "Specified 7zip command executable does not exist: 'some_nonexistent_binary'" - - @pytest.mark.parametrize("external_tool_exists", (True, False)) def test_set_7zip_checks_external_tool_when_specified(monkeypatch, capsys, external_tool_exists: bool): cli = Cli() From 70dbe2a997e8ebab75f0c3d51ed98c668065893d Mon Sep 17 00:00:00 2001 From: Dave Dalcino Date: Sun, 27 Aug 2023 15:18:40 -0700 Subject: [PATCH 5/8] mypy --- aqt/installer.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/aqt/installer.py b/aqt/installer.py index c81c6c0f..51d61437 100644 --- a/aqt/installer.py +++ b/aqt/installer.py @@ -236,7 +236,7 @@ def _warning_on_bad_combination(self, combo_message: str) -> str: def _set_sevenzip(self, external: Optional[str]) -> Optional[str]: sevenzip = external fallback = Settings.zipcmd - if not sevenzip: + if sevenzip is None: if EXT7Z: self.logger.warning(f"The py7zr module failed to load. Falling back to '{fallback}' for .7z extraction.") self.logger.warning(f"You can use the '--external | -E' flags to select your own extraction tool.") From 112080e6cbcc0bd39ae6cd63d3ca669d72ca88de Mon Sep 17 00:00:00 2001 From: Dave Dalcino Date: Sun, 27 Aug 2023 15:18:47 -0700 Subject: [PATCH 6/8] black --- tests/test_cli.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/test_cli.py b/tests/test_cli.py index 6a84fa00..167ad91d 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -388,6 +388,7 @@ def test_set_7zip_checks_external_tool_when_specified(monkeypatch, capsys, exter cli = Cli() cli._setup_settings() external = "my_7z_extractor" + def mock_subprocess_run(args, **kwargs): assert args[0] == external if not external_tool_exists: @@ -401,7 +402,7 @@ def mock_subprocess_run(args, **kwargs): with pytest.raises(CliInputError) as err: cli._set_sevenzip(external) assert format(err.value) == format(f"Specified 7zip command executable does not exist: '{external}'") - assert capsys.readouterr()[1] == '' + assert capsys.readouterr()[1] == "" @pytest.mark.parametrize("fallback_exists", (True, False)) @@ -409,6 +410,7 @@ def test_set_7zip_uses_fallback_when_py7zr_missing(monkeypatch, capsys, fallback cli = Cli() cli._setup_settings() external, fallback = None, Settings.zipcmd + def mock_subprocess_run(args, **kwargs): assert args[0] == fallback if not fallback_exists: @@ -437,7 +439,7 @@ def mock_subprocess_run(args, **kwargs): monkeypatch.setattr("aqt.installer.subprocess.run", mock_subprocess_run) monkeypatch.setattr("aqt.installer.EXT7Z", False) assert cli._set_sevenzip(external) is None - assert capsys.readouterr()[1] == '' + assert capsys.readouterr()[1] == "" @pytest.mark.parametrize( From 0ffc48a1b12dd1ae417ae0843410b3806b999ac6 Mon Sep 17 00:00:00 2001 From: Dave Dalcino Date: Sun, 27 Aug 2023 15:23:04 -0700 Subject: [PATCH 7/8] remove unused import --- tests/test_cli.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_cli.py b/tests/test_cli.py index 167ad91d..b8985e90 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -2,7 +2,7 @@ import sys from pathlib import Path from tempfile import TemporaryDirectory -from typing import Dict, List, Optional, Union +from typing import Dict, List, Optional import pytest From 2bc3766da71d33e30df4ae7b921efd33c87d54a0 Mon Sep 17 00:00:00 2001 From: Dave Dalcino Date: Sun, 27 Aug 2023 15:51:58 -0700 Subject: [PATCH 8/8] fix format string without placeholders --- aqt/installer.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/aqt/installer.py b/aqt/installer.py index 51d61437..0c39176b 100644 --- a/aqt/installer.py +++ b/aqt/installer.py @@ -239,7 +239,7 @@ def _set_sevenzip(self, external: Optional[str]) -> Optional[str]: if sevenzip is None: if EXT7Z: self.logger.warning(f"The py7zr module failed to load. Falling back to '{fallback}' for .7z extraction.") - self.logger.warning(f"You can use the '--external | -E' flags to select your own extraction tool.") + self.logger.warning("You can use the '--external | -E' flags to select your own extraction tool.") sevenzip = fallback else: # Just use py7zr