Skip to content

chore(ci): exit 77 when tools missing to distinguish skip from pass#3055

Merged
imbajin merged 2 commits into
apache:masterfrom
bitflicker64:ci/test-skip-exit-code
Jun 9, 2026
Merged

chore(ci): exit 77 when tools missing to distinguish skip from pass#3055
imbajin merged 2 commits into
apache:masterfrom
bitflicker64:ci/test-skip-exit-code

Conversation

@bitflicker64

@bitflicker64 bitflicker64 commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Purpose of the PR

Main Changes

Test scripts:

  • Change exit 0exit 77 in all tool-missing skip paths
  • Change exit 0exit 77 in the Store ulimit -n skip path

CI workflows (server-ci.yml, pd-store-ci.yml):

  • Wrap each test step with set +e / exit code capture
  • Treat exit 77 as a visible ::notice:: annotation (not a failure)
  • All other non-zero exits still fail the step

Verifying these changes

  • Need tests and can be verified as follows:
    • Remove lsof from the runner path → CI step shows ::notice::... skipped instead of silently passing
    • Normal run (all tools present) → exit 0, step passes as before
    • Test failure → exit 1, step fails as before

Does this PR potentially affect the following parts?

  • Dependencies
  • Modify configurations
  • The public API
  • Other affects
  • Nope

Documentation Status

  • Doc - No Need

All three startup test scripts previously exited 0 when required tools
(lsof, curl, java) were not found. This is indistinguishable from a
passing test run — CI shows green even though no tests ran.

Change skip exits to 77 (conventional skip code) and update the CI
workflow steps to treat exit 77 as a visible skip notice rather than
a failure.

Flagged as non-blocking follow-up in reviews of apache#3044 and apache#3047.
Related to: apache#3043
@dosubot dosubot Bot added size:XS This PR changes 0-9 lines, ignoring generated files. ci-cd Build or deploy tests Add or improve test cases labels Jun 8, 2026
@bitflicker64

Copy link
Copy Markdown
Contributor Author

@dosu review?

@dosubot

dosubot Bot commented Jun 8, 2026

Copy link
Copy Markdown

I don't perform code reviews (assessing PRs for bugs, design, style, performance, or security). However, I can help in other ways — for example, I can research how the existing CI workflows or test scripts are structured, answer questions about the conventions used in the repository, or help with documentation updates related to this change.

Would any of those be useful?

To reply, just mention @dosu.


Share context across your team and agents. Try Dosu.

Leave Feedback Learn about hugegraph with Dosu Add Dosu to your team

@imbajin imbajin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: yes. Summary: The startup test wrapper still reports skipped suites as successful GitHub Actions steps. Evidence: static review of the workflow wrapper and local dummy-dist runs where the scripts returned 77.

Latest-head CI has visible failures; please also check dependency-check / build-commons (11): 🔗 https://github.com/apache/hugegraph/actions/runs/27120670959/job/80036825383

Comment thread .github/workflows/server-ci.yml Outdated
Replace the exit-77 + set+e wrapper with a preflight step that writes
can_run and skip_reason to GITHUB_OUTPUT. The test step is guarded by
if: steps.<id>.outputs.can_run == 'true', so it shows as grey Skipped
(not green Success) when prerequisites are missing.

The Store preflight distinguishes 'missing tool: <name>' from
'ulimit -n is <N> (store requires >= 1024)' with separate skip_reason
values as requested in review.

Addresses blocking review feedback from imbajin on apache#3055.
Related to: apache#3043

@imbajin imbajin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 from my side. The previous concern has been addressed.

One non-blocking improvement for a future follow-up: the preflight checks are now duplicated between the workflow YAML and the startup scripts, so it may be worth consolidating them into a shared helper later to avoid drift when prerequisites change.

@dosubot dosubot Bot added the lgtm This PR has been approved by a maintainer label Jun 9, 2026
@bitflicker64

Copy link
Copy Markdown
Contributor Author

+1 from my side. The previous concern has been addressed.

One non-blocking improvement for a future follow-up: the preflight checks are now duplicated between the workflow YAML and the startup scripts, so it may be worth consolidating them into a shared helper later to avoid drift when prerequisites change.

suree that makes sense should i add a TODO ?

@imbajin

imbajin commented Jun 9, 2026

Copy link
Copy Markdown
Member

suree that makes sense should i add a TODO ?

I’m fine either way~

@imbajin imbajin changed the title ci(test): exit 77 when tools missing to distinguish skip from pass chore(ci): exit 77 when tools missing to distinguish skip from pass Jun 9, 2026
@imbajin imbajin merged commit 0ecd844 into apache:master Jun 9, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci-cd Build or deploy lgtm This PR has been approved by a maintainer size:XS This PR changes 0-9 lines, ignoring generated files. tests Add or improve test cases

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ci(test): distinguish "skipped due to missing tools" from "tested and passed" in startup script tests

2 participants