From e50fc9444a238d2519126f8972f490fa679dd906 Mon Sep 17 00:00:00 2001 From: Marty Oehme Date: Sat, 29 Nov 2025 11:18:48 +0100 Subject: [PATCH 1/7] test: Restructure test files Extract the individual parsing tests (cli, env, rc) and add additional configuration test file (TConf and config builder). To extract the fixtures they have to go into an additional 'conftest.py' file for pytest to recognize and automatically import them, see: https://docs.pytest.org/en/stable/reference/fixtures.html#conftest-py-sharing-fixtures-across-multiple-files and: https://docs.pytest.org/en/stable/how-to/fixtures.html#using-fixtures-from-other-projects --- test/conftest.py | 27 ++++++ test/test_configuration.py | 178 ++++++++++++++----------------------- test/test_parse_cli.py | 37 ++++++++ test/test_parse_env.py | 35 ++++++++ test/test_parse_rc.py | 36 ++++++++ 5 files changed, 201 insertions(+), 112 deletions(-) create mode 100644 test/conftest.py create mode 100644 test/test_parse_cli.py create mode 100644 test/test_parse_env.py create mode 100644 test/test_parse_rc.py diff --git a/test/conftest.py b/test/conftest.py new file mode 100644 index 0000000..28daf73 --- /dev/null +++ b/test/conftest.py @@ -0,0 +1,27 @@ +from pathlib import Path + +import pytest + +from topen import OPTIONS + + +@pytest.fixture +def fake_id(monkeypatch): + monkeypatch.setattr("sys.argv", ["topen", "0"]) + + +@pytest.fixture +def isolate_env(monkeypatch): + # delete all existing env vars that could interfere + monkeypatch.delenv("EDITOR", raising=False) + monkeypatch.delenv("VISUAL", raising=False) + for opt in OPTIONS.values(): + if opt.env: + monkeypatch.delenv(opt.env, raising=False) + + +@pytest.fixture +def fake_rc(tmp_path: Path, monkeypatch): + rc = tmp_path / "test.taskrc" + monkeypatch.setattr(OPTIONS["task_rc"], "default", rc) + return rc diff --git a/test/test_configuration.py b/test/test_configuration.py index d6b19bb..7afe8c8 100644 --- a/test/test_configuration.py +++ b/test/test_configuration.py @@ -1,118 +1,11 @@ +# pyright: reportUnusedImport=false, reportUnusedParameter=false +# ruff: noqa: F401, F811 +# ^ Turn off for implicit pytest fixture import from pathlib import Path import pytest -from topen import OPTIONS, TConf, parse_cli, parse_env, parse_rc - - -class TestCli: - def test_cli_minimum_id(self, monkeypatch): - monkeypatch.setattr("sys.argv", ["topen", "42"]) - assert parse_cli() == {"task_id": "42"} - - def test_cli_options(self, monkeypatch): - monkeypatch.setattr( - "sys.argv", - [ - "topen", - "123", - "--extension", - "txt", - "--editor", - "vim", - "--quiet", - "True", - ], - ) - assert parse_cli() == { - "task_id": "123", - "notes_ext": "txt", - "notes_editor": "vim", - "notes_quiet": True, - } - - -@pytest.fixture -def isolate_env(monkeypatch): - # delete all existing env vars that could interfere - monkeypatch.delenv("EDITOR", raising=False) - monkeypatch.delenv("VISUAL", raising=False) - for opt in OPTIONS.values(): - if opt.env: - monkeypatch.delenv(opt.env, raising=False) - - -class TestEnv: - def test_env_notes_ext(self, isolate_env, monkeypatch): - monkeypatch.setenv("TOPEN_NOTES_DIR", "/blablubb") - monkeypatch.setenv("TOPEN_NOTES_EXT", "rst") - monkeypatch.setenv("TOPEN_NOTES_ANNOT", "qmd") - monkeypatch.setenv("TOPEN_NOTES_EDITOR", "vim") - monkeypatch.setenv("TOPEN_NOTES_QUIET", "true") - env = parse_env() - assert env["notes_dir"] == Path("/blablubb") - assert env["notes_ext"] == "rst" - assert env["notes_annot"] == "qmd" - assert env["notes_editor"] == "vim" - assert env["notes_quiet"] is True - - def test_env_task_rc(self, isolate_env, monkeypatch): - monkeypatch.setenv("TASKRC", "/a/dir/that/dont/exist/file") - monkeypatch.setenv("TASKDATA", "~/somewhere/tasks") - env = parse_env() - assert env["task_rc"] == Path("/a/dir/that/dont/exist/file") - assert env["task_data"] == Path("~/somewhere/tasks") - - def test_env_parses_boolean_true(self, isolate_env, monkeypatch): - monkeypatch.setenv("TOPEN_NOTES_QUIET", "true") - env = parse_env() - assert env["notes_quiet"] is True - - def test_env_parses_boolean_false(self, isolate_env, monkeypatch): - monkeypatch.setenv("TOPEN_NOTES_QUIET", "false") - env = parse_env() - assert env["notes_quiet"] is False - - -@pytest.fixture -def fake_rc(tmp_path: Path, monkeypatch): - rc = tmp_path / "test.taskrc" - monkeypatch.setattr(OPTIONS["task_rc"], "default", rc) - return rc - - -class TestRcFile: - def test_taskrc_parsing(self, fake_rc): - fake_rc.write_text(""" - data.location=~/.taskies - notes.dir=/there - notes.ext=yaml - notes.annot=Boo! - notes.editor=micro - notes.quiet=true - """) - rc_cfg = parse_rc(fake_rc) - assert rc_cfg["task_data"] == Path("~/.taskies") - assert rc_cfg["notes_dir"] == Path("/there") - assert rc_cfg["notes_ext"] == "yaml" - assert rc_cfg["notes_annot"] == "Boo!" - assert rc_cfg["notes_editor"] == "micro" - assert rc_cfg["notes_quiet"] is True - - def test_taskrc_parses_boolean_true(self, fake_rc): - fake_rc.write_text(""" - notes.quiet=true - """) - rc_cfg = parse_rc(fake_rc) - assert rc_cfg["notes_quiet"] is True - - def test_taskrc_parses_boolean_false(self, fake_rc): - fake_rc.write_text(""" - notes.quiet=false - """) - rc_cfg = parse_rc(fake_rc) - assert rc_cfg["notes_quiet"] is False - +from topen import TConf, build_config class TestTConf: def test_paths_are_expanded(self): @@ -140,7 +33,68 @@ class TestTConf: ({"VISUAL": "nvim", "EDITOR": "notepad"}, "notepad"), ], ) - def test_editor_env_resolution(isolate_env, monkeypatch, env, expected): + def test_editor_env_resolution(self, isolate_env, monkeypatch, env, expected): for k, v in env.items(): monkeypatch.setenv(k, v) assert TConf(0).notes_editor == expected + + +class TestBuildConfigPrecedence: + """ + All tests exercise the same key (notes_ext) to keep the assertions short. + Each source sets a different value so we can be sure the right one wins. + """ + + def test_defaults_only(self, fake_rc, monkeypatch, isolate_env, fake_id): + cfg = build_config() + assert cfg.notes_ext == "md" + + def test_taskrc_overrides_defaults( + self, fake_rc, monkeypatch, isolate_env, fake_id + ): + fake_rc.write_text("notes.ext=from-rc\n") + cfg = build_config() + assert cfg.notes_ext == "from-rc" + + def test_env_overrides_taskrc(self, fake_rc, monkeypatch, isolate_env, fake_id): + fake_rc.write_text("notes.ext=from-rc\n") + monkeypatch.setenv("TOPEN_NOTES_EXT", "from-env") + cfg = build_config() + assert cfg.notes_ext == "from-env" + + def test_cli_overrides_env(self, fake_rc, monkeypatch, isolate_env): + fake_rc.write_text("notes.ext=from-rc\n") + monkeypatch.setenv("TOPEN_NOTES_EXT", "from-env") + monkeypatch.setattr("sys.argv", ["topen", "0", "--extension", "from-cli"]) + cfg = build_config() + assert cfg.notes_ext == "from-cli" + + def test_cli_overrides_everything(self, fake_rc, monkeypatch, isolate_env): + fake_rc.write_text("notes.ext=from-rc\nnotes.dir=/rc-dir\nnotes.editor=joe") + monkeypatch.setenv("TOPEN_NOTES_EXT", "from-env") + monkeypatch.setenv("TOPEN_NOTES_DIR", "/env-dir") + monkeypatch.setenv("EDITOR", "emacs") + # CLI wins + monkeypatch.setattr( + "sys.argv", + [ + "topen", + "0", + "--extension", + "cli-ext", + "--notes-dir", + "cli-dir", + "--editor", + "helix", + ], + ) + cfg = build_config() + assert cfg.notes_ext == "cli-ext" + assert cfg.notes_dir == Path("cli-dir") + assert cfg.notes_editor == "helix" + + # sanity check that the task-id coming from CLI is preserved + def test_cli_supplies_task_id(self, fake_rc, monkeypatch, isolate_env): + monkeypatch.setattr("sys.argv", ["topen", "42"]) + cfg = build_config() + assert cfg.task_id == "42" diff --git a/test/test_parse_cli.py b/test/test_parse_cli.py new file mode 100644 index 0000000..409ca8e --- /dev/null +++ b/test/test_parse_cli.py @@ -0,0 +1,37 @@ +from pathlib import Path + +from topen import parse_cli + + +class TestCli: + def test_cli_minimum_id(self, monkeypatch): + monkeypatch.setattr("sys.argv", ["topen", "42"]) + assert parse_cli() == {"task_id": "42"} + + def test_cli_options(self, monkeypatch): + monkeypatch.setattr( + "sys.argv", + [ + "topen", + "123", + "--extension", + "txt", + "--editor", + "vim", + "--annotation", + "HERENOTE", + ], + ) + assert parse_cli() == { + "task_id": "123", + "notes_ext": "txt", + "notes_editor": "vim", + "notes_annot": "HERENOTE", + } + + def test_cli_parses_paths(self, monkeypatch): + monkeypatch.setattr( + "sys.argv", + ["topen", "123", "--notes-dir", "/somewhere/else"], + ) + assert parse_cli()["notes_dir"] == Path("/somewhere/else") diff --git a/test/test_parse_env.py b/test/test_parse_env.py new file mode 100644 index 0000000..2f637e0 --- /dev/null +++ b/test/test_parse_env.py @@ -0,0 +1,35 @@ +from pathlib import Path + +from topen import parse_env + + +class TestEnv: + def test_env_notes_ext(self, isolate_env, monkeypatch): + monkeypatch.setenv("TOPEN_NOTES_DIR", "/blablubb") + monkeypatch.setenv("TOPEN_NOTES_EXT", "rst") + monkeypatch.setenv("TOPEN_NOTES_ANNOT", "qmd") + monkeypatch.setenv("TOPEN_NOTES_EDITOR", "vim") + monkeypatch.setenv("TOPEN_NOTES_QUIET", "true") + env = parse_env() + assert env["notes_dir"] == Path("/blablubb") + assert env["notes_ext"] == "rst" + assert env["notes_annot"] == "qmd" + assert env["notes_editor"] == "vim" + assert env["notes_quiet"] is True + + def test_env_task_rc(self, isolate_env, monkeypatch): + monkeypatch.setenv("TASKRC", "/a/dir/that/dont/exist/file") + monkeypatch.setenv("TASKDATA", "~/somewhere/tasks") + env = parse_env() + assert env["task_rc"] == Path("/a/dir/that/dont/exist/file") + assert env["task_data"] == Path("~/somewhere/tasks") + + def test_env_parses_boolean_true(self, isolate_env, monkeypatch): + monkeypatch.setenv("TOPEN_NOTES_QUIET", "true") + env = parse_env() + assert env["notes_quiet"] is True + + def test_env_parses_boolean_false(self, isolate_env, monkeypatch): + monkeypatch.setenv("TOPEN_NOTES_QUIET", "false") + env = parse_env() + assert env["notes_quiet"] is False diff --git a/test/test_parse_rc.py b/test/test_parse_rc.py new file mode 100644 index 0000000..c46b094 --- /dev/null +++ b/test/test_parse_rc.py @@ -0,0 +1,36 @@ +from pathlib import Path + +from topen import parse_rc + + +class TestRcFile: + def test_taskrc_parsing(self, fake_rc): + fake_rc.write_text(""" + data.location=~/.taskies + notes.dir=/there + notes.ext=yaml + notes.annot=Boo! + notes.editor=micro + notes.quiet=true + """) + rc_cfg = parse_rc(fake_rc) + assert rc_cfg["task_data"] == Path("~/.taskies") + assert rc_cfg["notes_dir"] == Path("/there") + assert rc_cfg["notes_ext"] == "yaml" + assert rc_cfg["notes_annot"] == "Boo!" + assert rc_cfg["notes_editor"] == "micro" + assert rc_cfg["notes_quiet"] is True + + def test_taskrc_parses_boolean_true(self, fake_rc): + fake_rc.write_text(""" + notes.quiet=true + """) + rc_cfg = parse_rc(fake_rc) + assert rc_cfg["notes_quiet"] is True + + def test_taskrc_parses_boolean_false(self, fake_rc): + fake_rc.write_text(""" + notes.quiet=false + """) + rc_cfg = parse_rc(fake_rc) + assert rc_cfg["notes_quiet"] is False From 1ea149c1de1908cc0005e57fe33d20b907f68b51 Mon Sep 17 00:00:00 2001 From: Marty Oehme Date: Sat, 29 Nov 2025 11:18:48 +0100 Subject: [PATCH 2/7] fix: Ensure quiet is a flag on the cli We regressed quiet into requiring a value to be set as a cli option (`--quiet=true`) instead of just functioning as a flag (`--quiet`). This change restores the previous interface on the command line, and adds a test to ensure no regressions. --- test/test_parse_cli.py | 11 +++++++++++ topen.py | 14 ++++++++++++-- 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/test/test_parse_cli.py b/test/test_parse_cli.py index 409ca8e..984dcf5 100644 --- a/test/test_parse_cli.py +++ b/test/test_parse_cli.py @@ -29,6 +29,17 @@ class TestCli: "notes_annot": "HERENOTE", } + def test_cli_notes_quiet_is_flag(self, monkeypatch): + monkeypatch.setattr( + "sys.argv", + [ + "topen", + "123", + "--quiet", + ], + ) + assert parse_cli()["notes_quiet"] is True + def test_cli_parses_paths(self, monkeypatch): monkeypatch.setattr( "sys.argv", diff --git a/topen.py b/topen.py index 0143907..e04e39e 100755 --- a/topen.py +++ b/topen.py @@ -132,7 +132,7 @@ class Opt: metavar: str | None = None cast: type | Callable = str help_text: str = "" - flag: bool = False + is_flag: bool = False def _real_path(p: Path | str) -> Path: @@ -224,7 +224,8 @@ OPTIONS: dict[str, Opt] = { "notes.quiet", default=False, cast=_strtobool, - help_text="Silence any verbose displayed information", + help_text="Silence any verbosely displayed information", + is_flag=True, ), } @@ -321,6 +322,15 @@ you view the task. for key, opt in OPTIONS.items(): if opt.cli is None: continue + if opt.is_flag: + parser.add_argument( + *opt.cli, + dest=key, + help=opt.help_text, + default=None, + action="store_true", + ) + continue parser.add_argument( *opt.cli, dest=key, From 49bd1292fadeebfdc16a817954493ebeecd8a09a Mon Sep 17 00:00:00 2001 From: Marty Oehme Date: Sat, 29 Nov 2025 18:28:52 +0100 Subject: [PATCH 3/7] ref: Extract note parent dir creation function --- topen.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/topen.py b/topen.py index e04e39e..fae010e 100755 --- a/topen.py +++ b/topen.py @@ -63,9 +63,7 @@ def main(cfg: "TConf | None" = None, io: "_IO | None" = None) -> int: fpath = get_notes_file(uuid, notes_dir=cfg.notes_dir, notes_ext=cfg.notes_ext) - if not fpath.parent.exists(): - fpath.parent.mkdir(parents=True, exist_ok=True) - + _ensure_parent_dir(fpath) io.out(f"Editing note: {fpath}") open_editor(fpath, editor=cfg.notes_editor) @@ -139,6 +137,11 @@ def _real_path(p: Path | str) -> Path: return Path(os.path.expandvars(p)).expanduser() +def _ensure_parent_dir(file: Path) -> None: + if not file.parent.exists(): + file.parent.mkdir(parents=True, exist_ok=True) + + def _determine_default_task_rc() -> Path: if _real_path("~/.taskrc").exists(): return _real_path("~/.taskrc") From 3f10b429a2c023b8f0b9aa24e6a0fefd9a4d3194 Mon Sep 17 00:00:00 2001 From: Marty Oehme Date: Sat, 29 Nov 2025 18:31:09 +0100 Subject: [PATCH 4/7] ref: Rename path expansion function From `_real_path` to `_expand_path` to better express its use. --- topen.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/topen.py b/topen.py index fae010e..05c3f16 100755 --- a/topen.py +++ b/topen.py @@ -133,7 +133,7 @@ class Opt: is_flag: bool = False -def _real_path(p: Path | str) -> Path: +def _expand_path(p: Path | str) -> Path: return Path(os.path.expandvars(p)).expanduser() @@ -143,11 +143,11 @@ def _ensure_parent_dir(file: Path) -> None: def _determine_default_task_rc() -> Path: - if _real_path("~/.taskrc").exists(): - return _real_path("~/.taskrc") - if _real_path("$XDG_CONFIG_HOME/task/taskrc").exists(): - return _real_path("$XDG_CONFIG_HOME/task/taskrc") - return _real_path("~/.config/task/taskrc") + if _expand_path("~/.taskrc").exists(): + return _expand_path("~/.taskrc") + if _expand_path("$XDG_CONFIG_HOME/task/taskrc").exists(): + return _expand_path("$XDG_CONFIG_HOME/task/taskrc") + return _expand_path("~/.config/task/taskrc") def _strtobool(val: str) -> bool: @@ -261,11 +261,11 @@ class TConf: """If set topen will give no feedback on note editing.""" def __post_init__(self): - self.task_rc = _real_path(self.task_rc) - self.task_data = _real_path(self.task_data) + self.task_rc = _expand_path(self.task_rc) + self.task_data = _expand_path(self.task_data) if self.notes_dir == NON_EXISTENT_PATH: self.notes_dir = self._default_notes_dir() - self.notes_dir = _real_path(self.notes_dir) + self.notes_dir = _expand_path(self.notes_dir) if not self.notes_editor: self.notes_editor = ( os.getenv("EDITOR") From 762b4a288f88442ee8704b8c2073e74da68fd797 Mon Sep 17 00:00:00 2001 From: Marty Oehme Date: Sat, 29 Nov 2025 18:28:52 +0100 Subject: [PATCH 5/7] fix: Correct whitespace separation on editor shell call Switch to using 'sequence'-delineated arguments given to the subprocess run call to correctly handle whitespace. Also check the output, so we exit if we have an error. Test accordingly. --- test/test_cli.py | 11 +++++++++++ topen.py | 2 +- 2 files changed, 12 insertions(+), 1 deletion(-) create mode 100644 test/test_cli.py diff --git a/test/test_cli.py b/test/test_cli.py new file mode 100644 index 0000000..fe70abb --- /dev/null +++ b/test/test_cli.py @@ -0,0 +1,11 @@ +from pathlib import Path +from unittest.mock import Mock, patch + +from topen import add_annotation, open_editor + + +def test_open_editor_escapes_shell(): + """Ensure filenames with spaces/metas do not allow shell injection.""" + with patch("subprocess.run") as run_mock: + open_editor(Path("my note$1.txt"), "vim") + run_mock.assert_called_once_with(["vim", "my note$1.txt"], check=True) diff --git a/topen.py b/topen.py index 05c3f16..03f559a 100755 --- a/topen.py +++ b/topen.py @@ -97,7 +97,7 @@ def get_notes_file(uuid: str, notes_dir: Path, notes_ext: str) -> Path: def open_editor(file: Path, editor: str) -> None: """Opens a file with the chosen editor.""" - _ = subprocess.run(f"{editor} {file}", shell=True) + _ = subprocess.run([editor, str(file)], check=True) def is_annotation_missing(task: Task, annotation_content: str) -> bool: From 28c551e15738086982be1c13db2156348421bed2 Mon Sep 17 00:00:00 2001 From: Marty Oehme Date: Sat, 29 Nov 2025 18:42:33 +0100 Subject: [PATCH 6/7] ref: Print optional error message on editor process error Editor function takes an optional io object which is used to print an error output if the subprocess errors. --- test/test_cli.py | 8 ++++++++ topen.py | 10 +++++++--- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/test/test_cli.py b/test/test_cli.py index fe70abb..1d9601b 100644 --- a/test/test_cli.py +++ b/test/test_cli.py @@ -9,3 +9,11 @@ def test_open_editor_escapes_shell(): with patch("subprocess.run") as run_mock: open_editor(Path("my note$1.txt"), "vim") run_mock.assert_called_once_with(["vim", "my note$1.txt"], check=True) + + +# +# def test_add_annotation_saves_task(): +# task = Mock() +# add_annotation(task, "hello") +# task.add_annotation.assert_called_once_with("hello") +# task.save.assert_called_once() diff --git a/topen.py b/topen.py index 03f559a..b5374e1 100755 --- a/topen.py +++ b/topen.py @@ -65,7 +65,7 @@ def main(cfg: "TConf | None" = None, io: "_IO | None" = None) -> int: _ensure_parent_dir(fpath) io.out(f"Editing note: {fpath}") - open_editor(fpath, editor=cfg.notes_editor) + open_editor(fpath, editor=cfg.notes_editor, io=io) if fpath.exists(): if is_annotation_missing(task, annotation_content=cfg.notes_annot): @@ -95,9 +95,13 @@ def get_notes_file(uuid: str, notes_dir: Path, notes_ext: str) -> Path: return Path(notes_dir).joinpath(f"{uuid}.{notes_ext}") -def open_editor(file: Path, editor: str) -> None: +def open_editor(file: Path, editor: str, io: "_IO | None" = None) -> None: """Opens a file with the chosen editor.""" - _ = subprocess.run([editor, str(file)], check=True) + try: + _ = subprocess.run([editor, str(file)], check=True) + except subprocess.CalledProcessError: + if io: + io.err("Editor exited with an error, aborting.\n") def is_annotation_missing(task: Task, annotation_content: str) -> bool: From 04d21c61fae8f7503d788ff67c29df1d3627a5fe Mon Sep 17 00:00:00 2001 From: Marty Oehme Date: Sat, 29 Nov 2025 18:45:28 +0100 Subject: [PATCH 7/7] test: Add simple happypath annotation adding --- test/test_cli.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/test/test_cli.py b/test/test_cli.py index 1d9601b..0bca9e7 100644 --- a/test/test_cli.py +++ b/test/test_cli.py @@ -11,9 +11,7 @@ def test_open_editor_escapes_shell(): run_mock.assert_called_once_with(["vim", "my note$1.txt"], check=True) -# -# def test_add_annotation_saves_task(): -# task = Mock() -# add_annotation(task, "hello") -# task.add_annotation.assert_called_once_with("hello") -# task.save.assert_called_once() +def test_add_annotation_calls_tasklib(): + task = Mock() + add_annotation(task, "hello") + task.add_annotation.assert_called_once_with("hello")