Skip to content

ci: make C tests and ARM cross-build fail closed - #41

Open
AbdulRehman77777 wants to merge 1 commit into
embeddedos-org:masterfrom
AbdulRehman77777:fix/eai-ci-verification
Open

ci: make C tests and ARM cross-build fail closed#41
AbdulRehman77777 wants to merge 1 commit into
embeddedos-org:masterfrom
AbdulRehman77777:fix/eai-ci-verification

Conversation

@AbdulRehman77777

Copy link
Copy Markdown

Problem

The build-and-test workflow can currently report success without performing
the verification its job names imply.

The native C job passes -DBUILD_TESTS=ON, but eAI gates its test suite on
EAI_BUILD_TESTS. CMake therefore accepts the unused variable while leaving
the actual test suite disabled.

Reproducing the current configuration showed:

BUILD_TESTS:UNINITIALIZED=ON
EAI_BUILD_TESTS:BOOL=OFF
Total Tests: 0
No tests were found!!!

The same workflow also masks CTest and ARM cross-build failures with
|| true, and the ARM job references a toolchain path that does not exist.

Change

This PR keeps the scope limited to .github/workflows/ci.yml:

  • use -DEAI_BUILD_TESTS=ON for the native C test build
  • run CTest with --no-tests=error
  • remove || true from CTest so failures propagate to CI
  • use the existing Cortex-M4 toolchain at
    cmake/toolchains/toolchain-arm-cortex-m4.cmake
  • use -DEAI_BUILD_TESTS=OFF for the ARM cross-build
  • stop suppressing ARM configure/build errors and diagnostics

The ARM completion marker remains after the build. With failure suppression
removed, normal GitHub Actions shell semantics prevent it from being reached
when configuration or compilation fails.

Verification

Local verification performed:

  • YAML syntax: PASS
  • CMake configure with EAI_BUILD_TESTS=ON: PASS
  • CTest registration: PASS — 24 tests registered
  • controlled fail-closed CTest check: PASS — exit status 8
  • canonical ARM toolchain path exists: PASS
  • git diff --check: PASS

Native test compilation on this macOS host exposed existing platform-specific
test compilation failures involving FS_OPS and HAL_CORE / HAL_MEM.
Those issues are unrelated to this CI patch and were intentionally neither
fixed nor hidden.

ARM compilation is NOT RUN locally because arm-none-eabi-gcc is not
installed.

Related upstream work

This PR deliberately does not change workflow triggers.

#39 separately restores the build-and-test workflow on the repository's
master branch.

The Python dependency/install failure is also intentionally out of scope and
is tracked separately in #40.

Because the ARM job depends on both the C and Python jobs, ARM CI may remain
blocked until the Python job is healthy.

Why

CI should report what actually happened.

A test suite that was not enabled, a failing CTest invocation, or a failed
cross-build should not be represented as successful.

@srpatcha srpatcha 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.

Approving. This is a better fix than it looks, and it corrects something I got
wrong yesterday.

The option name was doing nothing

CMakeLists.txt:16 defines:

option(EAI_BUILD_TESTS "Build unit tests" OFF)

There is no BUILD_TESTS. So the old workflow's -DBUILD_TESTS=ON set an
unrelated variable and left tests off. Measured:

-DBUILD_TESTS=ON       ->  No tests were found!!!
-DEAI_BUILD_TESTS=ON   ->  100% tests passed, 0 tests failed out of 24

Twenty-four tests that have never run in CI.

Which means my report on #39 was wrong

When I restored the trigger in #39 I wrote that C/C++ Tests passed and only
Python was broken. That pass was worthless — the job built zero tests, ctest
found nothing, and || true swallowed the result:

ctest with zero tests, `|| true`         ->  exit=0   job passes

So eAI had a green C/C++ badge over a suite that was never compiled. Restoring
the trigger made the job run; it took this PR to make it mean something. Both
were needed and I only saw half of it.

--no-tests=error is the right instrument

It is ctest's own answer to exactly the trap that made the old job green:

zero tests, --no-tests=error   ->  exit=8   job fails
24 tests,   --no-tests=error   ->  exit=0   job passes

Worth noting because I hit the same problem in ebuild and solved it by parsing
"No tests were found" out of the output — necessary there, because that command
wraps several runners including ones with no equivalent flag. Where ctest is
invoked directly, --no-tests=error is better: it is upstream's contract rather
than a string match on a message that could be reworded.

The rest

Dropping || true from ctest and 2>/dev/null || true from the ARM build is the
core of "fail closed". The suppressed stderr on the ARM step is the worse of the
two — it hid both the failure and the reason.

The toolchain path is also genuinely wrong on master:

cmake/arm-cortex-m4.cmake                          does not exist
cmake/toolchains/toolchain-arm-cortex-m4.cmake     exists

