Skip to content

ci: guard coverage combine against empty coverage glob in fast shards#2630

Merged
chtruong814 merged 1 commit into
mainfrom
achartier/fix-coverage-combine-empty-fast-shards
May 29, 2026
Merged

ci: guard coverage combine against empty coverage glob in fast shards#2630
chtruong814 merged 1 commit into
mainfrom
achartier/fix-coverage-combine-empty-fast-shards

Conversation

@achartier

Copy link
Copy Markdown
Contributor

What

Guard the trailing coverage combine .coverage* in all 13 L1_Functional_Tests_*.sh shard scripts so it is skipped when no coverage files were produced.

cd ${PROJECT_ROOT}/tests
if compgen -G ".coverage*" > /dev/null; then
    coverage combine .coverage*
fi

Why

#2345 split the monolithic L1 functional tests into per-domain shards, each ending with an unconditional coverage combine .coverage*. Under CI:Lfast (FAST=1), run_test skips every sub-test not marked fast. The AutoModel and Megatron_3 shards have zero run_test fast tests, so nothing runs and no .coverage* files are produced. The glob then expands to the literal .coverage*, coverage combine exits non-zero ("Couldn't combine from non-existent path"), and with set -euo pipefail the whole shard aborts → the harness reports "Did not finish." → FAILURE.

This makes fast_L1_Functional_Tests_AutoModel and fast_L1_Functional_Tests_Megatron_3 red on every CI:Lfast PR since #2345 merged. The other shards survive only because they each happen to have ≥1 fast test producing a coverage file.

Fix scope

The broken-today shards are AutoModel and Megatron_3, but the identical unguarded tail exists in all 13 shards, so the guard is applied uniformly to prevent the same latent failure if the last fast test is ever removed from another shard.

The guard still surfaces genuine coverage combine failures when coverage files do exist — it only no-ops the empty case.

Labeled CI:Lfast so the previously-failing AutoModel/Megatron_3 fast shards exercise this fix directly.

cc @chtruong814 (author of #2345)

PR #2345 split the L1 functional tests into per-domain shards, each ending
with an unconditional `coverage combine .coverage*`. Under CI:Lfast
(FAST=1), shards with no `run_test fast` tests (AutoModel, Megatron_3)
skip every sub-test and produce no .coverage* files. The glob then expands
to the literal `.coverage*`, `coverage combine` exits non-zero, and with
`set -euo pipefail` the whole shard aborts -> "Did not finish." -> FAILURE.

Guard the combine so it is a no-op when there is nothing to combine, while
still surfacing real combine failures when coverage files exist.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Aurelien Chartier <2567591+achartier@users.noreply.github.com>
@achartier achartier requested a review from a team as a code owner May 29, 2026 15:58
@copy-pr-bot

copy-pr-bot Bot commented May 29, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@achartier achartier added the CI:Lfast Runs a fast test suite and re-use nightly `main` container (but sync dependencies to PRs version) label May 29, 2026
@achartier achartier requested a review from chtruong814 May 29, 2026 15:58
@achartier

Copy link
Copy Markdown
Contributor Author

/ok to test a45a19b

@chtruong814

Copy link
Copy Markdown
Contributor

@achartier thanks for this! I was working on the fix as well and saw your PR.

@chtruong814 chtruong814 merged commit bb715a7 into main May 29, 2026
76 checks passed
@chtruong814 chtruong814 deleted the achartier/fix-coverage-combine-empty-fast-shards branch May 29, 2026 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI:Lfast Runs a fast test suite and re-use nightly `main` container (but sync dependencies to PRs version)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants