-
Notifications
You must be signed in to change notification settings - Fork 367
Fix CI can't be failed even the tests are failed #1367
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request fixes an issue where CI was not failing despite test failures by correctly capturing the exit status of the piped go test command. Key changes include:
- Replacing the usage of "$?" with "${PIPESTATUS[0]}" in the run-ci scripts.
- Updating four CI scripts to ensure accurate test result reporting.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| scripts/run-ci.sh | Updated to capture the correct exit status of go test |
| scripts/run-ci-extensible-load-manager.sh | Updated to capture the correct exit status of go test |
| scripts/run-ci-clustered.sh | Updated to capture the correct exit status of go test |
| scripts/run-ci-blue-green-cluster.sh | Updated to capture the correct exit status of go test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes CI scripts to correctly fail when go test encounters errors by enabling pipefail and capturing the real test command exit status.
- Adds
set -o pipefailto ensure pipeline failures propagate - Replaces
retval=$?withretval=${PIPESTATUS[0]}to grab the exit code ofgo testinstead oftee
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| scripts/run-ci.sh | Enables pipefail to prevent silent pipeline success |
| scripts/run-ci-extensible-load-manager.sh | Enables pipefail and captures test exit via PIPESTATUS |
| scripts/run-ci-clustered.sh | Enables pipefail and captures test exit via PIPESTATUS |
| scripts/run-ci-blue-green-cluster.sh | Enables pipefail and captures test exit via PIPESTATUS |
Comments suppressed due to low confidence (3)
scripts/run-ci-extensible-load-manager.sh:20
- The
pipefailoption and${PIPESTATUS}array are Bash-specific. Ensure the script’s shebang (e.g.,#!/usr/bin/env bash) invokes Bash rather than a POSIX shell to guarantee these features work.
set -o pipefail
scripts/run-ci-clustered.sh:20
- The
pipefailoption and${PIPESTATUS}array are Bash-specific. Ensure the script’s shebang (e.g.,#!/usr/bin/env bash) invokes Bash rather than a POSIX shell to guarantee these features work.
set -o pipefail
scripts/run-ci-blue-green-cluster.sh:20
- The
pipefailoption and${PIPESTATUS}array are Bash-specific. Ensure the script’s shebang (e.g.,#!/usr/bin/env bash) invokes Bash rather than a POSIX shell to guarantee these features work.
set -o pipefail
c89e7bf to
a071e78
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR addresses a CI issue where the pipeline incorrectly passes despite test failures by ensuring that the exit status of the go test command is correctly captured.
- Added "set -o pipefail" in multiple CI scripts to capture the proper exit status from the tee command.
- Updates include modifications to run-ci.sh, run-ci-extensible-load-manager.sh, run-ci-clustered.sh, and run-ci-blue-green-cluster.sh.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| scripts/run-ci.sh | Adds "set -o pipefail" to correctly capture exit status |
| scripts/run-ci-extensible-load-manager.sh | Adds "set -o pipefail" to correctly capture exit status |
| scripts/run-ci-clustered.sh | Adds "set -o pipefail" to correctly capture exit status |
| scripts/run-ci-blue-green-cluster.sh | Adds "set -o pipefail" to correctly capture exit status |
|
LGTM |
lhotari
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks
### Motivation This PR #1351 introduced some changes but breaked the CI. Currently, even if there are some failed tests, the CI won't be failed: https://github.com/apache/pulsar-client-go/actions/runs/14973771263/job/42060743359?pr=1364#step:6:9285 The root cause is because it captures the exit status of the tee command instead of the go test command. This causes the script to report "Tests passed" even when tests actually fail, leading to false positive CI results. ``` $TEST_CMD 2>&1 | tee $TEST_LOG ``` ### Modification - Use `set -o pipefail` to correctly capture the exit status of the `go test` command in the pipeline (cherry picked from commit a5c6dee)
Motivation
This PR #1351 introduced some changes but breaked the CI. Currently, even if there are some failed tests, the CI won't be failed: https://github.com/apache/pulsar-client-go/actions/runs/14973771263/job/42060743359?pr=1364#step:6:9285
The root cause is because it captures the exit status of the tee command instead of the go test command. This causes the script to report "Tests passed" even when tests actually fail, leading to false positive CI results.
Modification
set -o pipefailto correctly capture the exit status of thego testcommand in the pipelineVerifying this change
(Please pick either of the following options)
This change is a trivial rework / code cleanup without any test coverage.
(or)
This change is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(example:)
Does this pull request potentially affect one of the following parts:
If
yeswas chosen, please highlight the changesDocumentation