Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Print error messages not stacktraces #140

Merged
merged 2 commits into from
Feb 20, 2025
Merged

Print error messages not stacktraces #140

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c2fae19
Print error messages not stacktraces.
tomwhite Feb 19, 2025
2dbce16
Treat ValueError as known user errors
tomwhite Feb 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 41 additions & 12 deletions tests/test_bcftools_validation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,24 +9,34 @@
from .utils import assert_vcfs_close, vcz_path_cache


def run_bcftools(args: str) -> str:
"""Run bcftools (which must be on the PATH) and return stdout as a string."""
def run_bcftools(args: str, expect_error=False) -> tuple[str, str]:
"""
Run bcftools (which must be on the PATH) and return stdout and stderr
as a pair of strings.
"""
completed = subprocess.run(
f"bcftools {args}", capture_output=True, check=True, shell=True
f"bcftools {args}", capture_output=True, check=False, shell=True
)
return completed.stdout.decode("utf-8")
if expect_error:
assert completed.returncode != 0
else:
assert completed.returncode == 0
return completed.stdout.decode("utf-8"), completed.stderr.decode("utf-8")


def run_vcztools(args: str) -> str:
"""Run run_vcztools and return stdout as a string."""
def run_vcztools(args: str, expect_error=False) -> tuple[str, str]:
"""Run run_vcztools and return stdout and stderr as a pair of strings."""
runner = ct.CliRunner(mix_stderr=False)
result = runner.invoke(
cli.vcztools_main,
args,
catch_exceptions=False,
)
assert result.exit_code == 0
return result.stdout
if expect_error:
assert result.exit_code != 0
else:
assert result.exit_code == 0
return result.stdout, result.stderr


# fmt: off
Expand Down Expand Up @@ -76,12 +86,12 @@ def test_vcf_output(tmp_path, args, vcf_file):
original = pathlib.Path("tests/data/vcf") / vcf_file
vcz = vcz_path_cache(original)

bcftools_out = run_bcftools(f"{args} {original}")
bcftools_out, _ = run_bcftools(f"{args} {original}")
bcftools_out_file = tmp_path.joinpath("bcftools_out.vcf")
with open(bcftools_out_file, "w") as f:
f.write(bcftools_out)

vcztools_out = run_vcztools(f"{args} {vcz}")
vcztools_out, _ = run_vcztools(f"{args} {vcz}")
vcztools_out_file = tmp_path.joinpath("vcztools_out.vcf")
with open(vcztools_out_file, "w") as f:
f.write(vcztools_out)
Expand Down Expand Up @@ -143,7 +153,26 @@ def test_output(tmp_path, args, vcf_name):
vcf_path = pathlib.Path("tests/data/vcf") / vcf_name
vcz_path = vcz_path_cache(vcf_path)

bcftools_output = run_bcftools(f"{args} {vcf_path}")
vcztools_output = run_vcztools(f"{args} {vcz_path}")
bcftools_output, _ = run_bcftools(f"{args} {vcf_path}")
vcztools_output, _ = run_vcztools(f"{args} {vcz_path}")

assert vcztools_output == bcftools_output


@pytest.mark.parametrize(
("args", "vcf_name"),
[
("index -ns", "sample.vcf.gz"),
("query -f '%POS\n' -i 'INFO/DP > 10' -e 'INFO/DP < 50'", "sample.vcf.gz"),
("view -i 'INFO/DP > 10' -e 'INFO/DP < 50'", "sample.vcf.gz"),
],
)
def test_error(tmp_path, args, vcf_name):
vcf_path = pathlib.Path("tests/data/vcf") / vcf_name
vcz_path = vcz_path_cache(vcf_path)

_, bcftools_error = run_bcftools(f"{args} {vcf_path}", expect_error=True)
assert bcftools_error.startswith("Error:") or bcftools_error.startswith("[E::")

_, vcztools_error = run_vcztools(f"{args} {vcz_path}", expect_error=True)
assert vcztools_error.startswith("Error:")
55 changes: 30 additions & 25 deletions tests/test_cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import pathlib
import re
from unittest import mock

import click.testing as ct
Expand All @@ -17,7 +16,7 @@ def vcz_path():


def test_version_header(vcz_path):
output = run_vcztools(f"view {vcz_path}")
output, _ = run_vcztools(f"view {vcz_path}")
assert output.find("##vcztools_viewCommand=") >= 0
assert output.find("Date=") >= 0

Expand All @@ -26,20 +25,22 @@ class TestOutput:
def test_view_unsupported_output(self, tmp_path, vcz_path):
bad_output = tmp_path / "output.vcf.gz"

with pytest.raises(
ValueError,
match=re.escape(
"Only uncompressed VCF output supported, suffix .gz not allowed"
),
):
run_vcztools(f"view --no-version {vcz_path} -o {bad_output}")
_, vcztools_error = run_vcztools(
f"view --no-version {vcz_path} -o {bad_output}", expect_error=True
)
assert (
"Only uncompressed VCF output supported, suffix .gz not allowed"
in vcztools_error
)

@pytest.mark.parametrize("suffix", ["gz", "bgz", "bcf"])
def test_view_unsupported_output_suffix(self, tmp_path, vcz_path, suffix):
bad_output = tmp_path / f"output.vcf.{suffix}"

with pytest.raises(ValueError, match=f".{suffix} not allowed"):
run_vcztools(f"view --no-version {vcz_path} -o {bad_output}")
_, vcztools_error = run_vcztools(
f"view --no-version {vcz_path} -o {bad_output}", expect_error=True
)
assert f".{suffix} not allowed" in vcztools_error

def test_view_good_path(self, tmp_path, vcz_path):
output_path = tmp_path / "tmp.vcf"
Expand Down Expand Up @@ -78,12 +79,16 @@ def test_view_write_pipe(self, tmp_path, vcz_path):

