test(containers): make the container fixture able to fail (#108) - #125
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the container CI smoke tests so they can genuinely fail when the container’s scientific / rendering stack is broken, aligning the tests with the repo’s goal of validating published lecture build images (not just “does jupyter-book run”).
Changes:
- Add a new in-container smoke test runner (
smoke-test.sh) with a--self-testmode to ensure the fixture is capable of failing. - Make the minimal Jupyter Book fixture execute real
{code-cell}blocks (with jupytext front matter,execute_notebooks: force, andraise_on_error) and assert on results (numpy/scipy, pandas, matplotlib render, plotly+kaleido export). - Switch
test-container.ymlto run both images as truecontainer:jobs (plus a separate manifest-based compressed size report), and update related docs/changelog.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
TESTING.md |
Clarifies how action-level fixtures differ from container image smoke fixtures. |
containers/quantecon/tests/smoke-test.sh |
New smoke test script for running inside container jobs; adds --self-test inert-fixture guard. |
containers/quantecon/tests/run-local-tests.sh |
Strengthens local replication script (now uses pipefail and validates the Python stack). |
containers/quantecon/tests/minimal-jupyter-book/intro.md |
Converts fixture content into executable {code-cell} tests with assert-based checks. |
containers/quantecon/tests/minimal-jupyter-book/_config.yml |
Forces notebook execution and raises on cell errors to ensure failures are surfaced. |
containers/quantecon/README.md |
Updates container testing documentation to reflect new smoke test and fixture behavior. |
CHANGELOG.md |
Documents the behavioral change: container smoke tests now actually exercise the stack and can fail. |
.github/workflows/test-container.yml |
Runs smoke tests in real container: jobs for both images and reports manifest-based compressed sizes. |
.github/fixtures/mini-lectures/README.md |
Updates fixture documentation to distinguish action-logic testing vs image-contents testing. |
mmcky
force-pushed
the
test/108-container-fixture-can-fail
branch
from
August 5, 2026 06:11
ecf8a68 to
a0733e3
Compare
The container smoke tests could not fail. The fixture was inert for three independent reasons, not the one #108 names: 1. `execute_notebooks: "off"` in _config.yml 2. its one code block was a plain ```python fence, not a {code-cell} 3. no jupytext front matter, so the file was never a notebook at all An image whose numpy, matplotlib or kaleido was completely broken therefore still produced a green container test. Fixing only (1) and (2) would still not be enough: myst-nb reports a failed cell as a *warning*, so a raising cell yields a published book and exit 0 unless `-W` or `raise_on_error` is set. The fixture now executes real cells that ASSERT on results — a stack that imports cleanly but computes wrong answers is exactly what a smoke test should catch, and printing alone would not. Coverage: numpy/scipy eigenvalues against an exact closed form, a pandas groupby, a real matplotlib PNG render, and a plotly static export through kaleido. That last one is the #85 path and the reason both images pin kaleido<1.0. test-container.yml now runs both images as real `container:` jobs instead of `docker run`. Not cosmetic: a container job forces HOME=/github/home while docker run leaves HOME=/root, and that is precisely why #85 passed these tests while failing test-containers-lectures.yml, which does use a container job. The job asserts HOME explicitly so a silent revert is caught, and the lean image gains pdflatex coverage it never had. smoke-test.sh --self-test stages a deliberate exception and fails if the build does NOT go red — because the defect being fixed here is silence, and a fixture that cannot fail reads as coverage. It attributes the red on captured stdout rather than reports/*.err.log: verified locally that with raise_on_error myst-nb aborts before Sphinx writes that file, unlike build-fail-guard's path. Verified locally (minus the plotly cell — kaleido is not in my env): the cells genuinely execute, with real values in the built HTML; the book builds clean under -W with zero warnings; a staged raising cell exits 1; the sentinel appears in captured output; and reports/ is indeed absent. The plotly/kaleido cell and the container-job HOME assertion can only be verified in CI — workflow_dispatch on this branch, since workflow_run only fires for workflows already on the default branch. Image size is now read from the manifest, since a container job cannot run docker. That is COMPRESSED layer bytes — a different, smaller number than the old `docker images` figure, so the two series are not comparable. Refs #108 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three valid points, one of which was my own mess. TESTING.md: my earlier edit replaced a trailing clause and left the sentence it was attached to dangling — an em dash running straight into a capitalised new sentence, then a second em dash. Split into two sentences. run-local-tests.sh: the prereq block ran a bare `python -c`, unlike the three `command -v` checks directly above it. Many macOS setups have only python3, so the script would report a missing science stack when the real problem is no python at all — a misleading diagnostic in the one place whose whole job is diagnosing. Now resolves an interpreter (python3 first) and says which it found. intro.md: the matplotlib cell used np.linspace while importing only matplotlib. It works, because cells share a kernel, but it meant a numpy breakage would surface as a matplotlib-cell failure. Attribution is the point of this PR, so the cell now imports numpy itself. Re-verified locally after the change (minus the plotly cell, kaleido is not in my env): the book still builds clean under -W and the cells still execute, with numpy and matplotlib both reporting real values in the built HTML. Refs #108 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mmcky
force-pushed
the
test/108-container-fixture-can-fail
branch
from
August 5, 2026 06:35
2b80efe to
3f2886f
Compare
This was referenced Aug 6, 2026
Merged
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.
The container smoke tests could not fail. This is the substance of #108, though the fixture was inert for three independent reasons rather than the one the issue names.
Why it was inert
execute_notebooks: "off"in_config.yml```pythonfence, not a{code-cell}grep -rn 'code-cell'over the fixture returned zero hits. An image whose numpy, matplotlib or kaleido was completely broken still produced a green container test.And fixing only 1 and 2 would still not be enough: myst-nb reports a failed cell as a warning, so a raising cell yields a published book and exit 0 unless
-Worraise_on_erroris set. Both are now set — deliberately, because a typo in theraise_on_errorkey is accepted silently by jupyter-book's config validator, and-Wis then the only thing still catching a failing cell.What it tests now
Cells that assert rather than print. A stack that imports cleanly but computes wrong answers is exactly what a smoke test should catch, and printing alone would not.
kaleido<1.0container:jobs, notdocker runThis is the other half, and it is not cosmetic. A GitHub Actions
container:job forcesHOME=/github/home;docker runleavesHOME=/root. That difference is precisely why #85 passed these tests while failingtest-containers-lectures.yml, which does use a container job — kaleido v1 dropped its bundled chromium, and a chromium provisioned under/rootat image build time became unreachable at run time.So both images now run as a real
container:job matrix, and the job assertsHOME=/github/homeexplicitly so a silent revert to a weaker environment is caught. The lean image also gains pdflatex coverage it never had.The self-test
The defect here was silence — a fixture that cannot fail reads as coverage. So
smoke-test.sh --self-teststages a deliberate exception and fails if the build does not go red, and it attributes the red rather than accepting any non-zero (which a bad mount, an OOM, or an unrelated-Wwarning would also satisfy).It greps captured stdout rather than
reports/*.err.log, unlikebuild-fail-guard. Verified: withraise_on_error: truemyst-nb raises before Sphinx writes the report, so that file does not exist here.Verification
Run locally against jupyter-book 1.0.4.post1 / myst-nb 1.4.0, minus the plotly cell (kaleido is not in my local env):
numpy 2.3.5: eigenvalues [1.38 3.62],pandas 2.3.3: group means {a: 2.0, b: 5.0},matplotlib 3.10.6: rendered 12311 byte PNG— all present in the built HTML-Wreports/writtenThe plotly/kaleido cell and the
HOMEassertion can only be verified in CI. I'll triggerworkflow_dispatchon this branch before asking for merge —workflow_runonly fires for workflow files already on the default branch, so that dispatch is the only possible pre-merge gate.Deliberately not in scope
containers/tests/. Left alone: it would dangle at least six doc references, and this PR is already the behavioural change. Worth a follow-up.fontpkg: "". Removing it would engage the FreeSerif/fontspec chain that broke in FreeSerif font not accessible to Jupyter Book in Docker container #3 — real added coverage, but a separate and riskier change that I cannot verify locally.plotly. It is bare incontainers/quantecon-build/environment.yml:49whilekaleidois pinned<1.0. Since this PR makesfig.to_image()load-bearing for CI, adefaults-channel plotly bump past the kaleido v0 window would redden the smoke test with no code change here. Pinning it is an image change and belongs in its own PR — flagging it rather than doing it silently.Refs #108
🤖 Generated with Claude Code