Skip to content

Setup coverage (windows)#249

Merged
gvanrossum merged 2 commits intomicrosoft:mainfrom
bmerkle:setup-coverage
Apr 24, 2026
Merged

Setup coverage (windows)#249
gvanrossum merged 2 commits intomicrosoft:mainfrom
bmerkle:setup-coverage

Conversation

@bmerkle
Copy link
Copy Markdown
Collaborator

@bmerkle bmerkle commented Apr 24, 2026

This PR:
fixed setup for uv coverage under Windows

Copilot AI review requested due to automatic review settings April 24, 2026 15:26
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds Windows support for running test coverage via uv, aligning the developer workflow with the existing Unix make coverage target.

Changes:

  • Added a coverage command to make.bat that runs coverage.py via uv run.
  • Updated .coveragerc to measure coverage from src instead of test.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
make.bat Adds a coverage target and updates the usage string to include it.
.coveragerc Adjusts coverage source configuration to the src tree.

Comment thread make.bat
Comment on lines +50 to +53
uv run coverage erase
uv run coverage run -m pytest
uv run coverage combine
uv run coverage report
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

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

The Windows coverage target doesn't set COVERAGE_PROCESS_START. Some tests spawn the MCP server in a subprocess and only propagate COVERAGE_PROCESS_START if it’s already present, so coverage for subprocesses won’t be collected on Windows (unlike the Makefile target). Set COVERAGE_PROCESS_START=.coveragerc before coverage run (and optionally unset it afterwards) to match the Unix behavior.

Suggested change
uv run coverage erase
uv run coverage run -m pytest
uv run coverage combine
uv run coverage report
uv run coverage erase
set "COVERAGE_PROCESS_START=.coveragerc"
uv run coverage run -m pytest
uv run coverage combine
uv run coverage report
set "COVERAGE_PROCESS_START="

Copilot uses AI. Check for mistakes.
@gvanrossum gvanrossum merged commit 016d614 into microsoft:main Apr 24, 2026
20 checks passed
@KRRT7
Copy link
Copy Markdown
Contributor

KRRT7 commented Apr 25, 2026

A couple of things I noticed looking at this:

  1. source = src should probably be source = src,test — the previous source = test was wrong in the other direction, but this just swapped which half we're missing. Measuring test coverage catches dead test code from copy-paste naming mistakes, obsolete helpers, and tests that silently never run. Ned Batchelder has a good writeup on why: https://nedbatchelder.com/blog/202008/you_should_include_your_tests_in_coverage.html

Important

This matters even more now that AI agents are writing and modifying tests. They're great at producing code that looks correct — proper structure, reasonable assertions, good names — but they regularly introduce subtle bugs: tests that pass because they assert the wrong thing, helper functions that never get called, branches in test logic that are dead on arrival. The failure mode is silent: CI is green, coverage of src looks fine, nobody notices the test isn't actually exercising anything. Covering your test code is a cheap defensive check against that. If an agent generates a test function but misspells the name so pytest doesn't collect it, source = src won't catch it — source = src,test will show it as uncovered immediately.


  1. make.bat coverage target is missing COVERAGE_PROCESS_START — the Makefile sets it; make.bat doesn't. Without it, you get zero coverage from the MCP server code on Windows. Should be:

    set "COVERAGE_PROCESS_START=.coveragerc"
    uv run coverage run -m pytest
    set "COVERAGE_PROCESS_START="
    Why this matters

    The MCP server tests spawn the server as a subprocess (test_mcp_server.py launches python -m typeagent.mcp.server via StdioServerParameters). coverage run -m pytest only measures the main pytest process — it can't see into child processes. COVERAGE_PROCESS_START is the mechanism that bridges that gap: the test fixture propagates it to the subprocess env, server.py calls coverage.process_startup() which sees the var and starts collecting, and coverage combine merges everything at the end. Without it, process_startup() is a no-op.

@bmerkle
Copy link
Copy Markdown
Collaborator Author

bmerkle commented Apr 25, 2026

  1. regarding the inclusion of the test directory, I would disagree.
    The coverage should report the coverage of the production code or production app. Mixing up the src and test code dilutes the number or metric.
    I would also not necessarily measure coverage on infrastructure code which was only generated via a template or template based code generator.
    The argument has not changed with AI IMO, but I also asked on a mailing list where I know some TDD experts, about their opinion....
    Also please look at the comments too the article you cited...

  2. this is a bug for subprocesses. Without it, subprocess coverage (spawned by pytest) isn't collected.
    Thanks for finding it. Created a fix set COVERAGE_PROCESS_START, which the Makefile does. #253

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants