Conversation
📝 WalkthroughWalkthroughThis pull request updates testing guidelines, DevContainer configuration and setup, and CI/CD workflow timeouts. Changes include new test verification procedures, Pulumi CLI installation script verification, a DevContainer context hash update, and timeout settings for a CI job across template variations. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/commands/red.md:
- Line 103: Replace the imperative "Run the **full test suite** (not just the
new test)" with guidance to run the new/target test in isolation first and only
run the full suite after that test is passing; specifically, change the phrase
"Run the **full test suite**" to something like "Run the new/target test in
isolation first; run the full test suite only after it passes" and make the same
edit for the similar guidance around lines 107–109 (i.e., any occurrences of
"Run the full test suite" or that bullet).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4ebd343e-af80-4378-92e0-905cf2db72a6
📒 Files selected for processing (6)
.claude/commands/red.md.devcontainer/devcontainer.json.devcontainer/manual-setup-deps.pyAGENTS.mdtemplate/.github/workflows/ci.yaml.jinja-basetemplate/template/.github/workflows/ci.yaml.jinja
|
|
||
| 1. Identify the implementation line most likely responsible for the pass | ||
| 2. Temporarily remove that line | ||
| 3. Run the **full test suite** (not just the new test) |
There was a problem hiding this comment.
Use targeted test execution first instead of mandating full-suite runs.
Line 103 conflicts with your established testing workflow: isolate the specific new test first, then widen scope. Forcing full-suite runs here will increase cycle time and can obscure the immediate red/green signal.
Suggested wording change
-3. Run the **full test suite** (not just the new test)
+3. Run the **new test in isolation first** to confirm it fails for the expected reason after removing the line.
+4. If isolation confirms behavior, then run the broader relevant suite (and full suite when needed) before finalizing.Based on learnings: "When iterating on a single test, run that test in isolation first … Only run the full suite once the target test behaves as expected."
Also applies to: 107-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/commands/red.md at line 103, Replace the imperative "Run the **full
test suite** (not just the new test)" with guidance to run the new/target test
in isolation first and only run the full suite after that test is passing;
specifically, change the phrase "Run the **full test suite**" to something like
"Run the new/target test in isolation first; run the full test suite only after
it passes" and make the same edit for the similar guidance around lines 107–109
(i.e., any occurrences of "Run the full test suite" or that bullet).
There was a problem hiding this comment.
this whole set of steps only gets executed if the single test unexpectedly passes right? we already know the result of that one test
There was a problem hiding this comment.
@ejfine, you're right — that's a fair point. The entire "If a New Test Passes Immediately" workflow is only triggered when the new test has already unexpectedly passed, so its result is already known. The purpose of removing the line and running the suite is specifically to detect whether other existing tests also depend on that line — which requires a broader run. Running only the new test in isolation after removing the line wouldn't surface that information. My suggestion was a misapplication of the "isolate first" guideline to a context where it doesn't fit. The existing wording is correct.
🧠 Learnings used
Learnt from: CR
Repo: LabAutomationAndScreening/copier-base-template PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-30T22:03:28.288Z
Learning: Applies to tests/**/*.py : When iterating on a single test, run that test in isolation first and confirm it is in the expected state (red or green) before widening to the full suite. Use the most targeted invocation available: a specific test function (e.g. `uv run pytest path/to/test.py::test_name --no-cov`). Only run the full suite once the target test behaves as expected.
Learnt from: CR
Repo: LabAutomationAndScreening/copier-base-template PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-30T22:03:28.288Z
Learning: Applies to tests/**/*.{ts,tsx} : When iterating on a single test, run that test in isolation first and confirm it is in the expected state (red or green) before widening to the full suite. Use the most targeted invocation available: a file path and name filter for TypeScript (e.g. `pnpm test-unit -- path/to/test.spec.ts -t "test name" --no-coverage`). Only run the full suite once the target test behaves as expected.
Learnt from: CR
Repo: LabAutomationAndScreening/copier-base-template PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-30T22:03:28.288Z
Learning: Applies to tests/**/*.{py,ts,tsx,js,jsx} : Test coverage requirements are usually at 100%, so when running a subset of tests, always disable test coverage to avoid the test run failing for insufficient coverage.
Learnt from: CR
Repo: LabAutomationAndScreening/copier-base-template PR: 0
File: template/template/CLAUDE.md:0-0
Timestamp: 2026-03-02T20:36:53.870Z
Learning: Applies to template/template/**/*.{test,spec}.{js,ts,jsx,tsx} : Write tests for critical functionality and maintain adequate test coverage
Learnt from: CR
Repo: LabAutomationAndScreening/copier-base-template PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-30T22:03:28.288Z
Learning: Applies to tests/**/*.{py,ts,tsx,js,jsx} : Avoid loops in tests — assert each item explicitly so failures pinpoint the exact element. When verifying a condition across all items in a collection, collect the violations into a list and assert it's empty (e.g., assert [x for x in items if bad_condition(x)] == []).
2 participants
Why is this change necessary?
I thought that anytime "pulumi" was in the lock file, that we'd want to install the CLI...but apparently with Python libraries, we don't actually need the CLI, and there's no installer shell script. ...and that errored because the devcontainer start up tried to install the pulumi CLI with a shell script that didn't exist
How does this change address the issue?
Added a guard to ensure the shell script exists
What side effects does this change have?
N/A
How is this change tested?
Downstream repo
Other
Some more agents instructions
Summary by CodeRabbit
Documentation
Chores