Skip to content

ci: run the native CMake test suite - #26

Draft
srpatcha wants to merge 1 commit into
masterfrom
fix/ci-native-cmake-tests
Draft

srpatcha wants to merge 1 commit into
masterfrom
fix/ci-native-cmake-tests

Conversation

@srpatcha

@srpatcha srpatcha commented Sep 9, 2026

Copy link
Copy Markdown
Member

Summary

  • replace the inapplicable Python test matrix with the repository's native CMake/CTest suite
  • run all 20 registered tests on Linux with the production mbedTLS backend enabled
  • retain compile coverage on Linux, macOS, and Windows
  • remove duplicate release publishing from CI; the dedicated release workflow remains responsible for packaging
  • grant only read access to repository contents

The previous workflow failed during actions/setup-python because this C repository has neither requirements.txt nor pyproject.toml, so no project test executed.

Validation

  • YAML parse: PASS
  • Debug CMake build with BUILD_TESTING=ON, mbedTLS enabled, standalone disabled: PASS
  • CTest: 20/20 passed
  • Release CMake build: PASS
  • git diff --check: PASS
  • Independent review: zero issues after adding explicit Debug/Release configuration for multi-config generators

Fixes #29

@srpatcha srpatcha left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Review — eBrowser#26 "ci: run the native CMake test suite"

head: 8194c5a author: srpatcha ci: pass (7 checks green) draft

Verdict: The core move is right and overdue — the old matrix died at pip install -r requirements.txt on a repository that has no such file, so nothing it claimed to run ever ran, and replacing it with a real CMake/CTest job that uses --no-tests=error is the correct fix. But the diff does not only replace the broken Python job; it deletes Python testing from this repository entirely. There are 26 Python tests that pass today in 0.07 seconds with nothing but pytest installed, README.md:99 documents how to run them, and after this PR no workflow in the repo runs pytest at all. That is a weakened check, and the brief is explicit that it is a finding regardless of the reason given.

Findings

