Print error messages not stacktraces.

Add bcftools validation tests for error conditions.

Author: tomwhite

tests/test_bcftools_validation.py

@@ -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
@@ -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)
@@ -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:")

tests/test_cli.py

@@ -1,5 +1,4 @@
 import pathlib
-import re
 from unittest import mock
 
 import click.testing as ct
@@ -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
 
@@ -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"
@@ -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")
@@ -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
 
@@ -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):
@@ -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
@@ -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

vcztools/cli.py

@@ -1,6 +1,7 @@
 import contextlib
 import os
 import sys
+from functools import wraps
 
 import click
 
@@ -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:
+            raise click.ClickException(e) from e
+
+    return wrapper
+
+
 include = click.option(
     "-i", "--include", type=str, help="Filter expression to include variant sites."
 )
@@ -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")
@@ -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
@@ -177,6 +196,7 @@ def query(path, output, list_samples, format, include, exclude):
 )
 @include
 @exclude
+@handle_exception
 def view(
     path,
     output,