You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
GitHub Actions stops a job at the first failed step, so a red ctest used to take the
Codecov upload and every debug artifact down with it — exactly the runs where flake
statistics and logs are most useful. This makes the reporting steps run regardless.
Upload Codecov test results (JUnit) and debug artifacts even when ctest / pytest
fail, using if: ${{ !cancelled() }} instead of the implicit success().
Emit JUnit from the jobs that produced no report at all before: --output-junit is now
passed to ctest on Windows, macOS, and Alpine.
Tag each Codecov upload with one flag and a distinct name:. Integration uses <model>-<version> (e.g. ur5e-10.14.0), robot_model_check uses check_version_<model>-<version>, platforms use unit-windows / unit-macos-arm64 / unit-macos-x86_64 / unit-alpine, Python uses python_scripts, ROS uses ros-<distro>-<repo>. Codecov rejects comma-separated
flags on a single upload.
Attach junit.xml as a GitHub artifact from integration, model-check, Python, Windows,
macOS, Alpine, and ROS industrial CI. PolyScope X also uploads test_artifacts/.
Coverage XML is uploaded only from a successful full integration ctest, and no longer
depends on the Codecov test-results upload succeeding. robot_model_check uploads
JUnit but no gcov — that one-test report was pulling project coverage down.
Set BASEDIR=/tmp/ici_ws for industrial CI so colcon/catkin test_results XML is
bind-mounted out of the ICI container. The downstream ROS driver's reports were
previously unreachable from the runner. They are flattened once into ros_junit/ and
used for both the artifact and the Codecov upload.
Reports are uploaded exactly as the test runners write them. No new scripts, no changes
outside .github/.
Codecov Test Analytics
Worth recording, because it is the one goal this PR does not achieve.
Only the four pytest cases appear under Codecov Tests for this branch. The CTest and ROS
uploads are accepted and then dropped server-side:
The reports themselves are complete and valid. Running Codecov's own parser
(test-results-parser, the Rust library their worker imports) over the uploaded
artifacts reads 374 tests per platform job and 646 per integration job, with the
failure counts correct.
The CLI reports success for every one of them
(Found N test_results files, Upload queued for processing complete), but the commit
ends up with a single test-results upload record — the pytest one.
master already ingests raw ctest --output-junit output, so the CTest dialect is
not the obstacle either.
So this is not a report-format problem and not something the workflow files can fix.
Getting C++ and ROS cases into Test Analytics needs the branch to run inside this
repository with the org CODECOV_TOKEN rather than as a fork PR, or a Codecov support
ticket. Either way it is out of scope here; the JUnit is meanwhile available as GitHub
artifacts on every run.
Test plan
A failing integration ctest still runs the Codecov test-results step and attaches junit.xml plus URSim / test artifacts.
Verified on run_tests (10.12.0-ur5e): ctest exited 8, JUnit and debug artifacts
uploaded. Coverage is intentionally skipped on that path.
A passing run still uploads Codecov reports and does not skip run examples.
Verified on run_tests (3.14.3-ur5). run examples keeps its original position
before gcovr, so example coverage is still measured.
Codecov uses separate flags for Windows, macOS (arm64/x86_64), Alpine, and each
integration matrix cell. One flag per upload, check_model stays on check_version_*.
robot_model_check uploads JUnit on failure, and URSim docker logs only when the
job fails. Coverage XML is no longer generated there.
Windows, macOS, and Alpine produce and upload a JUnit report (374 tests each).
Industrial CI collects the downstream driver's test_results XML and uploads it as
an artifact.
Cancelled jobs do not upload half-written artifacts (!cancelled(), not always()).
CTest and ROS cases appear under Codecov Tests. Blocked, see above.
✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.68%. Comparing base (5062a24) to head (004d201). ⚠️ Report is 1 commits behind head on master.
✅ All tests successful. No failed tests found.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
The ROS Codecov glob cannot discover external results, and report failures can incorrectly skip example tests.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
.github/workflows/ci.yml:175
Moving this after the post-processing steps makes it depend on every earlier Codecov/gcovr step through GitHub Actions' implicit success() condition. Consequently, a passing ctest followed by a transient Codecov failure (fail_ci_if_error: true) now skips the examples, whereas they previously ran immediately after a successful test. Move this step before report uploads, or give the test step an ID and condition this specifically on that outcome.
One flag per upload. Codecov was rejecting integration,ur5e-10.12.0-style comma lists (Multiple flags detected). Each upload now has a single flag.
check_model vs integration stay separate. Integration keeps <model>-<version> (e.g. ur5e-10.14.0). check_model is back on check_version_<model>-<version>, so the one-test coverage report is not mixed into the integration tag.
Coverage drop. That ~3% was the check_model 12% reports (and a failed ctest partial gcovr) landing on the same flags as full integration runs. Coverage XML is now uploaded only when ctest succeeded; JUnit/test analytics still upload on failure.
Test reports on Codecov. Test analytics is a separate Codecov product (Tests tab), not the coverage flags table. With one flag per upload the JUnit reports should process; they often only show up after the upload is processed, and fork PRs can be delayed. GitHub still has the *_junit artifacts if you want to inspect the XML directly.
Also copied ROS JUnit into the workspace before the Codecov action (absolute /tmp/... globs were outside its search root) and run examples only if the test step itself succeeded, so a Codecov hiccup cannot skip them.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
The model-check job still uploads partial coverage from its single-test run, contrary to the stated coverage goal.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
.github/workflows/ci.yml:380
This condition still uploads coverage from robot_model_check, even though this job runs only PrimaryClientTest.test_robot_type at line 342. A successful one-test run therefore still contributes its partial gcov report to Codecov, contradicting the PR's stated goal that one-test runs must not affect project coverage. Remove the gcov generation/upload steps from this job (or otherwise exclude this report from project coverage).
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
Coverage sequencing, duplicate ROS discovery, and stale model-check coverage carryforward can produce incomplete or inaccurate Codecov results.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
.github/codecov.yml:8
Global carryforward also applies to the historical check_version_* coverage flags whose uploads this PR removes because they depress project coverage. When those coverage reports are absent on future commits, Codecov will reuse their previous one-test snapshots, defeating that removal. Keep carryforward disabled for every check_version_* flag (or scope it only to the coverage flags that should persist). .github/workflows/ci.yml:170
These conditions still inherit GitHub Actions' implicit success(). If the preceding test-results upload fails (it has fail_ci_if_error: true), all three coverage steps are skipped even though ctest passed. Add an explicit status check and gate each dependent step on its direct predecessor so a Codecov test-results outage cannot suppress valid coverage.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
Coverage can be suppressed by a Codecov failure, and multi-suite JUnit duration is calculated incorrectly.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
.github/workflows/ci.yml:170
These three conditions do not contain a status-check function, so GitHub implicitly prepends success(). If the preceding test-results Codecov upload fails while ctest succeeded, all coverage work is skipped despite steps.test.outcome == 'success'. Add !cancelled() to each condition, as done for run examples, so a reporting outage cannot suppress valid coverage generation/upload.
@urfeex thanks — your review turned out to be right on every point, including the one I
spent the longest getting wrong. Where each stands now:
1. ctest and check_model coverage mixed into one model-version tag. Fixed. robot_model_check uses check_version_<model>-<version> and no longer uploads gcov at
all; a single PrimaryClientTest.test_robot_type run was contributing an ~11% report
over the whole library. It still uploads its JUnit.
2. Codecov complains about multiple flags per upload. Fixed. Every upload carries
exactly one flag plus a distinct name:.
3. Project coverage drops by almost 3 percent. Cause found and removed. It was a
165-line Python helper this branch added, of which 86 lines were uncovered — the numbers
line up exactly (+284 lines, +6 hits). That script is gone, so this should be roughly
neutral now. Related: I had also added a global carryforward: true, which Copilot
correctly flagged; it was replaying the obsolete ~11% check-model reports into every new
commit, so it has been dropped too. .github/codecov.yml is now unchanged from master
and this PR touches only .github/workflows.
4. "I can't find the test reports on Codecov." You were right to be suspicious, and
this is the one thing the PR does not achieve. Only the four pytest cases ever appear.
I chased it for several commits on the theory that CTest's JUnit dialect needed
rewriting, which was wrong:
Codecov's own parser (test-results-parser, the library their worker imports) reads
our reports unmodified — 374 cases per platform job, 646 per integration job, failure
counts intact.
The CLI reports success for every upload (Found N test_results files, Upload queued for processing complete), but the commit ends up with a single
test-results upload record: the pytest one. The rest are accepted and dropped
server-side.
master already ingests raw ctest --output-junit output — ~313k test rows — so the
format is not the obstacle.
The difference I cannot rule out from outside Codecov is that this is a fork PR, where CODECOV_TOKEN is empty and uploads go through the tokenless path. pytest survives it;
the later uploads do not. So the remaining options are to run the branch inside this
repository with the org token, or to raise it with Codecov support. I have left it out of
scope and said so in the description, rather than keep reshaping XML that was never the
problem.
Net effect: the PR is now workflow-only, 183/32 across 6 files, down from 400/35 across 9
including that script. What it does deliver is the original goal — JUnit and debug
artifacts survive a red ctest — plus JUnit from Windows, macOS and Alpine, which
produced no report at all before, and the ROS driver's test_results XML, which could
not previously escape the industrial_ci container. Windows, Alpine and macOS are green on
the latest run.
Because this condition contains cancelled(), it bypasses the implicit success() check and remains true even when the preceding gcovr step fails, provided CTest passed. That can upload a partial coverage.xml (or add a misleading missing-report failure). Give the generator an ID and gate this upload on its outcome; this still keeps coverage independent of the earlier test-results upload.
Addressed in 275d2ab — this one was a real regression I had introduced, and it was
present in one more place than the review found.
The !cancelled() on the coverage steps was there to decouple them from the preceding
test-results upload, which carries fail_ci_if_error: true and can fail on its own on a
tokenless fork upload. But !cancelled() suppresses the implicit success() for the
whole chain, so it also decoupled the coverage steps from each other. Each step is now
gated on the outcome of whatever produces its input, which keeps the intended
independence from the test-results upload:
That closes three holes rather than one. A failed gcovr no longer uploads a partial coverage.xml (the point raised); a failed apt-get install gcovr no longer surfaces as
a misleading missing-report Codecov failure; and a failed run_examples.sh no longer
uploads coverage that is missing the examples' contribution, which matters because the
examples are deliberately ordered before gcovr so that they are counted. Gating on steps.gcovr.outcome also subsumes the previous steps.test.outcome check, since a
failed ctest leaves gcovr skipped rather than failed.
Also fixed, not flagged:test_python_scripts.yml had the same defect in a worse
form. I had put a bare if: ${{ !cancelled() }} on its coverage upload where master
relied on the implicit success(), so a failing pytest run uploaded its understated
coverage under the python_scripts flag. It now gates on the pytest step's outcome.
Uploading coverage from a failed test run was never a goal of this PR — only test results
and debug artifacts.
robot_model_check needs no equivalent change; it no longer produces or uploads coverage
at all.
GitHub Actions skipped later steps after a red ctest, so flake stats and debug artifacts were missing.
Tag each upload with job and environment flags so Windows, macOS, Alpine, and integration runs stay distinct.
Keep integration and check_model coverage on separate flags, and stop
attaching two flags to one report. That mixing was dragging project
coverage down and hiding test analytics.
Codecov's JUnit parser reads ctest --output-junit and colcon output as-is,
so the rewriting script was not needed to get reports ingested. Drop it and
point the test-results uploads at the reports the test runners produce.
Carryforward kept replaying the one-test robot_model_check coverage reports
this branch stops uploading, so those ~11% reports would have gone on
depressing project coverage anyway. Coverage was already skipped after a
failed ctest before this branch, so carryforward is not needed to keep the
existing behaviour.
Adding !cancelled() to the coverage steps was meant to decouple them from the
preceding test-results upload, which can fail on its own. It also decoupled
them from each other, so a failed run_examples.sh or gcovr would still upload
whatever coverage.xml happened to be on disk, or fail the job with a missing
report. Chain the steps on the outcome of the step that produces their input
instead, which keeps the intended independence from the test-results upload.
Tick the box to add this pull request to the merge queue (same as @mergifyio queue).
Queue this pull request
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
urfeex
left a comment
Summary
GitHub Actions stops a job at the first failed step, so a red
ctestused to take theCodecov upload and every debug artifact down with it — exactly the runs where flake
statistics and logs are most useful. This makes the reporting steps run regardless.
ctest/ pytestfail, using
if: ${{ !cancelled() }}instead of the implicitsuccess().--output-junitis nowpassed to
cteston Windows, macOS, and Alpine.name:. Integration uses<model>-<version>(e.g.ur5e-10.14.0),robot_model_checkusescheck_version_<model>-<version>, platforms useunit-windows/unit-macos-arm64/unit-macos-x86_64/unit-alpine, Python usespython_scripts, ROS usesros-<distro>-<repo>. Codecov rejects comma-separatedflags on a single upload.
junit.xmlas a GitHub artifact from integration, model-check, Python, Windows,macOS, Alpine, and ROS industrial CI. PolyScope X also uploads
test_artifacts/.ctest, and no longerdepends on the Codecov test-results upload succeeding.
robot_model_checkuploadsJUnit but no gcov — that one-test report was pulling project coverage down.
BASEDIR=/tmp/ici_wsfor industrial CI so colcon/catkintest_resultsXML isbind-mounted out of the ICI container. The downstream ROS driver's reports were
previously unreachable from the runner. They are flattened once into
ros_junit/andused for both the artifact and the Codecov upload.
Reports are uploaded exactly as the test runners write them. No new scripts, no changes
outside
.github/.Codecov Test Analytics
Worth recording, because it is the one goal this PR does not achieve.
Only the four pytest cases appear under Codecov Tests for this branch. The CTest and ROS
uploads are accepted and then dropped server-side:
(
test-results-parser, the Rust library their worker imports) over the uploadedartifacts reads 374 tests per platform job and 646 per integration job, with the
failure counts correct.
(
Found N test_results files,Upload queued for processing complete), but the commitends up with a single test-results upload record — the pytest one.
masteralready ingests rawctest --output-junitoutput, so the CTest dialect isnot the obstacle either.
So this is not a report-format problem and not something the workflow files can fix.
Getting C++ and ROS cases into Test Analytics needs the branch to run inside this
repository with the org
CODECOV_TOKENrather than as a fork PR, or a Codecov supportticket. Either way it is out of scope here; the JUnit is meanwhile available as GitHub
artifacts on every run.
Test plan
cteststill runs the Codecov test-results step and attachesjunit.xmlplus URSim / test artifacts.Verified on
run_tests (10.12.0-ur5e):ctestexited 8, JUnit and debug artifactsuploaded. Coverage is intentionally skipped on that path.
run examples.Verified on
run_tests (3.14.3-ur5).run exampleskeeps its original positionbefore
gcovr, so example coverage is still measured.integration matrix cell. One flag per upload,
check_modelstays oncheck_version_*.robot_model_checkuploads JUnit on failure, and URSim docker logs only when thejob fails. Coverage XML is no longer generated there.
test_resultsXML and uploads it asan artifact.
!cancelled(), notalways()).