Skip to content

ci: run strict-no-cover in scripts/test to catch stale pragmas locally #2305

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

Merged
maxisbey merged 3 commits into from
Mar 17, 2026
+19 −3
Merged

ci: run strict-no-cover in scripts/test to catch stale pragmas locally #2305

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
166f649
ci: run strict-no-cover in scripts/test to catch stale pragmas locally
maxisbey Mar 17, 2026
6165b2e
revert: undo session.py pragma change (speculative, bad evidence)
maxisbey Mar 17, 2026
daf837c
docs: tighten coverage-pragma guidance
maxisbey Mar 17, 2026
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
19 changes: 16 additions & 3 deletions CLAUDE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,18 +29,31 @@ This document contains critical information about working with this codebase. Fo
- IMPORTANT: The `tests/client/test_client.py` is the most well designed test file. Follow its patterns.
- IMPORTANT: Be minimal, and focus on E2E tests: Use the `mcp.client.Client` whenever possible.
- Coverage: CI requires 100% (`fail_under = 100`, `branch = true`).
- Full check: `./scripts/test` (~20s, matches CI exactly)
- Targeted check while iterating:
- Full check: `./scripts/test` (~23s). Runs coverage + `strict-no-cover` on the
default Python. Not identical to CI: CI also runs 3.10–3.14 × {ubuntu, windows},
Comment on lines +32 to +33
Copy link

@claude claude bot Mar 17, 2026

Choose a reason for hiding this comment

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

🟡 The CI matrix description says "3.10–14 × {ubuntu, windows}" (10 entries), but .github/workflows/shared.yml defines a three-dimensional matrix: python-version × dep-resolution × os (5×2×2 = 20 entries), omitting the {locked, lowest-direct} dimension. Consider updating to "CI also runs 3.10–14 × {ubuntu, windows} × {locked, lowest-direct}" — the lowest-direct strategy is especially worth mentioning since those failures cannot be reproduced locally with --frozen.

Extended reasoning...

What the bug is

CLAUDE.md lines 32-33 state:

Not identical to CI: CI also runs 3.10–14 × {ubuntu, windows},
and some branch-coverage quirks only surface on specific matrix entries.

This implies the CI matrix has 5 Python versions × 2 OSes = 10 entries. However, the actual CI matrix in .github/workflows/shared.yml (lines 49-56) defines three dimensions:

matrix:
  python-version: ["3.10", "3.11", "3.12", "3.13", "3.14"]
  dep-resolution:
    - name: lowest-direct
      install-flags: "--upgrade --resolution lowest-direct"
    - name: locked
      install-flags: "--frozen"
  os: [ubuntu-latest, windows-latest]

That is 5 × 2 × 2 = 20 matrix entries, not 10.

Why the omission matters

The lowest-direct resolution strategy (--upgrade --resolution lowest-direct) tests compatibility with minimum acceptable dependency versions. This is a meaningfully different CI dimension because:

  1. A dependency feature used by MCP might not exist in the lowest-direct resolved version, causing a CI failure that cannot be reproduced locally where --frozen uses the lockfile (latest) versions.
  2. A developer seeing failures only on lowest-direct matrix entries would have no context from CLAUDE.md to understand why — the documentation only mentions Python versions and OS as CI dimensions.

Step-by-step proof

  1. Developer reads CLAUDE.md line 32-33: "CI also runs 3.10–14 × {ubuntu, windows}".
  2. Developer infers the CI matrix is 10 entries varying only by Python version and OS.
  3. Developer pushes a change that uses a dependency feature added in a recent version.
  4. CI fails on lowest-direct matrix entries where the older dependency version lacks that feature.
  5. Developer is confused because the failure does not correlate with any Python version or OS — the two dimensions CLAUDE.md told them about.

Impact

This is a minor documentation inaccuracy. The text already represents a significant improvement over the previous claim that ./scripts/test "matches CI exactly," and the sentence does correctly state that scripts/test is "Not identical to CI." The missing dimension is unlikely to cause real confusion in most cases, but including it would make the documentation precisely accurate and help developers diagnose lowest-direct-specific CI failures.

Suggested fix

Change line 33 from:

CI also runs 3.10–14 × {ubuntu, windows},

to:

CI also runs 3.10–14 × {ubuntu, windows} × {locked, lowest-direct},

and some branch-coverage quirks only surface on specific matrix entries.
- Targeted check while iterating (~4s, deterministic):

```bash
uv run --frozen coverage erase
uv run --frozen coverage run -m pytest tests/path/test_foo.py
uv run --frozen coverage combine
uv run --frozen coverage report --include='src/mcp/path/foo.py' --fail-under=0
UV_FROZEN=1 uv run --frozen strict-no-cover
```

Partial runs can't hit 100% (coverage tracks `tests/` too), so `--fail-under=0`
and `--include` scope the report to what you actually changed.
and `--include` scope the report. `strict-no-cover` has no false positives on
partial runs — if your new test executes a line marked `# pragma: no cover`,
even a single-file run catches it.
- Coverage pragmas:
- `# pragma: no cover` — line is never executed. CI's `strict-no-cover` fails if
it IS executed. When your test starts covering such a line, remove the pragma.
- `# pragma: lax no cover` — excluded from coverage but not checked by
`strict-no-cover`. Use for lines covered on some platforms/versions but not
others.
- `# pragma: no branch` — excludes branch arcs only. coverage.py misreports the
`->exit` arc for nested `async with` on Python 3.11+ (worse on 3.14/Windows).
- Avoid `anyio.sleep()` with a fixed duration to wait for async operations. Instead:
- Use `anyio.Event` — set it in the callback/handler, `await event.wait()` in the test
- For stream messages, use `await stream.receive()` instead of `sleep()` + `receive_nowait()`
Expand Down
3 changes: 3 additions & 0 deletions scripts/test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,6 @@ uv run --frozen coverage erase
uv run --frozen coverage run -m pytest -n auto $@
uv run --frozen coverage combine
uv run --frozen coverage report
# strict-no-cover spawns `uv run coverage json` internally without --frozen;
# UV_FROZEN=1 propagates to that subprocess so it doesn't touch uv.lock.
UV_FROZEN=1 uv run --frozen strict-no-cover
Loading