def test_excluding_and_including_samples(vcz_path):
samples_file_path = pathlib.Path("tests/data/txt/samples.txt")
error_message = re.escape("vcztools does not support combining -s and -S")
error_message = "vcztools does not support combining -s and -S"

with pytest.raises(AssertionError, match=error_message):
run_vcztools(f"view {vcz_path} -s NA00001 -S ^{samples_file_path}")
with pytest.raises(AssertionError, match=error_message):
run_vcztools(f"view {vcz_path} -s ^NA00001 -S {samples_file_path}")
_, vcztools_error = run_vcztools(
f"view {vcz_path} -s NA00001 -S ^{samples_file_path}", expect_error=True
)
assert error_message in vcztools_error
_, vcztools_error = run_vcztools(
f"view {vcz_path} -s ^NA00001 -S {samples_file_path}", expect_error=True
)
assert error_message in vcztools_error


@mock.patch("sys.exit")
Expand All @@ -104,7 +109,7 @@ def test_format_required(self, vcz_path):
f"query {vcz_path} ",
catch_exceptions=False,
)
assert result.exit_code == 2
assert result.exit_code != 0
assert len(result.stdout) == 0
assert len(result.stderr) > 0

Expand All @@ -115,34 +120,34 @@ def test_path_required(self):
"query --format=POS ",
catch_exceptions=False,
)
assert result.exit_code == 2
assert result.exit_code != 0
assert len(result.stdout) == 0
assert len(result.stderr) > 0

def test_list(self, vcz_path):
result = run_vcztools(f"query -l {vcz_path}")
result, _ = run_vcztools(f"query -l {vcz_path}")
assert list(result.splitlines()) == ["NA00001", "NA00002", "NA00003"]

def test_list_ignores_output(self, vcz_path, tmp_path):
output = tmp_path / "tmp.txt"
result = run_vcztools(f"query -l {vcz_path} -o {output}")
result, _ = run_vcztools(f"query -l {vcz_path} -o {output}")
assert list(result.splitlines()) == ["NA00001", "NA00002", "NA00003"]
assert not output.exists()

def test_output(self, vcz_path, tmp_path):
output = tmp_path / "tmp.txt"
result = run_vcztools(f"query -f '%POS\n' {vcz_path} -o {output}")
result, _ = run_vcztools(f"query -f '%POS\n' {vcz_path} -o {output}")
assert list(result.splitlines()) == []
assert output.exists()


class TestIndex:
def test_stats(self, vcz_path):
result = run_vcztools(f"index -s {vcz_path}")
result, _ = run_vcztools(f"index -s {vcz_path}")
assert list(result.splitlines()) == ["19\t.\t2", "20\t.\t6", "X\t.\t1"]

def test_nrecords(self, vcz_path):
result = run_vcztools(f"index -n {vcz_path}")
result, _ = run_vcztools(f"index -n {vcz_path}")
assert list(result.splitlines()) == ["9"]

def test_stats_and_nrecords(self, vcz_path):
Expand All @@ -152,7 +157,7 @@ def test_stats_and_nrecords(self, vcz_path):
f"index -ns {vcz_path}",
catch_exceptions=False,
)
assert result.exit_code == 2
assert result.exit_code != 0
assert len(result.stdout) == 0
assert len(result.stderr) > 0
assert "Expected only one of --stats or --nrecords options" in result.stderr
Expand All @@ -164,7 +169,7 @@ def test_no_stats_or_nrecords(self, vcz_path):
f"index {vcz_path}",
catch_exceptions=False,
)
assert result.exit_code == 2
assert result.exit_code != 0
assert len(result.stdout) == 0
assert len(result.stderr) > 0
assert "Error: Building region indexes is not supported" in result.stderr
Expand Down
20 changes: 20 additions & 0 deletions vcztools/cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import contextlib
import os
import sys
from functools import wraps

import click

Expand Down Expand Up @@ -28,6 +29,22 @@ def handle_broken_pipe(output):
sys.exit(1) # Python exits with error code 1 on EPIPE


def handle_exception(func):
"""
Handle application exceptions by converting to a ClickException,
so the message is written to stderr and a non-zero exit code is set.
"""

@wraps(func)
def wrapper(*args, **kwargs):
try:
return func(*args, **kwargs)
except Exception as e:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good approach, but I think we should stack trace when unexpected exceptions happen. Perhaps we could catch ValueErrors here instead, which would probably get most of the actual interface errors we expect?

In particular, we should check to see if it's already a ClickException and just reraise if so. Otherwise we're losing a bunch of subtlety.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also - nice usage of functools.wraps!!!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good approach, but I think we should stack trace when unexpected exceptions happen. Perhaps we could catch ValueErrors here instead, which would probably get most of the actual interface errors we expect?

Done.

raise click.ClickException(e) from e

return wrapper


include = click.option(
"-i", "--include", type=str, help="Filter expression to include variant sites."
)
Expand Down Expand Up @@ -66,6 +83,7 @@ def list_commands(self, ctx):
is_flag=True,
help="Print per contig stats.",
)
@handle_exception
def index(path, nrecords, stats):
if nrecords and stats:
raise click.UsageError("Expected only one of --stats or --nrecords options")
Expand Down Expand Up @@ -95,6 +113,7 @@ def index(path, nrecords, stats):
)
@include
@exclude
@handle_exception
def query(path, output, list_samples, format, include, exclude):
if list_samples:
# bcftools query -l ignores the --output option and always writes to stdout
Expand Down Expand Up @@ -177,6 +196,7 @@ def query(path, output, list_samples, format, include, exclude):
)
@include
@exclude
@handle_exception
def view(
path,
output,
Expand Down
Loading