Skip to content

Add non-blocking Loom tests to CI (#55) - #154

Merged
leynos merged 10 commits into
mainfrom
issue-55-add-loom-tests-to-ci-non-blocking-initially
Jul 2, 2026
Merged

Add non-blocking Loom tests to CI (#55)#154
leynos merged 10 commits into
mainfrom
issue-55-add-loom-tests-to-ci-non-blocking-initially

Conversation

@lodyai

@lodyai lodyai Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Summary

This branch adds the existing Loom concurrency suite to CI as an advisory,
time-capped check for issue #55. It adds a canonical make test-loom entry
point, documents the scheduler budget that keeps the Loom model tractable, and
runs the suite in the unprivileged CI path without blocking pull requests while
coverage is still being expanded.

Closes #55.

Review walkthrough

  • Start with Makefile to see the new test-loom target and confirm it remains out of all.
  • Then review docs/developers-guide.md for the canonical local command and the documented scheduler-bound constraint.
  • Check src/env/loom_tests.rs for the in-code note around the current Loom builder bounds.
  • Finish with .github/workflows/ci.yml to verify the unprivileged, non-blocking, one-minute CI step.

Validation

  • mbake validate Makefile: passed.
  • make check-fmt: passed.
  • make markdownlint: passed.
  • make nixie: passed.
  • make lint: passed.
  • make test: passed.
  • make test-loom: passed.
  • make help: showed test-loom.
  • actionlint .github/workflows/ci.yml: passed.
  • coderabbit review --agent: passed with 0 findings after the Makefile/docs milestone.
  • coderabbit review --agent: passed with 0 findings after the CI milestone.

Notes

The second CodeRabbit review initially hit the free CLI rate limit. The agent
waited with vsleep for the requested randomized interval and retried the
review successfully.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry @LodyAI[bot], you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@coderabbitai

coderabbitai Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Add a test-loom Makefile target, run it in CI for the unprivileged matrix entry with a 1-minute timeout, and update the developers guide plus inline Loom comments and test docstrings to record the scheduler bounds.

Changes

Loom CI integration

Layer / File(s) Summary
test-loom target and lint inputs
Makefile
Add test-loom to .PHONY, define the target for cargo test --features "loom-tests" --lib -- --ignored, and parameterise interrogate with docstring coverage defaults.
CI workflow updates
.github/workflows/ci.yml
Install interrogate with uv and add a non-blocking Loom test step for the unprivileged matrix entry with make test-loom, always(), and a 1-minute timeout.
Loom documentation and comments
docs/developers-guide.md, src/env/loom_tests.rs, tests/test_workflow_integration.py
Replace the raw Loom command with make test-loom and add documentation comments for the Loom lock, scope helpers, model bounds, nested-scope test, and workflow helpers.

Possibly related issues

  • #54: Matches the Loom CI, Makefile, and bounded Loom execution changes in this PR.

Possibly related PRs

Suggested labels

Issue

Spin the Loom, keep it light,
Three threads bound the model tight.
make test-loom joins the rite,
CI watches, non-blocking, bright.
Bounds and notes now line the page,
Loom hums steadily on stage.

🚥 Pre-merge checks | ✅ 19 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning The PR also pins uv/interrogate and adds doc-only updates outside #55's Loom CI scope. Split the uv/interrogate pinning and other non-Loom edits into a separate PR, and keep this change limited to the Loom target and CI step.
✅ Passed checks (19 passed)
Check name Status Explanation
Title check ✅ Passed The title matches the PR's main change and correctly references issue #55.
Description check ✅ Passed The description is clearly about adding Loom tests to CI and stays on topic.
Linked Issues check ✅ Passed The PR adds the test-loom target, wires it into CI with a timeout, and keeps it non-blocking as requested for #55.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Testing (Overall) ✅ Passed The PR only wires existing Loom checks into CI; those tests exercise real ScopedEnv serialisation and reentrancy, so the added coverage is non-vacuous.
User-Facing Documentation ✅ Passed PASS: The PR only adds test/CI tooling (make test-loom, CI step) behind cfg(test); no user-facing behaviour changes need a users-guide update.
Developer Documentation ✅ Passed PASS: docs/developers-guide.md covers make test-loom and Loom bounds; roadmap 3.2.1 is checked off; the Loom execplan and design doc are up to date.
Module-Level Documentation ✅ Passed PASS: src/env/loom_tests.rs and tests/test_workflow_integration.py both start with module docstrings explaining purpose; other changed files are not modules.
Testing (Unit And Behavioural) ✅ Passed PASS: the PR adds CI wiring only; the Loom suite already exercises serialisation and re-entrancy invariants, with no disguised unit tests introduced.
Testing (Property / Proof) ✅ Passed Use the existing Loom bounded model checks for the concurrency invariant; the PR adds them to CI and introduces no proof assumptions.
Testing (Compile-Time / Ui) ✅ Passed No compile-time or UI-facing behaviour changed; the PR only adds CI/Makefile wiring and docs, so trybuild/snapshot coverage is not applicable.
Unit Architecture ✅ Passed Only docs, CI, and Makefile wiring changed; no query/command boundary, state, or dependency-visibility regressions appear in code.
Domain Architecture ✅ Passed Keep the change confined to CI, docs, and test-only Loom scaffolding; the domain model and adapter boundaries stay untouched.
Observability ✅ Passed No production operational behaviour changed; the PR only adds CI/docs/test wiring, so extra logging, metrics, tracing, or alerts are not required.
Security And Privacy ✅ Passed No secrets, auth changes, or privacy-sensitive data were added; the PR only adds test tooling, a non-blocking CI test, and documentation/comments.
Performance And Resource Use ✅ Passed The new Loom lane is bounded by explicit scheduler limits and a 1-minute CI cap; no unbounded loops, hot-path work, or repeated I/O were introduced.
Concurrency And State ✅ Passed Loom checks model concurrent and re-entrant scopes with explicit mutex/thread-local state, and CI now runs them non-blockingly with a hard timeout.
Architectural Complexity And Maintainability ✅ Passed PASS: It reuses the existing ThreadStateCore/EnvLockOps seam and adds only a small test-only harness, Makefile target, and CI/docs wiring—no new architectural layer or hidden lifecycle.
Rust Compiler Lint Integrity ✅ Passed No new dead_code/unused suppressions or anchor helpers; the Rust changes only add tests/docs and intentional Arc::clone/owned conversions.
📋 Issue Planner

Built with CodeRabbit's Coding Plans for faster development and fewer bugs.

View plan used: #55

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch issue-55-add-loom-tests-to-ci-non-blocking-initially

Comment @coderabbitai help to get the list of available commands.

codescene-access[bot]

This comment was marked as outdated.

@pandalump
pandalump marked this pull request as ready for review June 27, 2026 21:50
@coderabbitai coderabbitai Bot added the Issue label Jun 27, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 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 @.github/workflows/ci.yml:
- Around line 52-56: The Loom Concurrency Tests step is currently gated only on
matrix.privilege, so it gets skipped after earlier failures instead of remaining
a non-blocking advisory signal. Update the workflow condition on the Loom
Concurrency Tests (non-blocking) step to use always() together with the existing
unprivileged check, so the step still runs even when prior steps fail.

In `@docs/developers-guide.md`:
- Around line 48-52: Update the scheduler-budget note in the developers guide to
describe all three Loom bounds together, not just preemption_bound. In the
section referencing src/env/loom_tests.rs, revise the wording so max_threads,
max_branches, and preemption_bound are presented as jointly constraining the
search space, while still noting that changing any of them may require
justification and CI timeout adjustments.
🪄 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: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: f3db5f07-5fe4-4f3f-bd52-0d9024476357

📥 Commits

Reviewing files that changed from the base of the PR and between 0e28952 and 2e267e3.

📒 Files selected for processing (4)
  • .github/workflows/ci.yml
  • Makefile
  • docs/developers-guide.md
  • src/env/loom_tests.rs
🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

  • leynos/cuprum (auto-detected)

Comment thread .github/workflows/ci.yml
Comment thread docs/developers-guide.md Outdated
codescene-access[bot]

This comment was marked as outdated.

codescene-access[bot]

This comment was marked as outdated.

@lodyai
lodyai Bot marked this pull request as draft June 28, 2026 23:17
@pandalump
pandalump marked this pull request as ready for review June 29, 2026 10:47
@leynos

leynos commented Jun 29, 2026

Copy link
Copy Markdown
Owner

@coderabbitai resume

@coderabbitai

coderabbitai Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Reviews resumed.

codescene-access[bot]

This comment was marked as outdated.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 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 `@tests/test_workflow_integration.py`:
- Line 20: The docstring for run_act is out of sync with its return contract and
omits the artefact path. Update the run_act docstring so it explicitly states
that the function returns an exit code, an artefact path, and logs, keeping the
wording aligned with the implementation and the test_workflow_integration
helper.
🪄 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: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: d2ce7a44-cdbc-4370-8287-b5ddd913a3a5

📥 Commits

Reviewing files that changed from the base of the PR and between 225309d and 4514011.

📒 Files selected for processing (1)
  • tests/test_workflow_integration.py
🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

  • leynos/cuprum (auto-detected)
  • leynos/rstest-bdd (auto-detected)

Comment thread tests/test_workflow_integration.py Outdated
codescene-access[bot]

This comment was marked as outdated.

codescene-access[bot]

This comment was marked as outdated.

codescene-access[bot]

This comment was marked as outdated.

@pandalump

Copy link
Copy Markdown
Collaborator

@coderabbitai resume

@coderabbitai

coderabbitai Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Reviews resumed.

leynos and others added 8 commits June 29, 2026 17:35
Add `make test-loom` as the canonical entry point for the ignored
Loom suite. Document the current scheduler budget and the reason the
preemption bound should not be raised without justification.

Keep the target out of `all` so Loom remains opt-in locally until CI
coverage is ready to become blocking.
Add the existing Loom suite to the unprivileged CI path after coverage.
Keep the step informational with `continue-on-error` and cap it at one
minute so expanded model scenarios fail fast instead of consuming a CI
run indefinitely.
Add Rustdoc comments to the Loom-specific helper symbols and model tests.
This keeps the changed test module understandable and gives docstring
coverage tools real documentation to count on the branch delta.
Keep the advisory Loom CI step running after earlier unprivileged
failures by combining `always()` with the existing matrix guard.

Clarify that `max_threads`, `max_branches`, and `preemption_bound`
jointly constrain the Loom search space, and that changing any of
these bounds may need timeout adjustments.
Add function docstrings to the Python workflow integration test module so
external docstring coverage checks can measure the file correctly.
Clarify that `run_act` returns the exit code, artefact path, and logs so
the function documentation matches the helper contract.
Add `interrogate --fail-under 100 .` to `make lint` so Python
docstring coverage is enforced alongside rustdoc and Clippy.

Install `uv` and then install `interrogate` as a uv tool in CI
before the lint step so the Makefile target has the expected
executable available.
Update the workflow integration helper docstring so it names the
returned artefact directory alongside the exit code and captured logs.
@lodyai
lodyai Bot force-pushed the issue-55-add-loom-tests-to-ci-non-blocking-initially branch from 49dbdad to 49b7ccb Compare June 29, 2026 15:36
codescene-access[bot]

This comment was marked as outdated.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 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 @.github/workflows/ci.yml:
- Around line 35-39: The CI setup step for installing interrogate currently
pulls latest releases via uv, which makes the lint job non-deterministic. Update
the workflow step that runs in the ci.yml install block to pin both the uv tool
installation and the interrogate package version, using the existing install
command sequence around the uv tool install invocation so the lint lane stays
reproducible.

In `@docs/developers-guide.md`:
- Around line 39-42: The Loom test instructions in developers-guide are pointing
contributors to the wrong runner for the non-default-features lane. Update the
guidance around the Loom suite to match make test by replacing the follow-up
cargo test command with cargo nextest run, and keep the wording aligned with the
existing loom-tests / dev-worker flow so the documented local repro path matches
the actual Makefile behavior.
🪄 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: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: bbebe1c2-99b4-4571-981c-766a278363ef

📥 Commits

Reviewing files that changed from the base of the PR and between 225309d and 49b7ccb.

📒 Files selected for processing (5)
  • .github/workflows/ci.yml
  • Makefile
  • docs/developers-guide.md
  • src/env/loom_tests.rs
  • tests/test_workflow_integration.py
🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

  • leynos/cuprum (auto-detected)
  • leynos/rstest-bdd (auto-detected)

Comment thread .github/workflows/ci.yml Outdated
Comment thread docs/developers-guide.md Outdated
Pin the CI-installed uv and interrogate versions so the lint lane is
reproducible instead of resolving latest releases on each run.

Update the Loom testing guide to name cargo nextest for the
non-default-features follow-up lane, matching the Makefile test target.
codescene-access[bot]

This comment was marked as outdated.

Add the local Memtrace database directory to gitignore so runtime daemon
state does not appear as an outstanding repository change.
@leynos
leynos merged commit ad32879 into main Jul 2, 2026
8 checks passed
@leynos
leynos deleted the issue-55-add-loom-tests-to-ci-non-blocking-initially branch July 2, 2026 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add Loom tests to CI (non-blocking initially)

2 participants