# Severity File:line Finding Recommended fix
1 High .github/workflows/ci.yml, removed test job The Python suite loses all CI coverage. Verified: pytest tests/ -q on this head gives 26 passed in 0.07s in a bare venv with only pytest installed — tests/unit/test_unit_core.py, tests/functional/test_functional_e2e.py, tests/performance/test_performance_benchmarks.py, tests/simulation/test_emulation_simulation.py. These are live and green, not vestigial. Verified: grep -l pytest .github/workflows/*.yml returns nothing after this change — video-build.yml, release-smoke-test.yml and release.yml mention python but none of them runs pytest. And README.md:99 documents python run_all_tests.py # runs: pytest tests/unit tests/functional tests/performance tests/simulation -v as a supported entry point, so this is a documented surface going dark. The old job's actual defect was one linepip install -r requirements.txt against a file that does not exist. Deleting the job deletes 26 working tests' coverage to fix a missing requirements file. Keep the native job exactly as written and add a small second job beside it, not in place of it: actions/setup-python@v5pip install pytestpython -m pytest tests/ -q. No requirements.txt, no -r, no continue-on-error. That is four lines and it restores everything this diff drops. Drop mypy/ruff if you like — they genuinely had nothing to check and were continue-on-error: true anyway — but the tests are a different case.
2 Low codecov.yml The diff removes the only codecov/codecov-action upload, and codecov.yml is left in place declaring status.project.default.target: 100% and status.patch.default.target: 100% with require_ci_to_pass: yes. Nothing will ever produce a report for those gates to evaluate again. Brief item 11 — a change that makes existing configuration wrong is not finished. Either restore a coverage upload as part of finding 1's Python job, or delete codecov.yml in this PR and say so in the body. Leaving a 100% coverage gate wired to no producer is the worst of the three.
3 Low .github/workflows/ci.yml, build job The three-OS matrix runs --config Release with BUILD_TESTING=OFF, so ctest never executes on macOS or Windows. The -C Debug / --config Debug handling that the body credits the independent review with adding for multi-config generators therefore runs only on ubuntu-22.04, where the generator is single-config and -C is ignored. The one thing that change exists for is the one platform it is never exercised on. Cheapest: add -DBUILD_TESTING=ON and a ctest --test-dir build -C Release --output-on-failure --no-tests=error step to the matrix job. That also gets the C suite onto Windows and macOS, which is the coverage the body says it is retaining but is currently compile-only.
4 Low .github/workflows/ci.yml:5-7 on.push.tags: ["v*"] is retained but the release job it existed to trigger is gone. A tag push now runs test and build for nothing — release.yml already builds Linux, .deb and AppImage on the same tag, and it runs its own tests (release.yml:56). Drop tags: ["v*"] from this workflow's push trigger. The removal of the release job itself is correct and verified — release.yml does own packaging.

What is right, and verified

Stated because most of this diff is an improvement and should not get lost in the findings:

  • The premise holds. No requirements.txt, no pyproject.toml, no setup.py anywhere in the tree. The old pip install -r requirements.txt could only fail, which means Run unit tests and Run functional tests never executed, and Build Python Package (python -m build) could never have produced a wheel either. Four jobs' worth of green that verified nothing.
  • --no-tests=error is present on the new ctest step. .ai/security.md names the exact failure this prevents: "ctest exits 0 when it finds no tests at all. A security suite that silently collected nothing is a failed check reported as green." Getting this right unprompted is the single best line in the diff.
  • Three continue-on-error: true steps are gone (ruff, mypy, the benchmark run). Those are fail-open by construction and the diff is right to remove them rather than carry them forward.
  • permissions: contents: read added at workflow level, consistent with the release job's removal — nothing left in this workflow needs write.
  • Removing the release job is justified, not just asserted. release.yml exists, triggers on tag push, declares contents: write/packages: write/id-token: write, and has build-linux producing .deb and AppImage artifacts with its own Run tests step. The body's claim that "the dedicated release workflow remains responsible for packaging" is true.
  • mbedTLS is not lost on the matrix legs. I checked, because the test job passes -DeBrowser_USE_MBEDTLS=ON and the build job does not: CMakeLists.txt:34 declares option(eBrowser_USE_MBEDTLS "Use mbedTLS for real TLS support" ON), so the default is ON and all three OS legs still compile the production TLS backend. The explicit flag on the test job is redundant but harmless, and being explicit about the configuration under test is the right instinct.

Architecture conformance

Conforms, with one placement observation that is not this PR's to resolve.

  • §21 / Tier 5. eBrowser is Tier 5 — Applications ("Reference/advanced applications"), and §20.1 is explicit that "eBrowser and eOffice should be treated as reference or advanced applications proving platform capability, not as peer pillars of the kernel." A CI workflow change in a Tier-5 repository engages no tier rule.
  • §5.1. Not engaged. The diff is one workflow file: no #include, no import, no link line, no target_link_libraries, no manifest entry. Nothing points up or down a tier. The mbedTLS dependency is pre-existing and is a third-party library, not an EmbeddedOS tier.
  • §14.1, "Use reviewed cryptographic libraries; do not invent cryptographic primitives": CMakeLists.txt:110-115 FetchContents mbedTLS from upstream, which is the right shape. Out of scope here, but worth recording that the fetch is by GIT_REPOSITORY — provenance of that dependency is a §11 question this PR does not touch and I did not audit.
  • .github/STANDARDS.md, "A claim without (2) and (3) is aspirational, not asserted": finding 2 is the local instance — a 100% coverage target with no verifying workflow is aspirational, and codecov.yml does not label it as such.
  • No architecture proposal appended. Nothing here shows the master design wrong, stale or silent; this is a repository-local CI gap.

Proposed changes

Smallest sequence that keeps everything working:

  1. Add a python-tests job beside testsetup-pythonpip install pytestpython -m pytest tests/ -q. No requirements.txt reference, no continue-on-error (finding 1).
  2. Decide codecov.yml's fate in the same PR: coverage upload restored, or the file deleted (finding 2).
  3. Turn BUILD_TESTING=ON and add a ctest step to the three-OS matrix, so the multi-config path is actually exercised (finding 3).
  4. Drop tags: ["v*"] from the push trigger (finding 4).

1 and 2 are what I would hold the draft for. 3 and 4 are improvements.

Not checked

  • No build or test of the C suite was run. I did not configure or build eBrowser. The body's "Debug CMake build … PASS", "CTest: 20/20 passed" and "Release CMake build: PASS" are unverified here — building would FetchContent mbedTLS from GitHub, and I did not do that. The PR's own green Native C tests and three Native build checks are the evidence for those claims, not anything I ran. Note the count discrepancy worth confirming: the body says 20 registered tests, and I did not count add_test entries to check it.
  • Windows and macOS — NOT RUN. Finding 3 is reasoning about which steps execute where, from the YAML. I did not run either leg.
  • The 26 Python tests were run, but only on Linux with the venv's CPython, not across the 3.10/3.11/3.12 range the old matrix nominally covered. They pass; I did not check what they actually assert or whether they are meaningful tests, only that they exist, run, and are green — which is what finding 1 turns on.
  • Independence. The PR author is the same account that operates this autoreview pipeline. .ai/reviewer.md says an implementer does not approve their own work; this pipeline never approves anything and this comment is not an approval, but a reader should not count it as independent human review either.
  • Draft. This PR is marked draft and was last updated 2026-09-10. Findings 1 and 2 may already be intended before it is marked ready; they are recorded rather than assumed.
  • I did not audit the other 15 workflows for overlap with the new jobs beyond grepping them for pytest and reading release.yml's job list.

Automated architecture review of 8194c5a40edb — 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.

This branch has not been deployed

No deployments
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.

Run the native CMake test suite in pull-request CI

1 participant