Conversation
…to fail Opened by the scheduled autoreview pipeline after review of open PRs. Reviewed against the EmbeddedOS Master Design v2.0. Files: .github/workflows/ci.yml
Member
Author
|
Resolved the contribution-caused linked-issue policy failure without changing the branch.
|
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.
Problem
Three of the four test steps in
CI ΓÇö .githubcannot fail:.github/workflows/ci.yml:30-37. Each step's exit status is theecho's, so itis always 0. The
2>/dev/nullin front of it means a collection error ΓÇö asyntax error, a missing import, a renamed directory ΓÇö leaves no trace in the
log either. A regression in any of those three suites lands on
masterand therun still reports
success.Root cause
The
|| echo "... via run_all_tests.py"fallbacks were written for the casewhere
pytestis not installed andrun_all_tests.pyis the real runner. Butthe
Install dependenciesstep (:22) doespip install pytest pytest-cov gcovr, sopytestis always present, and the message these steps print is noteven true:
run_all_tests.pyis invoked only by theRun unit testsstepabove them. The fallback protects against a condition that cannot occur, and
pays for it by discarding the result in the condition that can.
.ai/reviewer.mdlists this shape explicitly ΓÇö "a|| trueon a build or teststep, a verification whose result is discarded" ΓÇö and directs that it be
treated as a finding regardless of the reason given.
Fix
Drop the
2>/dev/null || echo ...from the three steps, leaving the barepytestinvocation. Nothing else changes.Files changed
.github/workflows/ci.ymlExpected impact
Nothing changes on a passing run. The three suites already pass: run
34663355034
on
mastershows the same three invocations reporting3 passed,2 passedand
2 passedrespectively ΓÇö they were green, the workflow simply was notasking. From here on a failure in
tests/functional/,tests/performance/ortests/simulation/turns the job red instead of printing a sentence andcontinuing.
Risks and compatibility
The risk is the intended one: these steps can now fail. That is the point of
the change, and the evidence above is that they do not fail today.
Two related items are deliberately not touched here, so this PR stays one
focused change:
Run unit tests(:28) still readspytest tests/unit/ ... 2>/dev/null || python3 run_all_tests.py. There is notests/unit/directory in this repository, sopytestalways exits non-zeroand the fallback always runs. That step can fail ΓÇö
run_all_tests.pypropagates its status ΓÇö so it is not a gate that cannot fail, but the
arrangement is misleading and the right repair (point it at a directory that
exists, or drop the dead
pytesthalf) is a separate decision.security-scan(:55) runscppcheck --error-exitcode=0 --quiet . 2>&1 | head -50 || true, which cannotfail on three independent grounds. Making a static-analysis gate real is a
triage commitment ΓÇö someone has to own whatever it then reports ΓÇö so it is
recorded for a maintainer rather than flipped here.
Both are in the maintenance backlog.
Note on the verification table below: the three
pytest-*labels were firstrecorded as failures against
/usr/bin/python3, which has nopytestmodule onthis runner; they were re-run against the interpreter that does (Python 3.12)
and passed. CI installs
pytestexplicitly and runs Python 3.11, so thepython3 -m pytestform in the workflow is correct there.workflow-lintparses the edited YAML and asserts none of the three steps still discards its
own result.
Verification
Executed in an isolated worktree branched from
origin/master:pytest-functional/home/srpatcha/.local/share/uv/tools/pytest/bin/python -m pytest tests/functional/ -v --tb=shortpytest-performance/home/srpatcha/.local/share/uv/tools/pytest/bin/python -m pytest tests/performance/ -v --tb=shortpytest-simulation/home/srpatcha/.local/share/uv/tools/pytest/bin/python -m pytest tests/simulation/ -v --tb=shortworkflow-lintpython3 /tmp/check-ci-yaml.pyOpened by the scheduled autoreview pipeline (model
claude-opus-5), branched fromorigin/master. No human has reviewed this yet. Close it freely if the fix is wrong - a bad automated PR is a bug worth reporting.Fixes #11