test(hybrid): golden-file end-to-end tests#59
Merged
Conversation
Drive the full hybrid pipeline (builder XML -> builder2ibek -> ibek generate2 -> /ioc_nfs) for two real vacuum IOCs and compare the generated st.cmd/ioc.subst against committed baselines, asserting placement for the stubbed ioc.db/proto/dbd/binary. msi is stubbed (its db templates live only under /dls_sw) and ibek-support-dls-dependent tests skip when that internal submodule is absent. - pin ibek-support / ibek-support-dls submodules to the SHAs tested on a real RTEMS crate (parity / Hy8001 / pass-0 autosave present) - builder2ibek pinned in a CI-only dependency group (temporary git pin at 99108f1 for the unreleased hostlink-interpose fix) - Renovate git-submodules enabled so the pins move forward as reviewable PRs - docs/hybrid-testing.md describes the golden-file regen/review/redeploy loop NOTE: the matching .github/workflows/_test.yml submodule-init step is omitted here because the push token lacks workflow permission; until it is added the hybrid tests will skip in CI (submodules not checked out). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughIntroduces golden-file end-to-end testing for the hybrid pipeline. Submodules are redeclared and pinned via ChangesHybrid mode golden-file e2e testing
Sequence Diagram(s)sequenceDiagram
participant CI as pytest parametrize
participant conftest as conftest fixtures
participant b2i as builder2ibek xml2yaml
participant hybrid as rtems_proxy.hybrid.hybrid_prepare
participant fs as tmp NFS/TFTP filesystem
participant baselines as tests/samples baselines
CI->>conftest: build_tree (stub IOC dirs + artifacts)
CI->>conftest: fake_msi_bin (stub msi script)
CI->>b2i: run_builder2ibek(sample.xml → ioc.yaml)
b2i-->>CI: ioc.yaml
CI->>hybrid: hybrid_prepare(tmp EPICS_ROOT, stubbed msi)
hybrid->>fs: write st.cmd, ioc.subst, ioc.dbd, .req, boot binary
hybrid-->>CI: done
CI->>baselines: diff normalized st.cmd / ioc.subst
CI->>fs: assert NFS runtime files, TFTP binary
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/conftest.py (1)
1-121:⚠️ Potential issue | 🟡 MinorAdd timeout to subprocess.run() and run required Python quality checks before commit.
The
subprocess.run()call at lines 102-106 lacks a timeout, which could cause tests to hang indefinitely ifbuilder2ibekstalls. Add a reasonable timeout value (e.g.,timeout=30).Also run the mandated Python checks per coding guidelines—all
**/*.pyfiles require type checking viauv run pyright src testsoruv run tox -e type-checking, and linting viauv run tox -e pre-commitbefore committing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/conftest.py` around lines 1 - 121, The subprocess.run() call in the run_builder2ibek function lacks a timeout parameter, which could cause tests to hang indefinitely if builder2ibek stalls. Add a timeout parameter (e.g., timeout=30) to the subprocess.run() call to prevent indefinite hangs and ensure tests fail gracefully if the subprocess takes too long. Additionally, before committing, run type checking via uv run pyright src tests and linting via uv run tox -e pre-commit as mandated by the project's coding guidelines to ensure code quality.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/hybrid-testing.md`:
- Around line 71-73: The documentation in the hybrid-testing.md file describes
how Renovate opens PRs that run tests on submodule bumps, but this is misleading
because the required submodule-init step in .github/workflows/_test.yml is still
missing, causing the hybrid tests to skip in CI. Add an explicit caveat or note
immediately after the statement about Renovate running tests to clarify that
this coverage is not yet active in CI due to the missing submodule-init step, so
readers understand the current limitation and don't assume the tests are
actually running.
In `@tests/conftest.py`:
- Around line 102-109: The subprocess.run call in the builder2ibek helper
function lacks a timeout parameter, which can cause test jobs to hang
indefinitely if the builder2ibek process stalls. Add a timeout=120 parameter to
the subprocess.run call to enforce a 120-second limit on the conversion process.
Additionally, wrap the subprocess.run call in a try-except block to catch
subprocess.TimeoutExpired exceptions and handle them appropriately by failing
the assertion with a clear error message indicating that the builder2ibek
process timed out for the given XML file.
In `@tests/samples/BL04I-VA-IOC-01.xml`:
- Line 136: Fix the typo in the DESC attribute of the
interlock.overrideRequestIndividual element where "Overrride" (with three
consecutive r's) should be corrected to "Override" (with a single r). After
fixing the typo in the XML source, regenerate the sample output artifacts to
ensure the corrected text is propagated to all generated files like the
.ioc.subst file.
In `@tests/test_hybrid.py`:
- Around line 46-50: Add an assertion to validate that the expected_subst
baseline file exists before it is read, similar to the existing assertion for
expected_stcmd. The assertion should check expected_subst.exists() with a clear
error message indicating the missing baseline file. This validation needs to be
added at the current location where the expected_stcmd assertion is present, and
also at the other similar location later in the file where the same pattern
applies.
---
Outside diff comments:
In `@tests/conftest.py`:
- Around line 1-121: The subprocess.run() call in the run_builder2ibek function
lacks a timeout parameter, which could cause tests to hang indefinitely if
builder2ibek stalls. Add a timeout parameter (e.g., timeout=30) to the
subprocess.run() call to prevent indefinite hangs and ensure tests fail
gracefully if the subprocess takes too long. Additionally, before committing,
run type checking via uv run pyright src tests and linting via uv run tox -e
pre-commit as mandated by the project's coding guidelines to ensure code
quality.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b9e16c8b-88e0-44a9-b296-96a8a4fb24ee
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
.gitmodulesdocs/hybrid-testing.mddocs/hybrid.mdibek-supportibek-support-dlspyproject.tomlrenovate.jsontests/__init__.pytests/conftest.pytests/samples/BL04I-VA-IOC-01.xmltests/samples/BL19I-VA-IOC-01.xmltests/samples/bl04i-va-ioc-01.ioc.substtests/samples/bl04i-va-ioc-01.st.cmdtests/samples/bl19i-va-ioc-01.ioc.substtests/samples/bl19i-va-ioc-01.st.cmdtests/samples/make_samples.shtests/test_hybrid.py
The hybrid tests can't run on a public GitHub runner: ibek-support-dls is on
internal GitLab, and the builder2ibek git pin makes uv clone builder2ibek's
*own* vendored ibek-support-dls submodule, which a public runner can't reach.
So make them internal-only and skip cleanly elsewhere:
- builder2ibek moves out of the default install (no [tool.uv] default-groups);
it stays in the non-default `ci` group, so the default `tox -e tests` never
installs it and never triggers the transitive submodule clone.
- gate the hybrid tests on `requires_builder2ibek` as well as `requires_dls`,
so they skip when either is absent. Run them internally with:
git submodule update --init && uv run --group ci pytest tests/test_hybrid.py
- document where the tests run in docs/hybrid-testing.md.
Net: public CI runs test_cli/test_globals (hybrid skipped); the hybrid canary
runs on a DLS-internal runner / devcontainer.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…bmodules) The bl19i-va-ioc-01 work is now merged to main in both ibek-support repos and builder2ibek 2.1.0 (with the hostlink-interpose fix) is on PyPI, so retire the bridge pins: - builder2ibek: git@99108f1 -> ==2.1.0 (a flat wheel, no submodules) - ibek-support / ibek-support-dls submodules: tested-branch SHAs -> main Regenerated baselines are byte-identical (main == the tested branch + the released fix), confirming the steady state. The tests still run only on a DLS-internal runner / devcontainer because ibek-support-dls is internal. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The hybrid IOC tests need DLS-internal GitLab (ibek-support-dls) and are skipped on public CI, so local runs give real coverage. Add a verbose, always-passing pre-commit hook that prints a reminder on every commit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Hybrid mode golden-file end-to-end tests
Drives the full hybrid pipeline for two real vacuum IOCs and compares against committed baselines:
st.cmd+ioc.subst(what ibek decides about boot + records).msi(its db templates live only under/dls_sw), plus thedbd/proto/binary inputs — for those we assert placement only.requires_dlsskips the tests when the internalibek-support-dlssubmodule isn't checked out.Pins (bridge state)
ibek-support/ibek-support-dlssubmodules → the SHAs tested on a real RTEMS crate (parity / Hy8001 / pass-0 autosave present).builder2ibek→ CI-only dependency group, temporary git pin at99108f1for the unreleased hostlink-interpose fix.git-submodulesenabled so the pins move forward as reviewable PRs.Docs
docs/hybrid-testing.mdexplains the golden-file model and the regenerate → review → redeploy loop.Follow-ups
_test.ymlsubmodule-init step (needs a token with workflow permission). Until then the hybrid tests skip in CI —test_cli/test_globalsstill run.builder2ibek→ swap the git pin for a PyPI version.bl19i-va-ioc-01branch →mainin both ibek-support repos → repoint submodules tomainand re-runmake_samples.sh(expected no-op).🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Documentation
Tests
st.cmd/ioc.substagainst committed baselines, including expected NFS/TFTP placement.Chores