Changeset View
Standalone View
swh/scanner/tests/test_cli.py
- This file was added.
| # Copyright (C) 2020 The Software Heritage developers | |||||
| # See the AUTHORS file at the top-level directory of this distribution | |||||
| # License: GNU General Public License version 3, or any later version | |||||
| # See top-level LICENSE file for more information | |||||
| import os.path | |||||
| from pathlib import Path | |||||
| from unittest.mock import Mock | |||||
| from click.testing import CliRunner | |||||
| import pytest | |||||
| import swh.core.config as config | |||||
| import swh.scanner.cli as cli | |||||
| import swh.scanner.scanner as scanner | |||||
douardda: why are these fixtures?
What aspect/feature of a fixture is used here? | |||||
| DATADIR = os.path.join(os.path.abspath(os.path.dirname(__file__)), "data") | |||||
| TEST_CONFIG_PATH = os.path.join(DATADIR, "global.yml") | |||||
| ROOTPATH_GOOD = os.path.join(DATADIR, "") | |||||
| ROOTPATH_BAD = "~/not_expanduser-ed" | |||||
Not Done Inline Actionswhat??? what's this directory doing here? How can you be sure it exists? douardda: what??? what's this directory doing here? How can you be sure it exists? | |||||
| CONFPATH_GOOD = TEST_CONFIG_PATH | |||||
| CONFPATH_GOOD2 = TEST_CONFIG_PATH.replace("/swh/", "/swh/../swh/") # or a copy? | |||||
| CONFPATH_BAD = "~/not_expanduser-ed" | |||||
| GLOBAL_CONFIG_DATA = Path(TEST_CONFIG_PATH).read_text() | |||||
Not Done Inline Actionssame, not a valid value. ~ may very well be a valid directory. If you want a directory that does not exists, you create a temp directory (that exists), than use a path that is a subdirectoy of this and you are sure does not exists (because you have full control of it). See e.g. the tmpdir fixture https://docs.pytest.org/en/stable/tmpdir.html douardda: same, not a valid value. `~` may very well be a valid directory.
If you want a directory that… | |||||
Not Done Inline ActionsIn this case, as expand_user is never called, this directory cannot exist. tenma: In this case, as expand_user is never called, this directory cannot exist.
Keep it and comment… | |||||
| @pytest.fixture(scope="function") | |||||
| def m_scanner(monkeypatch): | |||||
| """Returns a mock swh.core.config object whose do_scan is noop""" | |||||
Not Done Inline ActionsI don't get it how a unit test can expect to have this an existing file? Please use tmpdirs/tmpfiles for this kind of test. douardda: I don't get it how a unit test can expect to have this an existing file?
Please use… | |||||
| # Customizable mock of scanner module | |||||
| # Fortunately, noop is the default behavior | |||||
| scanner_mock = Mock(scanner) | |||||
| monkeypatch.setattr("swh.scanner.scanner", scanner_mock) | |||||
| yield scanner_mock | |||||
| @pytest.fixture(scope="function") | |||||
| def m_config(monkeypatch): | |||||
Not Done Inline Actionsnot a big deal, but is this really a smoke test? Also if you test the swh scanner --help invocation, why not verify a few sanity checks are ok on the expected result (aka the help string)? douardda: not a big deal, but is this really a smoke test?
Also if you test the `swh scanner --help`… | |||||
| """Returns a mock swh.core.config object that return default config data""" | |||||
| import yaml | |||||
Not Done Inline ActionsThis is not a TODO, according the very nature of this diff, testing this is a must have. douardda: This is not a TODO, according the very nature of this diff, testing this is a must have. | |||||
| # Customizable mock of config module, with some methods not mocked | |||||
| # Whitelist approach, could be stricter with Mock.spec_set | |||||
Not Done Inline Actionsnote that making this comment a docstring makes pytest display it when the test fail (in verbose mode I believe). douardda: note that making this comment a docstring makes pytest display it when the test fail (in… | |||||
| config_mock = Mock(config) | |||||
| config_mock.config_exists = config.config_exists | |||||
| config_mock.read_raw_config = Mock(return_value=yaml.safe_load(GLOBAL_CONFIG_DATA)) | |||||
| config_mock.merge_configs = config.merge_configs | |||||
Not Done Inline ActionsUsing these kind of helper functions (cli_run_[n]ok) as is (return a boolean) has a rather annoying drawbacks: on failure, the test only reports an "False is not True' kind of message, which is not so user friendly. It's better to implement theses as assert_cli_run_ok or silmilar and make them do the assertion (and use a better assertion message is possible). It's also prevent you from writing: assert call_function() which is not very meaningful for the fellow code reader (even if in this case the function name is rather explanatory). douardda: Using these kind of helper functions (cli_run_[n]ok) as is (return a boolean) has a rather… | |||||
| monkeypatch.setattr("swh.scanner.cli.config", config_mock) | |||||
| yield config_mock | |||||
Not Done Inline Actionsit's unclear to me in what situation an execution raises an exception or prints error messages but does have a null exit code. Is this really happening ? If not, checking the exit code is enough. Otherwise, there is something else to fix :-) douardda: it's unclear to me in what situation an execution raises an exception or prints error messages… | |||||
Not Done Inline Actionsoops douardda: oops | |||||
| @pytest.fixture(scope="function") | |||||
| def cli_runner(): | |||||
| """Return a CliRunner with default environment variable SWH_CONFIG_FILE unset""" | |||||
| return CliRunner(env={"SWH_CONFIG_FILE": ""}) | |||||
| @pytest.fixture(scope="function") | |||||
| def good_default_config_path(monkeypatch): | |||||
| """Patch default config path to a valid value""" | |||||
| assert os.path.exists(TEST_CONFIG_PATH) | |||||
| monkeypatch.setattr(cli, "DEFAULT_CONFIG_PATH", TEST_CONFIG_PATH) | |||||
| @pytest.fixture(scope="function") | |||||
| def bad_default_config_path(monkeypatch, tmpdir): | |||||
| """Patch default config path to an invalid value""" | |||||
| default_config_path = os.path.join(tmpdir, "missing") | |||||
| monkeypatch.setattr(cli, "DEFAULT_CONFIG_PATH", default_config_path) | |||||
| def cli_run_ok(res): | |||||
| """Return whether cli command ran successfully, given its result object""" | |||||
| return res.exit_code == 0 and res.exception is None and "Error:" not in res.stdout | |||||
| # TEST BEGIN | |||||
| # For nominal code paths, check that the right config file is loaded with a mock | |||||
| # mock scanner.scan to not run actual scan | |||||
| # TODO give ROOTPATH_GOOD an existing package within the temp directory | |||||
Not Done Inline Actionssame, needs to be done in this diff. douardda: same, needs to be done in this diff. | |||||
| def test_smoke(cli_runner): | |||||
| """Break if basic functionality breaks""" | |||||
| res = cli_runner.invoke(cli.scanner, ["scan", "-h"]) | |||||
| assert cli_run_ok(res) | |||||
| def test_config_path_option_bad(cli_runner, bad_default_config_path): | |||||
| """Test bad option no envvar bad default""" | |||||
| res = cli_runner.invoke(cli.scanner, ["-C", CONFPATH_BAD, "scan", ROOTPATH_GOOD]) | |||||
| assert res.exit_code != 0 and res.exception.__class__ == FileNotFoundError | |||||
| def test_default_config_path(cli_runner, good_default_config_path, m_scanner, m_config): | |||||
| """Test no option no envvar good default""" | |||||
| res = cli_runner.invoke(cli.scanner, ["scan", ROOTPATH_GOOD]) | |||||
| assert cli_run_ok(res) | |||||
| m_config.read_raw_config.assert_called_with( | |||||
| m_config.config_basepath(TEST_CONFIG_PATH) | |||||
| ) | |||||
| m_scanner.do_scan.assert_called_once() | |||||
| def test_root_no_config(cli_runner, bad_default_config_path, m_scanner, m_config): | |||||
| """Test no config = no option no envvar bad default, good root""" | |||||
| res = cli_runner.invoke(cli.scanner, ["scan", ROOTPATH_GOOD]) | |||||
| assert cli_run_ok(res) | |||||
| m_config.read_raw_config.assert_not_called() | |||||
| m_scanner.do_scan.assert_called_once() | |||||
| def test_root_bad(cli_runner, bad_default_config_path): | |||||
| """Test no option no envvar bad default bad root""" | |||||
| res = cli_runner.invoke(cli.scanner, ["scan", ROOTPATH_BAD]) | |||||
| assert res.exit_code == 2 | |||||
| def test_config_path_envvar_good( | |||||
| cli_runner, bad_default_config_path, m_scanner, m_config | |||||
| ): | |||||
| """Test no option good envvar bad default good root""" | |||||
| cli_runner.env["SWH_CONFIG_FILE"] = CONFPATH_GOOD | |||||
| res = cli_runner.invoke(cli.scanner, ["scan", ROOTPATH_GOOD]) | |||||
| assert cli_run_ok(res) | |||||
| m_config.read_raw_config.assert_called_with(m_config.config_basepath(CONFPATH_GOOD)) | |||||
| m_scanner.do_scan.assert_called_once() | |||||
| def test_config_path_envvar_bad(cli_runner, bad_default_config_path): | |||||
| """Test no option bad envvar bad default good root""" | |||||
| cli_runner.env["SWH_CONFIG_FILE"] = CONFPATH_BAD | |||||
| res = cli_runner.invoke(cli.scanner, ["scan", ROOTPATH_GOOD]) | |||||
| assert res.exception.__class__ == FileNotFoundError | |||||
| def test_config_path_option_envvar( | |||||
| cli_runner, bad_default_config_path, m_scanner, m_config | |||||
| ): | |||||
| """Test good option good envvar bad default good root | |||||
| Check that option has precedence over envvar""" | |||||
| cli_runner.env["SWH_CONFIG_FILE"] = CONFPATH_GOOD2 | |||||
| res = cli_runner.invoke(cli.scanner, ["-C", CONFPATH_GOOD, "scan", ROOTPATH_GOOD]) | |||||
| assert cli_run_ok(res) | |||||
| m_config.read_raw_config.assert_called_with(m_config.config_basepath(CONFPATH_GOOD)) | |||||
| m_scanner.do_scan.assert_called_once() | |||||
| def test_api_url_option(cli_runner, bad_default_config_path, m_scanner, m_config): | |||||
| """Test no config good root good url""" | |||||
| api_url = cli.DEFAULT_CONFIG["web-api"]["url"] | |||||
| res = cli_runner.invoke(cli.scanner, ["scan", ROOTPATH_GOOD, "-u", api_url]) | |||||
| assert cli_run_ok(res) | |||||
| m_config.read_raw_config.assert_not_called() | |||||
| m_scanner.do_scan.assert_called_once() | |||||
why are these fixtures?
What aspect/feature of a fixture is used here?