so that step was failing to configure and being silenced. With the suppression
removed and the path corrected, the ARM cross-build will actually run — and may
well be red first time, like everything else this repository has not been
checking.

Five lines, four independent defects, each verified. Nice work.

@AbdulRehman77777

Copy link
Copy Markdown
Author

Thanks Mr. Srikanth. I really appreciate the detailed review and the explanation around the CI behavior. Glad I could help uncover and fix it.

@srpatcha srpatcha 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.

Review — eAI#41 "ci: make C tests and ARM cross-build fail closed"

head: ef4d249 author: AbdulRehman77777 ci: none ran

Verdict: The diagnosis is right and the ARM half is still needed. The C half has
landed on master independently, which is most of why this now conflicts — rebase and
this becomes a three-line PR.

Findings

# Severity File:line Finding Recommended fix
1 Medium .github/workflows/ci.yml:26-31 The test-c hunk is already upstream. master (abd678b, merge of #33) has -DEAI_BUILD_TESTS=ON and ctest --output-on-failure --no-tests=error. git merge-tree against current master conflicts on exactly this hunk and nothing else: master wrote cd build && ctest --output-on-failure --no-tests=error, this branch wrote ctest --test-dir build --output-on-failure --no-tests=error. The two are equivalent. Rebase, take master's line for test-c, keep only the build-arm hunk.
2 Medium (whole PR) No CI has ever run on this PR. checks.txt is empty. The base branch's ci.yml has pull_request: branches: [main], so a PR against master does not match the trigger — the exact defect #39 fixes. The fail-closed behaviour this PR introduces is therefore unverified by CI, and cannot be verified here until #39 lands. Merge #39 first, then push this so it gets a real run.
3 Low .github/workflows/ci.yml:67 + :70-77 mkdir -p dist && echo "ARM build complete" > dist/build.txt writes an assertion, not evidence. With || true gone and Actions' default bash -e, it is no longer reached after a failed build — that part of the body's reasoning holds. But upload-artifact has no if-no-files-found, which defaults to warn: a configure/build that succeeds while emitting no .elf/.bin still uploads a directory whose only content says the build completed. Add if-no-files-found: error to the upload step and drop dist/build.txt; the artifact itself is the evidence.
4 Low cmake/toolchain-arm-cortex-m4.cmake The repo carries two Cortex-M4 toolchain files. diff shows them byte-identical apart from line endings (cmake/toolchain-arm-cortex-m4.cmake is LF, cmake/toolchains/toolchain-arm-cortex-m4.cmake is CRLF). This PR picks the toolchains/ one, which is the right choice — it sits with the other eight targets and its own usage comment names that path. The stale copy is what will let the two diverge later. Delete cmake/toolchain-arm-cortex-m4.cmake in a follow-up.

On the claims

The Verification section holds up, and the two things it does not claim are the ones
that matter: ARM compilation is marked NOT RUN with the reason (arm-none-eabi-gcc
absent), and the FS_OPS / HAL_CORE / HAL_MEM compile failures on the macOS host are
named rather than hidden. That is the right shape for a fail-closed PR — with || true
gone, if those failures reproduce under gcc-12 on ubuntu-22.04 the test-c job will
fail at the build step and the 24 tests still will not run. That is the intended outcome,
not a regression, but it should not be a surprise on the first green-or-red run.

The toolchain claim checks out: the old workflow named cmake/arm-cortex-m4.cmake, and
no such file exists at any commit on master.

The body's reasoning about shell semantics is correct — GitHub Actions runs run: steps
under bash -e on Linux by default, so a failing cmake aborts the step.

Architecture conformance

Conforms. §21 Tier 3 (eAI, Advanced); CI configuration inside the owning repo, no
dependency edges added, §5.1 untouched. The change moves the repo toward §28's evidence
policy rather than away from it: a job that reports Implemented while compiling zero
tests is the failure mode §28 exists to prevent.

Proposed changes

rebase onto origin/master
  - drop the test-c hunk entirely (already upstream, equivalent form)
  - keep the build-arm hunk:
        -DCMAKE_TOOLCHAIN_FILE=cmake/toolchains/toolchain-arm-cortex-m4.cmake
        -DEAI_BUILD_TESTS=OFF
        cmake --build build/arm --parallel $(nproc)
  - add   if-no-files-found: error   to the upload-artifact step
  - drop  mkdir -p dist && echo "ARM build complete" > dist/build.txt

That leaves a diff of about three lines with no conflict.

Not checked

  • Nothing was built or run. arm-none-eabi-gcc availability, whether the ARM
    cross-build actually succeeds, and whether the 24 C tests pass under gcc-12 are all
    unverified here — the same gaps the author declared.
  • reviewDecision: APPROVED is recorded; I did not look at who approved or on which
    head.

Automated architecture review of ef4d2490d20e — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants