test: skip GCC-dependent test when compiler is unavailable - #114
test: skip GCC-dependent test when compiler is unavailable#114chandupatel-ai wants to merge 3 commits into
Conversation
srpatcha
left a comment
There was a problem hiding this comment.
Review — ebuild#114 "test: skip GCC-dependent test when compiler is unavailable"
head: 6c6166a author: chandupatel-ai ci: none reported
Verdict: Correct one-line change that matches the convention already used elsewhere in
this suite, but it does not reach the stated objective — the suite still fails without gcc,
by the author's own numbers — and no CI run exists to confirm anything.
The substitution itself is right. subprocess.run(["gcc", ...]) raises FileNotFoundError
when gcc is absent, and because the call sits in the skipif argument it is evaluated at
import time, so the exception surfaces as a collection error rather than a skip.
shutil.which("gcc") is None is the pattern tests/ebuild/test_ninja_backend.py:121
already uses (shutil.which("cc") is None and shutil.which("gcc") is None), so this makes
the two consistent rather than introducing a new idiom.
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Medium | tests/unit/test_footprint.py:66 (master) | TestMeasure::test_measures_a_real_binary runs subprocess.run(["gcc", str(src), "-o", str(exe)], check=True) with no guard of any kind. The PR body records this as the one remaining failure ("553 passed, 7 skipped, 1 failed") and leaves it. The PR's stated goal is that the suite behaves correctly where gcc is not installed; after this change it still does not. Verified against origin/master; no other open ebuild PR (#103–#113) touches this file. |
Add the same guard to the test: @pytest.mark.skipif(shutil.which("gcc") is None, reason="needs gcc to produce a binary to measure"), importing shutil in that module. One skipif, and the suite is green without a compiler. |
| 2 | Low | tests/ebuild/test_build_dir_resolution.py:246–248 | The predicate and the reason no longer agree. reason="needs a working gcc to link the executable" but shutil.which only proves a file named gcc is on PATH. A gcc that is present and broken previously skipped (non-zero --version); now it runs the test and fails. Running is arguably the better behaviour — a broken toolchain should be loud — but the reason string states a check that is no longer performed. |
Reword to reason="needs gcc on PATH to link the executable", so the text describes the predicate that is actually evaluated. |
| 3 | Low | — | No independent evidence. gh pr checks 114 --repo embeddedos-org/ebuild returns "no checks reported on the 'fix/gcc-test-skip' branch", although .github/workflows/ci.yml:9 declares pull_request: branches: [master, main] and this PR targets master. mergeStateStatus is BLOCKED. The only evidence for the change is the pass/skip counts typed into the PR body — no command output, no CI run. |
Most likely first-time-contributor workflow approval is pending; a maintainer approving the run is enough. If CI stays absent, that is a repo-level defect worth its own issue, not this author's. |
Architecture conformance
ebuild is Tier 1 — Foundation (§21). The change is confined to tests/, adds one
stdlib import, and introduces no #include, import, link line or manifest entry, so §5.1
is not engaged in either direction. §9's rule that eBuild is the developer control plane
and not a runtime dependency is untouched. Conforms.
Worth noting for the record: CI runs ubuntu-22.04, macos-latest and windows-2022
(ci.yml:30), and gcc resolves on all three, so this defect never reddened CI — it only
broke local runs on machines without a compiler. That is still worth fixing; §25.2 puts
"one-command environment diagnosis" and a working local loop on the MLP path, and a suite
that cannot be collected on a fresh machine is squarely in that column.
Proposed changes
- In
tests/unit/test_footprint.py, addimport shutiland decorate
test_measures_a_real_binarywith
@pytest.mark.skipif(shutil.which("gcc") is None, reason="needs gcc on PATH to build the binary under test").
That closes finding 1 and makes the PR's claim true. - Reword the
reasonin the hunk under review per finding 2. - Re-run
python -m pytest tests/ -qon a machine with no gcc onPATHand paste the
summary line into the PR body, replacing the current prose count.
Not checked
- The change was not executed. The local
ebuildclone is on branchv90with a dirty
tree (4 files) and was skipped by the sync step; head6c6166a8is not fetched locally,
so no worktree at this PR's tree exists to runpytestin. Findings above come from
reading the diff againstorigin/master, not from a run. - Windows and macOS behaviour of
shutil.which("gcc")was not exercised, only reasoned
about from the stdlib contract (PATHEXThandling on Windows). - The claim "553 passed, 7 skipped, 1 failed" was not reproduced; finding 1 was verified
by readingorigin/master:tests/unit/test_footprint.py, which independently supports it.
Automated architecture review of 6c6166a8d765 — 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.
SummaryAddressed the requested review changes.
TestingFull test suite on Windows without GCC: |
srpatcha
left a comment
There was a problem hiding this comment.
Review — ebuild#114 "test: skip GCC-dependent test when compiler is unavailable"
head: cc47987 author: chandupatel-ai ci: none reported (bundle checks.txt is empty; no workflow has run on this head)
Verdict: Correct, minimal, and it fixes more than the title claims — I reproduced both halves rather than taking the numbers on trust. On the base commit e5d8052 with gcc off PATH, pytest tests/ does not merely fail one test: tests/ebuild/test_build_dir_resolution.py:246 evaluates subprocess.run(["gcc", ...]) at import time, so collection raises FileNotFoundError and pytest exits with Interrupted: 1 error during collection — all 561 tests never run. On this head the same command gives 556 passed, 5 skipped, 0 failed. That is 556 tests recovered on every runner without a host compiler, i.e. the three windows-2022 jobs in ci.yml's matrix. shutil.which is also the convention already used at tests/ebuild/test_ninja_backend.py:132 and tests/ebuild/test_integration_initramfs_security.py:38, so this aligns the outlier rather than inventing a pattern. Two small gaps below; neither blocks the fix.
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Low | tests/unit/test_footprint.py:60-63 |
The new guard covers gcc but not the other external tool this test needs. measure() calls find_size_tool(), so on a host with a compiler and no size the test still hard-fails with the same class of environment error the PR exists to remove. Executed on this head with gcc present and size masked off PATH: FAILED TestMeasure::test_measures_a_real_binary — FootprintError: no 'size' tool on PATH, so the footprint cannot be measured (install binutils, ...). This is not hypothetical: the author of ebuild#109 has reported both failure modes on their own host across two rounds ("test_footprint needs an external size binary", later "needs system gcc"), so the half you guarded and the half you did not have both been hit in this org. |
One predicate, using a symbol the file already imports at line 30: shutil.which("gcc") is None or find_size_tool() is None. Check find_size_tool's contract first — it may raise rather than return None, in which case wrap it or reuse the module's own probe. Fold the reason string into one sentence naming both tools so the skip says which one was missing. |
| 2 | Low | tests/unit/test_footprint.py:60 |
Recorded because the brief treats a newly-skipped test as a finding regardless of the reason given: TestMeasure::test_measures_a_real_binary was unconditional and is now conditional. Measured, so the cost is stated rather than assumed — base at e5d8052, tests/unit only, gcc masked: 1 failed, 345 passed, the failure being this test; head, same conditions: it skips. So the change converts a hard failure into a skip on hosts without a compiler and alters nothing on hosts with one. Against ci.yml's matrix (ubuntu-22.04, macos-latest, windows-2022 × py3.10/3.11/3.12) the assertion still executes on the six jobs that ship a compiler and is skipped on the three windows-2022 jobs, where it previously could only fail. No measured coverage is lost; what is lost is any signal if the skip ever starts firing on Linux too, because nothing asserts an expected skip set. |
No change requested — the guard is the right call for a host-toolchain dependency and matches the repo's convention. If you want the residual risk closed, the cheap version is an assertion in the ubuntu-22.04 CI job that this test ran rather than skipped; that is a CI change, not a test change, and belongs in its own PR. |
Architecture conformance
Conforms; no architectural surface is touched. Both files are under tests/, no production module changes (the PR body's "No production code was changed" is accurate — files.txt lists exactly two test files, 3+ 2- and 5+ 1-). No #include, import, link line or manifest entry is added, so §5.1's dependency direction is unaffected, and the only new imports are stdlib shutil in two test modules. §21 tier placement is correct: ebuild is Tier 1 - Foundation and this is Tier-1 test hygiene in the Tier-1 repo. Relevant to the design only in that it serves §17's CI contract and §25.2's "one-command environment diagnosis" — a suite that aborts at collection on a supported developer host is the opposite of both.
Proposed changes
- Add the
sizepredicate to thetest_footprint.pyguard (Finding 1). One line. - Optional tidy in the file you are already editing: the diff removes the second blank line before
class TestMeasure:, leaving one where PEP 8 asks for two. CI does not care — verifiedruff check --select=E,F,W --ignore=E501(the exact selectionci.yml:52uses) passes on both changed files, because ruff'sEset does not enable the blank-line rulesE301–E306outside preview mode. Restoring the line costs nothing and keeps the diff to its subject.
Nothing else. Do not widen this PR.
Not checked
- CI: nothing ran.
checks.txtis empty for this head andpr.jsoncarries no check rollup, so none of the repo'spull_request-triggered workflows have produced a result oncc479879. The claim that matters here is inherently platform-specific and cannot be confirmed from CI output that does not exist. - Windows was not tested. Your
553 passed, 8 skippedis from a Windows host without GCC and I have no Windows runner. I reproduced the equivalent scenario on Linux by maskinggccoffPATHand got556 passed, 5 skipped— three fewer skips and three more passes, consistent with the Windows-only skips in the tree (e.g.test_integration_initramfs_security.py:190,os.name == "nt"), so your figure is plausible but is neither confirmed nor disputed. Your targeted figure I did reproduce exactly:pytest tests/ebuild/test_build_dir_resolution.pywithgccabsent →7 passed, 1 skipped. - What I ran, so it is not mistaken for CI: full
tests/on Linux withgcc+ninjapresent →561 passed, 0 failed, 0 skipped; fulltests/withgccmasked →556 passed, 5 skipped; basee5d8052withgccmasked → collection error, 0 tests run; basetests/unitwithgccmasked →1 failed, 345 passed;tests/unit/test_footprint.pywithsizemasked →1 failed, 42 passed; ruff withci.yml's selection on both changed files → clean. Scratch checkout of the head tree, not the working-root clone, which is dirty and was left untouched by the sync step. - Not assessed: whether
macos-latestprovidesgcconPATH. Apple's Command Line Tools install agccshim over clang, so the guard most likely does not skip there, but I did not verify it and the PR's coverage claim on macOS therefore rests on an assumption, not a measurement. - Not assessed: the other two
subprocess-using test modules for the same import-time pattern. I grepped everyskipifintests/and the remaining three predicates areshutil.whichoros.namechecks with no subprocess call, sotest_build_dir_resolution.py:246was the only instance — but that is a grep overskipifoccurrences, not a proof that no other module executes an external tool at import time.
Automated architecture review of cc479879c140 — 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.
Summary
Fixes test collection failures on environments where GCC is not installed.
The end-to-end build test previously invoked
gcc --versiondirectly while evaluating itspytest.mark.skipifcondition. When GCC was unavailable,subprocess.run()raisedFileNotFoundError, causing test collection to fail instead of skipping the compiler-dependent test.This change uses
shutil.which("gcc")to detect whether GCC is available before running the test.Testing
Targeted test: