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 | |||||
| from pathlib import Path | |||||
| from unittest.mock import Mock, call | |||||
| from click.testing import CliRunner | |||||
| import pytest | |||||
| import swh.scanner.cli as cli | |||||
| import swh.scanner.scanner as scanner | |||||
| DATADIR = Path(__file__).absolute().parent / "data" | |||||
| CONFIG_PATH_GOOD = str(DATADIR / "global.yml") | |||||
douardda: why are these fixtures?
What aspect/feature of a fixture is used here? | |||||
| CONFIG_PATH_GOOD2 = str(DATADIR / "global2.yml") # alternative to global.yml | |||||
| ROOTPATH_GOOD = str(DATADIR) | |||||
| @pytest.fixture(scope="function") | |||||
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? | |||||
| def m_scanner(mocker): | |||||
| """Returns a mock swh.scanner.scanner object with all attributes mocked""" | |||||
| # Customizable mock of scanner module | |||||
| # Fortunately, noop is the default behavior for all methods | |||||
| scanner_mock = Mock(scanner) | |||||
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… | |||||
| yield mocker.patch("swh.scanner.scanner", scanner_mock) | |||||
| @pytest.fixture(scope="function") | |||||
| def spy_configopen(mocker): | |||||
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… | |||||
| """Returns a mock of open builtin scoped to swh.core.config""" | |||||
| yield mocker.patch("swh.core.config.open", wraps=open) | |||||
| @pytest.fixture(scope="function") | |||||
| def cli_runner(monkeypatch, tmp_path): | |||||
| """Return a CliRunner with default environment variable SWH_CONFIG_FILE unset""" | |||||
| BAD_CONFIG_PATH = str(tmp_path / "missing") | |||||
| monkeypatch.setattr(cli, "DEFAULT_CONFIG_PATH", BAD_CONFIG_PATH) | |||||
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`… | |||||
| return CliRunner(env={"SWH_CONFIG_FILE": None}) | |||||
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. | |||||
| # TEST BEGIN | |||||
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… | |||||
| # For nominal code paths, check that the right config file is loaded | |||||
| # scanner is mocked to not run actual scan, config loading is mocked to check its usage | |||||
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… | |||||
| def test_smoke(cli_runner): | |||||
| """Break if basic functionality breaks""" | |||||
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… | |||||
| res = cli_runner.invoke(cli.scanner, ["scan", "-h"]) | |||||
| assert res.exit_code == 0 | |||||
Not Done Inline Actionsoops douardda: oops | |||||
| def test_config_path_option_bad(cli_runner, tmp_path): | |||||
| """Test bad option no envvar bad default""" | |||||
| CONFPATH_BAD = str(tmp_path / "missing") | |||||
| res = cli_runner.invoke(cli.scanner, ["-C", CONFPATH_BAD, "scan", ROOTPATH_GOOD]) | |||||
| assert res.exit_code != 0 | |||||
| def test_default_config_path(cli_runner, m_scanner, spy_configopen, monkeypatch): | |||||
| """Test no option no envvar good default""" | |||||
| monkeypatch.setattr(cli, "DEFAULT_CONFIG_PATH", CONFIG_PATH_GOOD) | |||||
| res = cli_runner.invoke(cli.scanner, ["scan", ROOTPATH_GOOD]) | |||||
| assert res.exit_code == 0 | |||||
| assert spy_configopen.call_args == call(CONFIG_PATH_GOOD) | |||||
| assert m_scanner.scan.call_count == 1 | |||||
| def test_root_no_config(cli_runner, m_scanner, spy_configopen): | |||||
| """Test no config = no option no envvar bad default, good root""" | |||||
| res = cli_runner.invoke(cli.scanner, ["scan", ROOTPATH_GOOD]) | |||||
| assert res.exit_code == 0 | |||||
| assert spy_configopen.call_count == 0 | |||||
| assert m_scanner.scan.call_count == 1 | |||||
| def test_root_bad(cli_runner, tmp_path): | |||||
| """Test no option no envvar bad default bad root""" | |||||
| ROOTPATH_BAD = str(tmp_path / "missing") | |||||
| res = cli_runner.invoke(cli.scanner, ["scan", ROOTPATH_BAD]) | |||||
Not Done Inline Actionssame, needs to be done in this diff. douardda: same, needs to be done in this diff. | |||||
| assert res.exit_code != 0 | |||||
| def test_config_path_envvar_good(cli_runner, m_scanner, spy_configopen): | |||||
| """Test no option good envvar bad default good root""" | |||||
| cli_runner.env["SWH_CONFIG_FILE"] = CONFIG_PATH_GOOD | |||||
| res = cli_runner.invoke(cli.scanner, ["scan", ROOTPATH_GOOD]) | |||||
| assert res.exit_code == 0 | |||||
| assert spy_configopen.call_args == call(CONFIG_PATH_GOOD) | |||||
| assert m_scanner.scan.call_count == 1 | |||||
| def test_config_path_envvar_bad(cli_runner, tmp_path): | |||||
| """Test no option bad envvar bad default good root""" | |||||
| CONFPATH_BAD = str(tmp_path / "missing") | |||||
| cli_runner.env["SWH_CONFIG_FILE"] = CONFPATH_BAD | |||||
| res = cli_runner.invoke(cli.scanner, ["scan", ROOTPATH_GOOD]) | |||||
| assert res.exit_code != 0 | |||||
| def test_config_path_option_envvar(cli_runner, m_scanner, spy_configopen): | |||||
| """Test good option good envvar bad default good root | |||||
| Check that option has precedence over envvar""" | |||||
| cli_runner.env["SWH_CONFIG_FILE"] = CONFIG_PATH_GOOD2 | |||||
| res = cli_runner.invoke( | |||||
| cli.scanner, ["-C", CONFIG_PATH_GOOD, "scan", ROOTPATH_GOOD] | |||||
| ) | |||||
| assert res.exit_code == 0 | |||||
| assert spy_configopen.call_args == call(CONFIG_PATH_GOOD) | |||||
| assert m_scanner.scan.call_count == 1 | |||||
| def test_api_url_option(cli_runner, m_scanner): | |||||
| """Test no config good root good url""" | |||||
| API_URL = "https://example.com/api/1" # without trailing "/" | |||||
| res = cli_runner.invoke(cli.scanner, ["scan", ROOTPATH_GOOD, "-u", API_URL]) | |||||
| assert res.exit_code == 0 | |||||
| assert m_scanner.scan.call_count == 1 | |||||
why are these fixtures?
What aspect/feature of a fixture is used here?