WEB-11226: Add worktree-isolated test flow to shared make helpers - #2
Conversation
Add php_tests_worktree, which runs a git worktree's test suite against the already-running local services (MySQL / Redis / Elasticsearch) with per-worktree database isolation, resolving composer/vendor drift first. Add the DKR_COMPOSE_SRC parameter and document the compose convention (parameterised source mount, no hardcoded container_name) that makes an isolated run coexist with the main stack. Cover the flow with a Docker-free make -n test and wire it into CI. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 Walkthrough
WalkthroughAdds a ChangesWorktree PHP Test Execution
Sequence Diagram(s)flowchart TD
DevMakeCall --> ValidateWorktree
ValidateWorktree --> ResolveWorktreeId
ResolveWorktreeId --> SetDbDatabase
SetDbDatabase --> CheckComposerDrift
CheckComposerDrift -->|drift or missing vendor| RunComposerInstall
CheckComposerDrift -->|no drift| ReuseVendor
RunComposerInstall --> RunDkrComposeCmdRun
ReuseVendor --> RunDkrComposeCmdRun
Estimated code review effort: 3/5 (Medium) — the Makefile logic for drift detection and worktree resolution warrants careful reading. Related issues: None found in the provided context. Related PRs: None found in the provided context. Suggested labels: enhancement, documentation, ci, testing Suggested reviewers: None found in the provided context. A young padawan wrote a script, tested with care, Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/test.yaml:
- Around line 4-8: The workflow trigger in test.yaml currently lacks run
cancellation, so repeated pushes can queue competing jobs. Add a concurrency
block to the workflow definition and key it off the workflow plus ref, using the
existing on: pull_request/push setup, so newer runs cancel superseded ones
instead of contending for runners.
- Line 13: The checkout step in the workflow should be hardened by replacing the
mutable actions/checkout@v4 reference with a full commit SHA and disabling
credential persistence. Update the checkout usage in the workflow so it is
pinned to a specific commit for the checkout action, and add persist-credentials
set to false since the job only runs a local script and does not need the
GITHUB_TOKEN left in git config. Use the actions/checkout step as the unique
locator when making the change.
In `@php.mk`:
- Around line 25-33: The per-worktree database identifier in php_tests_worktree
is too weak because WORKTREE_ID only uses the basename, so different worktrees
with the same folder name can collide and punctuation can produce invalid DB
names. Update the DB_DATABASE derivation to use a collision-resistant, sanitised
identifier based on WORKTREE_SRC or WORKTREE, and adjust the
WORKTREE_ID/DB_DATABASE logic so the resulting database name remains unique and
valid across parent paths and branch-style names.
- Line 37: The composer install path in php_tests_worktree is duplicating the
compose invocation instead of reusing the shared docker.mk composition. Update
the branch that runs composer install to use the same compose command pattern as
DKR_COMPOSE_CMD_RUN, including DKR_COMPOSE_ADDITIONAL and
DKR_COMPOSE_ADDITIONAL_RUN, so php_tests_worktree stays consistent with the rest
of the make-based Docker calls and does not silently diverge when those
variables are set.
In `@test/php_tests_worktree.sh`:
- Line 11: The worktree fixture name in the php_tests_worktree.sh setup is too
session-specific and should be renamed to a neutral, project-agnostic temporary
path. Update the worktree variable assignment in the script to use a generic
fixture name (such as a shared test temp directory) so the helper remains clean
and reusable; the relevant symbol to change is the worktree assignment in
php_tests_worktree.sh.
🪄 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
Run ID: ae55b54c-cd69-4842-bd2f-9d0e65ad68ae
📒 Files selected for processing (6)
.github/workflows/test.yamldocker.mkdocs/docker.mddocs/php.mdphp.mktest/php_tests_worktree.sh
- test.yaml: add a concurrency group (cancel superseded runs) and set persist-credentials: false on checkout (job only runs a local script) - php.mk: derive DB_DATABASE from a sanitised basename plus a hash of the full worktree path, so worktrees sharing a basename under different parents can't collide and punctuation can't yield an invalid identifier; include DKR_COMPOSE_ADDITIONAL in the composer-install call for consistency with DKR_COMPOSE_CMD_RUN - test/docs: rename the fixture to a neutral name; document the DB-name derivation Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Summary
The shared
lickdltd/makehelpers bind-mount the main checkout into the CLI container, so tests can't run against a git worktree's code — blocking parallel, multi-worktree development in every repo that inherits them. This addsphp_tests_worktree, which runs a worktree's suite against the already-running local services with per-worktree database isolation, resolving composer/vendor drift first. It also introduces theDKR_COMPOSE_SRCparameter and documents the compose convention that lets an isolated run coexist with the main stack. Fully backwards compatible — every existing target renders byte-identical.Changes
php_tests_worktreein php.mk;WORKTREE=/pathmounts that tree (viaDKR_COMPOSE_SRC) and runs the suite (TESTS=phpunitby default, or an arbitraryCOMMAND).DB_DATABASE=test_<worktree>(hyphen-sanitised, overridable) so a worktree run never collides with the main checkout's DB or a concurrent worktree's.--no-deps(notdocker compose up), reusing the already-running MySQL / Redis / Elasticsearch instead of duplicating the stack.composer.lockis compared with the main checkout's; on drift (or missingvendor/) dependencies are installed for the worktree, otherwise the existingvendor/is reused.DKR_COMPOSE_SRC(configurable source-mount path); the documented convention uses${DKR_COMPOSE_SRC}for the code volume and dropscontainer_nameso worktree and main-checkout stacks don't conflict.docker.mk/php.mk(inherited on the nextmake init) and documented in docs/docker.md and docs/php.md.make -ntest (test/php_tests_worktree.sh) asserting the flow's behaviour, wired into a new Test workflow so it runs on PRs.How to test
./test/php_tests_worktree.sh(needs onlymake) — all assertions should pass.make -n -f docker.mk -f php.mk php_tests_phpunitstill renders a full-stackup, andphp_composer_installis unchanged.make -n -f docker.mk -f php.mk php_tests_worktree WORKTREE=/tmp/foo— the test run usesrun --rm --no-deps,--env DB_DATABASE=test_foo, and noup.Deployment notes
Consuming repos adopt this opt-in when they want the worktree flow: reference
${DKR_COMPOSE_SRC}for the code volume and remove any hardcodedcontainer_namein their compose file (see docs/docker.md). No change is required for existing targets to keep working.Closes WEB-11226