Mandate 99% coverage for the tests module

[medley56]

Summary

Closes #202.

The ask is a codecov status that holds tests/** to a very high bar. That status is one small block of YAML, but adding it on its own would have been permanently red on every commit (only_pulls: false): coverage of tests/ on main today is 96.59% — 64 uncovered statements out of 1876. So the bulk of this PR is closing that gap, and the codecov change is the last step.

Before: 96.59% (64 / 1876 uncovered). After: 99.89% (2 / 1881 uncovered).

Measured with coverage report --include="tests/*" --precision=2 on a single Python 3.13 environment. The full suite goes from 481 to 495 passing tests. Library coverage also ticked up as a side effect (92.04% → 92.35%), and the combined number codecov sees went 94% → 95.75%.

No coverage suppression of any kind was used. No # pragma: no cover, no [tool.coverage.report] exclude_lines in pyproject.toml, no tests/** entry in codecov's ignore: list, and no test was deleted to raise a percentage. This is per @greglucas's request on the previous attempt (#203): "Please add tests to get the coverage to 100%. You shouldn't add any no-pragma comments to ignore coverage on specific lines. I want those lines to get test parameterizations added to increase the coverage, not ignore the current coverage." pyproject.toml is untouched.

1. 17 of the 64 lines were a pytest-benchmark tracer artifact, not missing tests

This is the least obvious part of the diff, so it's worth spelling out.

All three files under tests/benchmark/ reported missing lines — every statement after a benchmark(...) call in a test body, plus the helper functions that are only invoked from inside the timed loop. Those statements plainly do execute; the tests pass and assert on their results.

The cause is in pytest_benchmark/fixture.py. Around the timed loop the fixture does:

self.prev_tracer = sys.gettrace()
sys.settrace(None)
...   # timed loop runs here
sys.settrace(self.prev_tracer)

Disabling tracing for the measurement is deliberate — a tracer would dominate the timing. The problem is the restore. sys.settrace() sets the global trace function, which CPython consults only when a frame is created. Frames that are already on the stack carry their own f_trace, and that was cleared by settrace(None). Restoring the global hook therefore cannot re-arm the already-executing test function's frame. Coverage silently loses the remainder of every benchmark test body.

Fix, in .github/workflows/ci.yml:

pytest --color=yes --cov --cov-report=xml --benchmark-disable

With --benchmark-disable, pytest-benchmark still calls each benchmarked function exactly once (so the tests still genuinely exercise the library) but skips the timed loop and its settrace dance. All three benchmark files go to 100%. Nothing consumes CI benchmark timings — numbers from shared GitHub runners are noise — and local pytest runs are unaffected, so developers still get timings by default. A comment in the workflow explains the flag so it doesn't get "cleaned up" later.

COVERAGE_CORE=sysmon also fixes it (sys.monitoring is immune to settrace(None)), but it is unavailable on Python 3.10/3.11, which are in the CI matrix, so --benchmark-disable is the portable choice.

2. Thirteen error parametrizations, every exception type observed rather than guessed

Thirteen parametrized tests carry an if isinstance(expectation, Exception): branch that no case ever took. The pattern for filling them in already existed in-repo (test_encodings.py passes a bare AttributeError() / ValueError() as the expectation value), so these copy it.

The previous attempt's main failure mode was asserting exception types that the code did not actually raise. For every case below I wrote the XML, executed it, read the real exception, and only then chose the assertion. Most turned out to be meaningful, typed library errors rather than incidental ones:

Test	New case	Observed exception
test_string_parameter_type	StringParameterType with an IntegerDataEncoding	ValueError: StringParameterType may only be instantiated with a StringDataEncoding encoding.
test_integer_parameter_type	no data encoding element	ValueError: No Data Encoding element found for Parameter Type ...
test_float_parameter_type	no data encoding element	ValueError: No Data Encoding element found ...
test_enumerated_parameter_type	no EnumerationList	ValueError: An EnumeratedParameterType must contain an EnumerationList.
test_binary_parameter_type	BinaryParameterType with an IntegerDataEncoding	ValueError: BinaryParameterType may only be instantiated with a BinaryDataEncoding encoding.
test_boolean_parameter_type	no data encoding element	ValueError: No Data Encoding element found ...
test_absolute_time_parameter_type	no data encoding element	ValueError: No Data Encoding element found ...
test_spline_calibrator	order="2"	NotImplementedError: Spline calibrators of order > 1 are not implemented.
test_integer_data_encoding	encoding="notARealEncoding"	ValueError: Encoding must be one of ('unsigned', 'signed', 'twosCompliment', 'twosComplement')
test_binary_data_encoding	SizeInBits containing an unrecognized tag	ValueError: Tried parsing a binary parameter length using Fixed, Dynamic, and DiscreteLookupList but failed.
test_boolean_expression	ComparisonOperator of =!=	ValueError: Unrecognized operator syntax =!=. Must be one of {...}

Two cases have no meaningful library-level error reachable, and I'm flagging them explicitly rather than pretending otherwise:

• test_polynomial_calibrator — a <Term> with coefficient="not-a-number" raises ValueError straight out of the builtin float(), not from a library check. Every other malformed-Term shape (e.g. a missing coefficient attribute) raises a bare KeyError. I picked the ValueError because it is the least brittle of the available options, but it is an incidental exception type, not a designed one.
• test_polynomial_calibrator_calibrate — calibrating a non-numeric query point raises TypeError: unsupported operand type(s) for ** or pow(). Again incidental: it comes from the ** operator, not from input validation.

If the maintainers would rather these raised a typed space_packet_parser.exceptions error, that is a library behavior change and belongs in its own ticket; I deliberately kept it out of scope here. The tests assert only the exception type (pytest.raises(type(expectation)), which is what the existing branch does), not the message, so they will not fight a future improvement.

3. Dead fixture code in tests/conftest.py (14 lines)

This is exactly the defect class #202 is about.

• clarreo_test_data_dir — deleted. Zero usages anywhere in the repo, and tests/test_data/clarreo/ does not exist, so the fixture could not have worked if anything had requested it.
• mock_schema_download — kept, and now actually exercised. Six tests request this fixture, but its inner mock_urlopen / MockResponse bodies never ran: since 6.2.0 bundles the OMG schema, documents referencing the standard schema URL resolve offline and no download is attempted. Rather than delete a useful network guard, I added test_schema_validation_downloads_non_bundled_schema, which validates a document whose xsi:schemaLocation points at a non-bundled path on the allowlisted www.omg.org host. That drives validation._download_schema end-to-end through the public validate_xtce API (the existing security tests only reach it through the private _load_schema), including the size cap, the XSD parse, and the write-to-cache-only-after-validation behavior. The cache directory is redirected to tmp_path so the download is genuinely attempted instead of being served from a previous run's cache. For the other six tests the fixture now documents itself as what it actually is: a guard that no network call happens.

4. Small cleanups (5 lines)

• test_definitions.py — the _flatten_container helper used if cond: raise AssertionError(msg) three times, so the raise statements only executed on failure. Rewritten as assert cond, msg. Identical semantics, more idiomatic in a test, and the statement now executes on the happy path.
• test_common.py — TestClass.ignored() existed to demonstrate that methods are excluded from AttrComparable comparison, but was never called. Now called with an assertion, which is a slightly stronger statement of intent: the method is callable and irrelevant to equality.
• test_validation_security.py — the _mock_urlopen_returning helper's read() had an if args: ... else: ... split, and nothing in the library ever calls response.read() without a size (_download_schema always passes MAX_SCHEMA_BYTES + 1), so the no-argument arm was unreachable. The helper now delegates to an io.BytesIO, which reproduces both behaviors of a real response body exactly with no branch of its own.

5. The codecov status

Only after all of the above does the new status group go in, under coverage.status.project:

      tests:
        target: 99% # Very high threshold for test files
        threshold: 1% # Allow only a 1% drop
        paths:
          - "tests/**"
        if_no_uploads: error
        only_pulls: false

This is additive — project.default (95% / 5%) still covers the whole repo including tests/. POSTing the file to https://codecov.io/validate returns Valid! and shows paths compiling to (?s:tests/.*)\Z, i.e. correct recursive matching. The validate-codecov-yaml pre-commit hook is in ci.skip because it needs network, so it was run locally; it passes.

At 99.89% against a 99% target with a 1% threshold, there is real headroom rather than a number that squeaks past.

Every remaining uncovered line

Two lines, both in tests/integration/test_xtce_based_parsing/test_suda_parsing.py — 2 of 1881 statements, 0.11%:

• Line 91 — the break that exits the packet-accumulation loop when the next scitype is 1, i.e. when a new event begins.
• Line 94 — p = p_next, the statement immediately after that loop, reachable only via the same break.

tests/test_data/suda/sciData_2022_130_17_41_53.spl contains a single complete event, so the loop always terminates by StopIteration at end-of-file and never by break. The test's own inline comment already says so: "For this example, we only have one full event so we have already hit a StopIteration by this point." Covering these would require fabricating a multi-event SUDA binary file, which would be inventing mission data to move a coverage number — not a real test. They are left uncovered deliberately, and 99.89% clears the 99% target with them.

No other line under tests/ is uncovered; the remaining 34 files are all at 100.00%.

Verification

• uv run pytest --cov --cov-report=term-missing --benchmark-disable → 495 passed.
• uv run pytest (no flags, i.e. what a developer runs locally, benchmarks enabled) → 495 passed. Local runs are unaffected by the CI flag.
• uv run coverage report --include="tests/*" --precision=2 → 1881 statements, 2 missed, 99.89%.
• uv run pre-commit run --all-files → all 12 hooks pass, including prettier, ruff-format, trim trailing whitespace, and the network-dependent validate codecov.yml.
• curl -X POST --data-binary @codecov.yml https://codecov.io/validate → Valid!.

Not done, on purpose

• No CHANGELOG entry. Nothing here is user-facing — it is CI configuration plus test-suite internals; no library behavior, API, or dependency changed. Consistent with precedent: Add local pre-commit Codecov YAML validator #269 (the codecov pre-commit validator) and Update README status badge to track CI workflow #272 (the CI status badge) added none either.
• No change to project.default (95%) or patch.default (85%), and no attempt to raise coverage of space_packet_parser/** — out of scope for this ticket.
• No [tool.coverage.run] source setting. Bare --cov already measures tests/; pinning a source list would risk quietly dropping test modules from the report.

🤖 Generated with Claude Code

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.75%. Comparing base (e6359c6) to head (be3ddd1).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #289      +/-   ##
==========================================
+ Coverage   94.49%   95.75%   +1.25%     
==========================================
  Files          49       49              
  Lines        4163     4168       +5     
==========================================
+ Hits         3934     3991      +57     
+ Misses        229      177      -52     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Copilot]

🟡 Changes recommended

Two moderate test-isolation and schema-mocking issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR enforces 99% coverage for tests/** and expands test coverage while disabling benchmarks during CI coverage runs.

Changes:

• Adds a dedicated Codecov status for tests/**.
• Adds error-path, validation, and fixture coverage.
• Disables benchmark timing during coverage collection.

File summaries

File	Summary
tests/unit/test_xtce/test_validation.py	Adds non-bundled schema download coverage; schema host policy should be explicit.
tests/unit/test_xtce/test_validation_security.py	Improves mocked response stream behavior.
tests/unit/test_xtce/test_parameter_types.py	Adds invalid parameter-type cases.
tests/unit/test_xtce/test_encodings.py	Adds invalid encoding cases.
tests/unit/test_xtce/test_definitions.py	Simplifies coverage assertions.
tests/unit/test_xtce/test_comparisons.py	Adds invalid operator coverage.
tests/unit/test_xtce/test_calibrators.py	Adds invalid calibrator cases.
tests/unit/test_common.py	Exercises the ignored method.
tests/conftest.py	Removes an unused fixture and updates schema mocking; the network guard should reject bundled URLs or assert no call.
codecov.yml	Adds the 99% tests coverage status.
.github/workflows/ci.yml	Disables benchmark timing during coverage collection.

Review details

Suppressed comments (1)

tests/unit/test_xtce/test_validation.py:88

• This test leaves allowed_schema_hosts unset, so _resolve_schema_policy will honor any SPP_ALLOWED_SCHEMA_HOSTS value from the process environment. A developer or CI environment that omits www.omg.org would make this otherwise valid test fail before the mocked download is reached; pass the intended allowlist explicitly (or clear the environment variable with monkeypatch) to keep the test hermetic.

        result = validate_xtce(io.StringIO(xtce_str), level="schema", raise_on_error=False)

• Files reviewed: 11/11 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.