From deabc981d790fbb16495af4a57f6c14143c9795c Mon Sep 17 00:00:00 2001 From: Dimitri Merejkowsky Date: Mon, 9 Apr 2018 13:48:12 +0200 Subject: [PATCH 1/6] test_main.py: check error messages instead of exceptions types and attributes Tests using checking the type and attributes of the exceptions look like this: with pytest.raises(SomeError) as e: some_code() assert e.value.field == "expected value" and they belong in low-level tests In high-level tests such as `test_main()` we write assertions about the error messages instead. This makes sure there are no bugs in the implementation of the `print_error()` method. This matters because if there is a bug in a `print_error()` method, users will get a confusing "double" stacktrace: ("During handling of the above exception, another exception occurred".) --- tbump/test/conftest.py | 3 +++ tbump/test/test_main.py | 45 +++++++++++++++++++++-------------------- 2 files changed, 26 insertions(+), 22 deletions(-) diff --git a/tbump/test/conftest.py b/tbump/test/conftest.py index 422592b..27a54aa 100644 --- a/tbump/test/conftest.py +++ b/tbump/test/conftest.py @@ -5,6 +5,9 @@ import tbump.git +from ui.tests.conftest import message_recorder +message_recorder # silence pyflakes + @pytest.fixture() def tmp_path(tmpdir): diff --git a/tbump/test/test_main.py b/tbump/test/test_main.py index 1a8bcb4..06b7b70 100644 --- a/tbump/test/test_main.py +++ b/tbump/test/test_main.py @@ -8,7 +8,7 @@ def test_replaces(test_repo): - tbump.main.run(["-C", test_repo, "1.2.41-alpha-2", "--non-interactive"]) + tbump.main.main(["-C", test_repo, "1.2.41-alpha-2", "--non-interactive"]) toml_path = test_repo.joinpath("tbump.toml") new_toml = toml.loads(toml_path.text()) @@ -19,23 +19,23 @@ def test_replaces(test_repo): assert_in_file("pub.js", "PUBLIC_VERSION = '1.2.41'") -def test_new_version_does_not_match(test_repo): - with pytest.raises(tbump.file_bumper.InvalidVersion) as e: - tbump.main.run(["-C", test_repo, "1.2.41a2", "--non-interactive"]) - assert e.value.version == "1.2.41a2" +def test_new_version_does_not_match(test_repo, message_recorder): + with pytest.raises(SystemExit): + tbump.main.main(["-C", test_repo, "1.2.41a2", "--non-interactive"]) + assert message_recorder.find("Could not parse 1.2.41a2") -def test_abort_if_file_does_not_exist(test_repo): +def test_abort_if_file_does_not_exist(test_repo, message_recorder): test_repo.joinpath("package.json").remove() tbump.git.run_git(test_repo, "add", "--update") tbump.git.run_git(test_repo, "commit", "--message", "remove package.json") - with pytest.raises(tbump.file_bumper.SourceFileNotFound) as e: - tbump.main.run(["-C", test_repo, "1.2.41-alpha-2", "--non-interactive"]) - assert e.value.src == "package.json" + with pytest.raises(SystemExit): + tbump.main.main(["-C", test_repo, "1.2.41-alpha-2", "--non-interactive"]) + assert message_recorder.find("package.json does not exist") def test_commit_and_tag(test_repo): - tbump.main.run(["-C", test_repo, "1.2.41-alpha-2", "--non-interactive"]) + tbump.main.main(["-C", test_repo, "1.2.41-alpha-2", "--non-interactive"]) _, out = tbump.git.run_git_captured(test_repo, "log", "--oneline") assert "Bump to 1.2.41-alpha-2" in out @@ -44,22 +44,23 @@ def test_commit_and_tag(test_repo): assert out == "v1.2.41-alpha-2" -def test_abort_if_dirty(test_repo): +def test_abort_if_dirty(test_repo, message_recorder): test_repo.joinpath("VERSION").write_text("unstaged changes\n", append=True) - with pytest.raises(tbump.git_bumper.DirtyRepository): - tbump.main.run(["-C", test_repo, "1.2.41-alpha-2", "--non-interactive"]) + with pytest.raises(SystemExit): + tbump.main.main(["-C", test_repo, "1.2.41-alpha-2", "--non-interactive"]) + assert message_recorder.find("dirty") -def test_abort_if_tag_exists(test_repo): +def test_abort_if_tag_exists(test_repo, message_recorder): tbump.git.run_git(test_repo, "tag", "v1.2.42") - with pytest.raises(tbump.git_bumper.RefAlreadyExists) as e: - tbump.main.run(["-C", test_repo, "1.2.42", "--non-interactive"]) - assert e.value.ref == "v1.2.42" + with pytest.raises(SystemExit): + tbump.main.main(["-C", test_repo, "1.2.42", "--non-interactive"]) + assert message_recorder.find("1.2.42 already exists") -def test_abort_if_file_does_not_match(test_repo): +def test_abort_if_file_does_not_match(test_repo, message_recorder): invalid_src = test_repo.joinpath("foo.txt") invalid_src.write_text("this is foo") tbump_path = test_repo.joinpath("tbump.toml") @@ -70,11 +71,11 @@ def test_abort_if_file_does_not_match(test_repo): tbump.git.run_git(test_repo, "add", ".") tbump.git.run_git(test_repo, "commit", "--message", "add foo.txt") - with pytest.raises(tbump.file_bumper.OldVersionNotFound) as e: - tbump.main.run(["-C", test_repo, "1.2.42", "--non-interactive"]) - assert e.value.sources == ["foo.txt"] + with pytest.raises(SystemExit): + tbump.main.main(["-C", test_repo, "1.2.42", "--non-interactive"]) + assert message_recorder.find("did not match") + assert message_recorder.find("foo\.txt") assert_in_file("VERSION", "1.2.41-alpha-1") - assert e.value.sources == ["foo.txt"] def test_no_tracked_branch__interactive__ask_to_skip_push(test_repo, mock): From d9780e47ce96b6f1d9fef3c537d12d5f9e0bfd0d Mon Sep 17 00:00:00 2001 From: Dimitri Merejkowsky Date: Mon, 9 Apr 2018 13:48:33 +0200 Subject: [PATCH 2/6] Fix crash in OldVersionNotFound.print_error() --- tbump/file_bumper.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tbump/file_bumper.py b/tbump/file_bumper.py index cb1ce73..6b9ce0a 100644 --- a/tbump/file_bumper.py +++ b/tbump/file_bumper.py @@ -51,7 +51,7 @@ def print_error(self): class OldVersionNotFound(tbump.Error): def print_error(self): message = [" Some files did not match the old version string\n"] - for src in self.src: + for src in self.sources: message.extend([ui.reset, " * ", ui.bold, src, "\n"]) ui.error(*message, sep="", end="") From c6b3ef5f4311a5e54056e204b36e2d0bdbbd0bc8 Mon Sep 17 00:00:00 2001 From: Dimitri Merejkowsky Date: Mon, 9 Apr 2018 14:44:38 +0200 Subject: [PATCH 3/6] Update Changelog --- Changelog.rst | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Changelog.rst b/Changelog.rst index 3cfc463..4c1060e 100644 --- a/Changelog.rst +++ b/Changelog.rst @@ -1,6 +1,10 @@ Changelog ========= +v1.0.2 +------ + +* Fix printing a big ugly stacktrace when the looking for the old version number failed for one or more files. v1.0.1 ------ From 1dffee6ad489944563836120655d7fa6dc17add4 Mon Sep 17 00:00:00 2001 From: Dimitri Merejkowsky Date: Mon, 9 Apr 2018 15:39:55 +0200 Subject: [PATCH 4/6] Make sure to restore working dir at the end of the test session We change the working dir for every test that run tbump with `-C`, and we need to restore the working dir before writing for instance the html coverage output. --- tbump/test/conftest.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tbump/test/conftest.py b/tbump/test/conftest.py index 27a54aa..4c2365b 100644 --- a/tbump/test/conftest.py +++ b/tbump/test/conftest.py @@ -1,4 +1,5 @@ import re +import os import path import pytest @@ -38,6 +39,13 @@ def complex_version_regex(): return re.compile(pattern, re.VERBOSE) +@pytest.fixture(autouse=True, scope="session") +def restore_cwd(): + old_cwd = os.getcwd() + yield + os.chdir(old_cwd) + + def assert_in_file(*args): parts = args[0:-1] first = parts[0] From 49650195175624c281ced83545bfbf7c3679e999 Mon Sep 17 00:00:00 2001 From: Dimitri Merejkowsky Date: Mon, 9 Apr 2018 15:37:20 +0200 Subject: [PATCH 5/6] Remove dead code --- tbump/main.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/tbump/main.py b/tbump/main.py index 87fecdf..705919b 100644 --- a/tbump/main.py +++ b/tbump/main.py @@ -72,10 +72,6 @@ def parse_config(self): raise InvalidConfig(parse_error=parse_error) return config - def handle_working_dir(args): - if args.working_dir: - os.chdir(args.working_dir) - def setup_git_bumper(self): git_bumper = GitBumper(self.working_path) git_bumper.set_config(self.config) From be665d68db5da209def9e228b94e811d4ea23070 Mon Sep 17 00:00:00 2001 From: Dimitri Merejkowsky Date: Mon, 9 Apr 2018 15:08:27 +0200 Subject: [PATCH 6/6] Increase coverage and fix a few more bugs --- tbump/file_bumper.py | 2 +- tbump/main.py | 9 ++++-- tbump/test/test_git_bumper.py | 26 ++++++++++----- tbump/test/test_main.py | 60 ++++++++++++++++++++++++++++++----- 4 files changed, 78 insertions(+), 19 deletions(-) diff --git a/tbump/file_bumper.py b/tbump/file_bumper.py index 6b9ce0a..ebc2a46 100644 --- a/tbump/file_bumper.py +++ b/tbump/file_bumper.py @@ -64,7 +64,7 @@ def should_replace(line, old_string, search=None): def on_version_containing_none(src, verb, version, *, groups, template): - raise BadSubstitution(src=src, version=version, groups=groups, template=template) + raise BadSubstitution(src=src, verb=verb, version=version, groups=groups, template=template) def find_replacements(file_path, old_string, new_string, search=None): diff --git a/tbump/main.py b/tbump/main.py index 705919b..284bbf9 100644 --- a/tbump/main.py +++ b/tbump/main.py @@ -16,6 +16,10 @@ class InvalidConfig(tbump.Error): + def __init__(self, io_error=None, parse_error=None): + self.io_error = io_error + self.parse_error = parse_error + def print_error(self): if self.io_error: ui.error("Could not read config file:", self.io_error) @@ -24,7 +28,8 @@ def print_error(self): class Cancelled(tbump.Error): - pass + def print_error(self): + ui.error("Cancelled by user") def main(args=None): @@ -120,7 +125,7 @@ def check(self): self.tracked_branch = False proceed = ui.ask_yes_no("Continue anyway?", default=False) if not proceed: - raise Cancelled() + raise Cancelled from None def push(self): if self.dry_run: diff --git a/tbump/test/test_git_bumper.py b/tbump/test/test_git_bumper.py index ea30a2e..1398071 100644 --- a/tbump/test/test_git_bumper.py +++ b/tbump/test/test_git_bumper.py @@ -5,22 +5,32 @@ import pytest -def test_git_bumper_happy_path(test_repo): +@pytest.fixture +def test_git_bumper(test_repo): config = tbump.config.parse(test_repo.joinpath("tbump.toml")) git_bumper = tbump.git_bumper.GitBumper(test_repo) git_bumper.set_config(config) - git_bumper.check_state("1.2.42") + return git_bumper + + +def test_git_bumper_happy_path(test_repo, test_git_bumper): + test_git_bumper.check_state("1.2.42") test_repo.joinpath("VERSION").write_text("1.2.42") - git_bumper.bump("1.2.42") + test_git_bumper.bump("1.2.42") _, out = tbump.git.run_git_captured(test_repo, "log", "--oneline") assert "Bump to 1.2.42" in out -def test_git_bumper_no_tracking_ref(test_repo): - config = tbump.config.parse(test_repo.joinpath("tbump.toml")) - git_bumper = tbump.git_bumper.GitBumper(test_repo) - git_bumper.set_config(config) +def test_git_bumper_no_tracking_ref(test_repo, test_git_bumper): tbump.git.run_git(test_repo, "checkout", "-b", "devel") with pytest.raises(tbump.git_bumper.NoTrackedBranch): - git_bumper.check_state("1.2.42") + test_git_bumper.check_state("1.2.42") + + +def test_not_on_any_branch(test_repo, test_git_bumper): + tbump.git.run_git(test_repo, "commit", "--message", "test", "--allow-empty") + tbump.git.run_git(test_repo, "checkout", "HEAD~1") + + with pytest.raises(tbump.git_bumper.NotOnAnyBranch): + test_git_bumper.check_state("1.2.42") diff --git a/tbump/test/test_main.py b/tbump/test/test_main.py index 06b7b70..619637e 100644 --- a/tbump/test/test_main.py +++ b/tbump/test/test_main.py @@ -19,6 +19,24 @@ def test_replaces(test_repo): assert_in_file("pub.js", "PUBLIC_VERSION = '1.2.41'") +def test_tbump_toml_not_found(test_repo, message_recorder): + toml_path = test_repo.joinpath("tbump.toml") + toml_path.remove() + with pytest.raises(SystemExit): + tbump.main.main(["-C", test_repo, "1.2.42", "--non-interactive"]) + assert message_recorder.find("No such file") + + +def test_tbump_toml_bad_syntax(test_repo, message_recorder): + toml_path = test_repo.joinpath("tbump.toml") + bad_toml = toml.loads(toml_path.text()) + del bad_toml["git"] + toml_path.write_text(toml.dumps(bad_toml)) + with pytest.raises(SystemExit): + tbump.main.main(["-C", test_repo, "1.2.42", "--non-interactive"]) + assert message_recorder.find("Missing keys") + + def test_new_version_does_not_match(test_repo, message_recorder): with pytest.raises(SystemExit): tbump.main.main(["-C", test_repo, "1.2.41a2", "--non-interactive"]) @@ -78,28 +96,42 @@ def test_abort_if_file_does_not_match(test_repo, message_recorder): assert_in_file("VERSION", "1.2.41-alpha-1") -def test_no_tracked_branch__interactive__ask_to_skip_push(test_repo, mock): +def test_no_tracked_branch_proceed_and_skip_push(test_repo, mock): ask_mock = mock.patch("ui.ask_yes_no") ask_mock.return_value = True tbump.git.run_git(test_repo, "checkout", "-b", "devel") - tbump.main.run(["-C", test_repo, "1.2.42"]) + tbump.main.main(["-C", test_repo, "1.2.42"]) ask_mock.assert_called_with("Continue anyway?", default=False) assert_in_file("VERSION", "1.2.42") -def test_no_tracked_branch__non_interactive__abort(test_repo, mock): +def test_no_tracked_branch_cancel(test_repo, mock, message_recorder): + ask_mock = mock.patch("ui.ask_yes_no") + ask_mock.return_value = False + tbump.git.run_git(test_repo, "checkout", "-b", "devel") + + with pytest.raises(SystemExit): + tbump.main.main(["-C", test_repo, "1.2.42"]) + + ask_mock.assert_called_with("Continue anyway?", default=False) + assert_in_file("VERSION", "1.2.41") + assert message_recorder.find("Cancelled") + + +def test_no_tracked_branch_non_interactive(test_repo, message_recorder): tbump.git.run_git(test_repo, "checkout", "-b", "devel") - with pytest.raises(tbump.git_bumper.NoTrackedBranch): - tbump.main.run(["-C", test_repo, "1.2.42", "--non-interactive"]) + with pytest.raises(SystemExit): + tbump.main.main(["-C", test_repo, "1.2.42", "--non-interactive"]) + assert message_recorder.find("Cannot push") def test_interactive_push(test_repo, mock): ask_mock = mock.patch("ui.ask_yes_no") ask_mock.return_value = True - tbump.main.run(["-C", test_repo, "1.2.42"]) + tbump.main.main(["-C", test_repo, "1.2.42"]) ask_mock.assert_called_with("OK to push", default=False) _, out = tbump.git.run_git_captured(test_repo, "ls-remote") assert "tags/v1.2.42" in out @@ -107,11 +139,23 @@ def test_interactive_push(test_repo, mock): def test_do_not_add_untracked_files(test_repo): test_repo.joinpath("untracked.txt").write_text("please don't add me") - tbump.main.run(["-C", test_repo, "1.2.42", "--non-interactive"]) + tbump.main.main(["-C", test_repo, "1.2.42", "--non-interactive"]) _, out = tbump.git.run_git_captured(test_repo, "show", "--stat", "HEAD") assert "untracked.txt" not in out def test_dry_run(test_repo): - tbump.main.run(["-C", test_repo, "1.2.41-alpha-2", "--dry-run"]) + tbump.main.main(["-C", test_repo, "1.2.41-alpha-2", "--dry-run"]) assert_in_file("VERSION", "1.2.41-alpha-1") + + +def test_bad_subsitiution(test_repo, message_recorder): + toml_path = test_repo.joinpath("tbump.toml") + new_toml = toml.loads(toml_path.text()) + new_toml["file"][0]["version_template"] = "{release}" + toml_path.write_text(toml.dumps(new_toml)) + tbump.git.run_git(test_repo, "add", ".") + tbump.git.run_git(test_repo, "commit", "--message", "update repo") + with pytest.raises(SystemExit): + tbump.main.main(["-C", test_repo, "1.2.42", "--dry-run"]) + assert message_recorder.find("refusing to replace by version containing 'None'")