(fix) Fix code coverage reporting - #48822
Conversation
Read package-local .coverage files directly when generating the combined CI coverage report. Add regression tests for discovery, combination, and XML generation gating.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 1 pipeline(s). 9 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Azure Pipelines: Successfully started running 1 pipeline(s). 10 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🟡 Changes recommended
Coverage path normalization fails for duplicate package names across service directories.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes Azure Pipelines coverage collection, normalization, reporting, and cleanup.
Changes:
- Adds per-check coverage collection and opt-out support.
- Preserves test environments until reporting, then cleans them up.
- Adds regression tests for coverage and cleanup behavior.
File summaries
| File | Description |
|---|---|
scripts/devops_tasks/test_create_coverage.py |
Tests coverage reporting behavior. |
scripts/devops_tasks/create_coverage.py |
Combines and normalizes coverage data; package resolution does not handle duplicate package names across services. |
eng/tools/azure-sdk-tools/tests/test_install_and_test_coverage.py |
Tests coverage arguments and opt-out behavior. |
eng/tools/azure-sdk-tools/tests/test_dispatch_checks.py |
Tests environment finalization. |
eng/tools/azure-sdk-tools/tests/test_cleanup_isolate_dirs.py |
Tests isolated-environment cleanup. |
eng/tools/azure-sdk-tools/azpysdk/main.py |
Adds the coverage opt-out option. |
eng/tools/azure-sdk-tools/azpysdk/install_and_test.py |
Enables per-check package coverage. |
eng/scripts/run_coverage.py |
Accepts an explicit coverage data file. |
eng/scripts/dispatch_checks.py |
Conditionally preserves coverage environments. |
eng/scripts/cleanup_isolate_dirs.py |
Adds deferred environment cleanup. |
eng/pipelines/templates/steps/build-test.yml |
Runs cleanup after coverage generation. |
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| if len(matches) == 1: | ||
| return matches[0] | ||
|
|
||
| logging.warning( | ||
| "Unable to map coverage paths for %s: expected one package directory, found %d", | ||
| package_name, | ||
| len(matches), | ||
| ) | ||
| return None |
Package names are not unique across sdk/ (e.g. azure-ai-textanalytics exists under both sdk/textanalytics and sdk/cognitivelanguage). The previous find_package_directory search matched by basename globally, so ambiguous packages resolved to None and their .venv site-packages paths were left unnormalized, breaking source mapping in the coverage report. Record each package's originating directory as its .coverage file is discovered in find_coverage_files (unambiguous, since the coverage file lives directly under the actual package directory that produced it), and use that mapping first in find_package_directory. Retain the prior filesystem search as a fallback for callers that don't run coverage file discovery first. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The two moderate coverage attribution and isolate-retention issues must be addressed before approval.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
eng/scripts/dispatch_checks.py:768
- This now preserves isolate directories for every Azure Pipelines invocation unless
--disablecovwas supplied, even when the selected checks cannot produce coverage. Existing callers such asrun_pylint.yml,run_pyright.yml, and the samples invocation inbuild-test.ymlomit that flag; their environments will therefore no longer be deleted, and those jobs do not all run the new cleanup step. This can accumulate large virtual environments for the rest of the job. Restrict preservation to the checks that actually emit package coverage (currentlywhl,sdist,devtest, andwhl_no_aio) or to detected coverage output.
scripts/devops_tasks/create_coverage.py:49
- This does not actually keep duplicate package names unambiguous: the dictionary is keyed only by the basename, and on the second directory it warns but retains the first path. The repository currently has both
sdk/cognitivelanguage/azure-ai-textanalyticsandsdk/textanalytics/azure-ai-textanalytics, so if both produce data, every.venv/azure-ai-textanalytics/...XML path is rewritten to whichever directory sorted first and coverage is attributed to the wrong source. Please carry a unique service/package identity through the isolate path and normalization (or otherwise rewrite each data file using its originating full directory before combining).
existing_directory = package_directories.get(package_name)
if existing_directory and existing_directory != root:
logging.warning(
"Multiple package directories produced coverage for %s: %s and %s. "
"Coverage paths for this package may not map back to repository sources.",
package_name,
existing_directory,
root,
)
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Resolve two issues raised in Copilot PR review: 1. create_coverage.py: after "coverage combine", identically named isolate paths (e.g. azure-ai-textanalytics under both textanalytics and cognitivelanguage) collapse into one mis-attributed entry, since the recorded venv path only carries the package basename. Combine the per-package data files ourselves and relocate each file's paths to its originating package directory *before* merging, so duplicate package names stay unambiguous. Revert find_package_directory to a filesystem safety-net that returns None on ambiguity rather than guessing. 2. dispatch_checks.py: isolate directories were preserved for every CI invocation unless --disablecov was passed, even for checks that emit no coverage (pylint, pyright, samples), leaking large virtual environments in jobs without a cleanup step. Restrict preservation to the checks that actually produce package coverage (whl, whl_no_aio, sdist, devtest, optional). Also run black on test_cleanup_isolate_dirs.py and update affected tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The static-analysis workflow runs black with eng/black-pyproject.toml (line-length 120); reformat the touched files to match. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| "whl_no_aio", | ||
| "sdist", | ||
| "devtest", | ||
| "optional", |
There was a problem hiding this comment.
🟡 Changes recommended
Duplicate packages can corrupt XML output, and optional-only dispatches do not produce the coverage data expected by the reporting flow.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
eng/scripts/dispatch_checks.py:67
optionaldoes not currently emit package coverage: its overriddenrun/prepare_and_test_optionalpath builds pytest arguments without--cov=<namespace>, never sets the newCOVERAGE_FILE, and never callscheck_coverage(azpysdk/optional.py:55-215). Consequently an optional-only dispatch is treated as coverage-producing here and preserves its isolate, butcreate_coverage.pystill finds no data or report. Either removeoptionalfrom this set or wire its override into the same coverage flow as the other listed checks.
"optional",
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Balanced
| generate_coverage_xml() | ||
| create_coverage_report() | ||
| if collect_coverage_files() and generate_coverage_xml(): | ||
| create_coverage_report() |
closes #46867
storage run that now outputs code coverage:
https://dev.azure.com/azure-sdk/internal/_build/results?buildId=6786508&view=codecoverage-tab
code coverage for core: https://dev.azure.com/azure-sdk/internal/_build/results?buildId=6785738&view=codecoverage-tab
Summary
sdk/**/.coveragefiles directly instead of the obsolete_coveragestaging directory.coveragefileTesting
python -m pytest -q scripts/devops_tasks/test_create_coverage.pypython -m black --check scripts/devops_tasks/create_coverage.py scripts/devops_tasks/test_create_coverage.py