Fix the gaps in workflows to upload L1 test results to portal - #407
Open
shibu-kv wants to merge 7 commits into
Open
Fix the gaps in workflows to upload L1 test results to portal#407shibu-kv wants to merge 7 commits into
shibu-kv wants to merge 7 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR repairs the L1 unit test result publishing pipeline after a container migration by ensuring the GTest report directory exists and by transferring reports between jobs via GitHub Actions artifacts rather than container volume mounts.
Changes:
- Ensure
/tmp/Gtest_Reportexists during unit test execution so report generation/copying doesn’t fail due to a missing directory. - Upload the copied L1 GTest reports as a workflow artifact and download them in the upload job (replacing the previous
/tmpvolume mapping).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
test/run_ut.sh |
Creates the GTest report output directory before running test binaries so reports can be produced and collected reliably. |
.github/workflows/L1-tests.yml |
Switches cross-job report transfer from container volume mounts to upload/download artifact steps to support the new execution container flow. |
Suppressed comments (1)
.github/workflows/L1-tests.yml:53
upload-test-resultscurrently won’t run ifexecute-L1-tests-on-prfails (defaultneedsbehavior), even if the report artifact was uploaded viaif: always(). Addif: always()at the job level so results can still be pushed to the portal on failing test runs (or at least attempt/skip based on secret availability).
upload-test-results:
name: Upload L1 test results to automatic test result management system
needs: execute-L1-tests-on-pr
runs-on: ubuntu-latest
container:
Comment on lines
+47
to
+49
| # Create test results directory | ||
| mkdir -p /tmp/Gtest_Report | ||
|
|
Comment on lines
38
to
+42
| run: | | ||
| docker cp native-platform:/tmp/Gtest_Report /tmp/Gtest_Report | ||
| ls -l /tmp/Gtest_Report | ||
|
|
||
| - name: Upload test results as artifact |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (4)
test/run_ut.sh:49
- The script creates /tmp/Gtest_Report but doesn’t check for failure. If /tmp is read-only/full, tests will still run and CI may later fail to collect/upload results with little signal. Fail fast (and optionally clear stale JSON from previous runs) when the directory cannot be prepared.
# Create test results directory
mkdir -p /tmp/Gtest_Report
.github/workflows/L1-tests.yml:75
- In the container-based upload job, downloading the artifact to /tmp can land on the runner filesystem and not be visible inside the job container; the subsequent
ls/gtest-json-result-push.pythen won’t see any files. Download into $GITHUB_WORKSPACE (mounted into the container) and reference that path in the upload step.
- name: Download test results
uses: actions/download-artifact@v4
with:
name: gtest-results
path: /tmp/Gtest_Report
- name: Upload results
if: github.repository_owner == 'rdkcentral'
env:
TEST_RESULTS_PUSH_URL: ${{ secrets.TEST_RESULTS_PUSH_URL }}
run: |
echo "Contents in /tmp/Gtest_Report:"
ls -l /tmp/Gtest_Report
if [ -z "${TEST_RESULTS_PUSH_URL}" ]; then
echo "TEST_RESULTS_PUSH_URL secret not set. Skipping test result upload."
exit 0
fi
git config --global --add safe.directory `pwd`
gtest-json-result-push.py /tmp/Gtest_Report "${TEST_RESULTS_PUSH_URL}" `pwd`
.github/workflows/L1-tests.yml:5
- The pull_request branch filter now includes a feature branch name. This deviates from other workflows in this repo (which target stable base branches like develop/main) and can cause the workflow to run (or be skipped) unexpectedly as branches are created/removed. Consider limiting to stable base branches.
pull_request:
branches: [ develop, main , feature/L1-test-result-upload ]
.github/workflows/L1-tests.yml:47
- Artifact upload uses an absolute /tmp path. To avoid runner-/container-specific filesystem differences and to keep artifacts within the checked-out workspace, copy results into ${{ github.workspace }} and upload from there (also avoids clobbering host /tmp across parallel jobs).
This issue also appears on line 57 of the same file.
- name: Copy L1 test results to runner
run: |
docker cp native-platform:/tmp/Gtest_Report /tmp/Gtest_Report
ls -l /tmp/Gtest_Report
- name: Upload test results as artifact
uses: actions/upload-artifact@v4
with:
name: gtest-results
path: /tmp/Gtest_Report
retention-days: 1
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (6)
test/run_ut.sh:48
mkdir -pfailure is currently ignored; if/tmpis read-only or the command fails, the script will continue and later steps may silently miss result files. Treat this as a hard failure so the CI job reports the real root cause.
# Create test results directory
mkdir -p /tmp/Gtest_Report
.github/workflows/L1-tests.yml:47
- Same as the copy step: when tests fail, this artifact upload won't run unless forced, which breaks the goal of uploading results for failed test runs. Add
if: ${{ always() }}so the artifact is published even on failure.
- name: Upload test results as artifact
uses: actions/upload-artifact@v4
with:
name: gtest-results
path: /tmp/Gtest_Report
retention-days: 1
.github/workflows/L1-tests.yml:53
upload-test-resultswill not run ifexecute-L1-tests-on-prfails (defaultneedsbehavior), which prevents result upload exactly when tests fail. Add a job-levelif: ${{ always() }}so uploading can still proceed when prior job fails but artifacts exist.
upload-test-results:
name: Upload L1 test results to automatic test result management system
needs: execute-L1-tests-on-pr
runs-on: ubuntu-latest
container:
.github/workflows/L1-tests.yml:41
- If any unit test fails,
docker exec ... run_ut.shexits non-zero and this step will be skipped, so you lose the JSON results needed for artifact/portal upload. Make this copy step run regardless of earlier step success.
This issue also appears in the following locations of the same file:
- line 42
- line 49
- name: Copy L1 test results to runner
run: |
docker cp native-platform:/tmp/Gtest_Report /tmp/Gtest_Report
ls -l /tmp/Gtest_Report
.github/workflows/L1-tests.yml:5
- Including
feature/L1-test-result-uploadunderon.pull_request.branchesonly triggers the workflow for PRs targeting that branch (base ref). If the intent was to run this workflow for PRs coming from that feature branch, this filter won’t help and may be left behind accidentally.
branches: [ develop, main , feature/L1-test-result-upload ]
.github/workflows/code-coverage.yml:5
- Same branch-filter concern here:
on.pull_request.branchesmatches the PR base branch, not the source branch. Keeping a temporary feature branch name here is likely unintended and won’t make the workflow run for PRs originating from that branch.
branches: [ develop, main, feature/L1-test-result-upload ]
The upload scripts were not running within the source checkout directory
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (3)
test/run_ut.sh:48
- The new test-results directory creation doesn’t check for failure. If /tmp is not writable (or filesystem issues occur), the script will continue and later steps may fail with less clear errors. Please fail fast with an explicit error message when mkdir fails.
# Create test results directory
mkdir -p /tmp/Gtest_Report
.github/workflows/code-coverage.yml:5
on.pull_request.branchesfilters the PR base branch. Including a feature branch name here is unusual and likely temporary; it can cause workflows to run for PRs targeting that feature branch. Consider limiting this to long-lived base branches only.
branches: [ develop, main, feature/L1-test-result-upload ]
.github/workflows/L1-tests.yml:5
on.pull_request.branchesfilters the PR base branch. Including a feature branch name here is unusual and likely temporary; it can cause workflows to run for PRs targeting that feature branch and is inconsistent with other workflows (e.g., .github/workflows/L2-tests.yml:5 uses onlydevelop). Consider limiting this to long-lived base branches only.
branches: [ develop, main , feature/L1-test-result-upload ]
Code Coverage Summary |
Include posting coverage summary to every PR in develop
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (5)
test/run_ut.sh:49
- Creating /tmp/Gtest_Report without clearing it can cause stale JSON reports from previous runs (e.g., local dev or a reused container) to be included in the aggregation and uploaded artifacts. Clean the directory before running tests to ensure only current results are reported.
# Create test results directory
mkdir -p /tmp/Gtest_Report
.github/workflows/L1-tests.yml:6
- The workflow uses actions/github-script to create a PR comment, but the workflow does not declare write permissions for issues/PRs. If repo defaults are restricted, comment creation can fail with 403. Declare explicit minimal permissions at workflow level.
on:
pull_request:
branches: [ develop, main , feature/L1-test-result-upload ]
.github/workflows/L1-tests.yml:43
docker cp native-platform:/tmp/Gtest_Report /tmp/Gtest_Reportcan create a nested directory if /tmp/Gtest_Report already exists on the runner (docker cp copies the source directory into the destination directory). Remove/recreate the destination and copy the directory contents to keep artifact paths stable.
- name: Copy L1 test results to runner
run: |
docker cp native-platform:/tmp/Gtest_Report /tmp/Gtest_Report
ls -l /tmp/Gtest_Report
.github/workflows/L1-tests.yml:62
- The github-script step triggers an async API call without awaiting it. Without
await, the action can finish before the comment is actually posted, leading to intermittent missing PR comments. Await the promise and add newlines so the fenced block renders correctly.
const fs = require('fs');
const lcov_result = fs.readFileSync('/tmp/coverage_summary.txt', 'utf8');
github.issues.createComment({
issue_number: context.issue.number,
owner: context.repo.owner,
repo: context.repo.repo,
body:
'## Code Coverage Summary \n' +
' ' +
'```' +
lcov_result +
'```'
});
.github/workflows/L1-tests.yml:47
- PR comment creation will fail for pull requests coming from forks because the pull_request event GITHUB_TOKEN is read-only in that case. Guard the comment step so the job still produces artifacts/results even when the comment cannot be posted.
- name: Update the coverage report to Pull request using actions
uses: actions/github-script@v4
with:
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (5)
test/run_ut.sh:49
mkdir -p /tmp/Gtest_Reportis not checked for failure, so the script can continue and later steps will silently miss JSON results if/tmpis not writable or the directory cannot be created. Fail fast with an explicit error when the directory cannot be created.
# Create test results directory
mkdir -p /tmp/Gtest_Report
.github/workflows/L1-tests.yml:53
- The
github-scriptstep doesn’t await thecreateCommentcall and doesn’t guard against oversized content. If the API call fails (e.g., due to comment size limits), this can result in missing coverage comments or flaky behavior. Useawaitand truncate the body to stay within GitHub’s limits.
const fs = require('fs');
const lcov_result = fs.readFileSync('/tmp/coverage_summary.txt', 'utf8');
github.issues.createComment({
issue_number: context.issue.number,
.github/workflows/L1-tests.yml:5
- Including a feature branch name in the
pull_request.branchesfilter is likely temporary and can cause confusion/maintenance overhead after merge. If the workflow should run for PRs targetingdevelopandmain, remove the feature branch entry.
branches: [ develop, main , feature/L1-test-result-upload ]
.github/workflows/L1-tests.yml:36
lcov --list coverage.infocan produce very large output; writing the full listing to/tmp/coverage_summary.txt(and then posting it to a PR comment) is likely to exceed GitHub comment size limits and/or slow the workflow. Prefer extracting only the summary lines.
This issue also appears on line 49 of the same file.
- name: Run L1 Unit Tests inside container with coverage flags enabled
run: docker exec -i native-platform /bin/bash -c "cd /mnt/L1_CONTAINER_SHARED_VOLUME/ && sh test/run_ut.sh --enable-cov && lcov --list coverage.info > /tmp/coverage_summary.txt"
.github/workflows/code-coverage.yml:36
lcov --list coverage.infowrites the full per-file listing into/tmp/coverage_summary.txt. This is likely much larger than intended for a “summary” and can increase CI time and artifact size. Consider filtering to the aggregate summary lines only (as was previously done).
- name: Run unit tests with coverage flags enabled and Caculate the code coverage summary
run: docker exec -i native-platform /bin/bash -c "cd /mnt/L1_CONTAINER_SHARED_VOLUME/ && sh test/run_ut.sh --enable-cov && lcov --list coverage.info > /tmp/coverage_summary.txt"
Code Coverage Summary |
Code Coverage Summary |
Verified with the execution and limiting to develop based on results.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (4)
test/run_ut.sh:48
mkdir -pfailure would be silently ignored and the rest of the script would run without producing JSON reports. Since this directory is required for test result generation, fail fast (or at least log+exit) if it cannot be created.
# Create test results directory
mkdir -p /tmp/Gtest_Report
.github/workflows/L1-tests.yml:47
- Posting a PR comment will fail (and fail the job) on PRs coming from forks because the
GITHUB_TOKENis read-only forpull_requestevents. Gate this step to same-repo PRs (or make it non-blocking) so external contributions don’t break CI.
- name: Update the coverage report to Pull request using actions
uses: actions/github-script@v4
with:
.github/workflows/L1-tests.yml:36
lcov --list coverage.infocan be very large; writing the full per-file listing into a PR comment frequently exceeds GitHub’s comment size limit and adds noise. Keep this to a small summary (e.g., only Lines/Total) like the previous workflow did.
This issue also appears on line 45 of the same file.
- name: Run L1 Unit Tests inside container with coverage flags enabled
run: docker exec -i native-platform /bin/bash -c "cd /mnt/L1_CONTAINER_SHARED_VOLUME/ && sh test/run_ut.sh --enable-cov && lcov --list coverage.info > /tmp/coverage_summary.txt"
.github/workflows/code-coverage.yml:36
- This change captures the full
lcov --listoutput intocoverage_summary.txt. That content can be extremely large and may cause the subsequent PR comment step to fail due to GitHub comment size limits; prefer a small summary (e.g.,grep 'Lines\|Total') instead.
- name: Run unit tests with coverage flags enabled and Caculate the code coverage summary
run: docker exec -i native-platform /bin/bash -c "cd /mnt/L1_CONTAINER_SHARED_VOLUME/ && sh test/run_ut.sh --enable-cov && lcov --list coverage.info > /tmp/coverage_summary.txt"
Code Coverage Summary |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fix the workflow that seems to be broken with migration of container used for actual test execution