Improve parallel testing and Testbench package mode#416
Merged
Conversation
Cast ParaTest process tokens to strings when resolving the Hypervel parallel testing token from ParallelRunner. ParallelTesting::token() is typed as string|false, while Collection::range() yields integers, so the resolver should preserve the public token contract. Add focused coverage proving process tokens are exposed as string values during ParallelRunner iteration. Also correct the InteractsWithRedis testing docblock so the documented REDIS_DB default matches the actual framework config and helper behavior: 0, not 1.
Document the agreed design for replacing the components Redis preflight wrapper with runtime Redis isolation owned by the testing helpers. The plan captures the final split between raw framework ParaTest runs and scoped Testbench package-mode contract runs, including why package:test should not be the full mixed-suite runner for the components repository. It also records the Testbench environment ownership decisions, Redis worker database allocation rules, secondary database behavior, route cache isolation work, AOP source-map fix, documentation updates, and the focused/full test plan used to verify the change.
Move Redis worker database selection out of the components preflight wrapper and into the testing runtime that actually uses Redis. Introduce RedisTestDatabases as the shared allocator for InteractsWithRedis and subprocess bootstraps such as the Horizon worker fixture. Parallel workers now use REDIS_TEST_DB_MIN through REDIS_TEST_DB_MAX, skip an explicitly configured REDIS_TEST_SECONDARY_DB, and fail clearly when the configured range cannot cover the active worker token. Sequential Redis tests continue to use REDIS_DB directly. Secondary database use is now explicit through REDIS_TEST_SECONDARY_DB so normal app and package tests do not reserve or touch a second Redis database unless a test asks for one. Update Redis integration cleanup so shared secondary database tests delete their own keys, and configure the Redis workflow and example env file with the secondary database needed by the low-level Redis integration suite.
Fix TestCommandBase::clearEnv() so test commands remove parent application dotenv values from every environment store before spawning PHPUnit or ParaTest. The previous implementation called the immutable repository writer's clear() method after enablePutenv() rebuilt the repository. That writer had not loaded the parent dotenv keys, so it treated them as external values and refused to clear them, allowing values to leak into Symfony child processes. Use Env::deleteMany() after rebuilding the adapter list so keys from the configured dotenv file are removed from \, \, and the putenv adapter. Add coverage proving the command clears all three stores.
Stop forwarding the parent Testbench CLI application's full runtime environment to package test child processes. Add EnvironmentFile as the shared resolver for package and workbench env files, and use it from both the skeleton sync command path and Bootstrapper runtime clone creation. In package-test mode the resolved package/workbench env file, or the skeleton fallback, is copied into the child runtime application as .env; outside package-test mode Bootstrapper leaves raw PHPUnit Testbench behavior alone. Keep explicit Testbench YAML env values as the command-owned package-test channel and preserve package command variables as the final overrides. Add coverage for not forwarding parent runtime env, forwarding configured YAML env, package command override precedence, runtime clone env copying, skeleton fallback copying, and raw-mode non-copying. Also make runtime clone deletion tolerant of same-token sibling cleanup races by retrying once and accepting the desired postcondition only when the directory is actually gone.
Make cached route setup safe for Testbench package-mode parallel runs. Configure worker-specific route cache paths before application setup when defineCacheRoutes() is called early, because those tests can run before the application and ParallelTesting service exist. Write unique route fixture files per test instance using the worker token, process id, and monotonic time, then delete only the files owned by that test. This preserves remote child route synchronization while preventing one test from deleting sibling route files that another test still needs. Normalize glob() results before route sync and harden the serve smoke test diagnostics and port selection so package-mode parallel failures point at the real issue.
Restore Testbench package-mode defaults that package tests expect from the skeleton environment. Add SESSION_DRIVER=cookie and CACHE_STORE=database to the skeleton .env.example so package-test mode exercises the same defaults as the upstream Testbench contract. Keep the session config minimal by reading the driver from env while leaving framework base config merge to provide the rest of the session configuration. Add the missing return types in the related env tests while touching the Testbench env contract area.
Keep Testbench's own package-mode contract suite from auto-discovering the hypervel/components root package aggregate. The components root package is a monorepo development aggregate, not the package-under-test for Testbench's contract tests. Ignoring it in Testbench's own testbench.yaml prevents optional providers such as Fortify from leaking into Slim skeleton package-mode tests while preserving normal discovery behavior for real packages. Add coverage that package-specific dont-discover entries filter the manifest view even when the cached package file still contains the root package metadata. Record the remaining package-author discovery determinism cleanup in docs/todo.md so the broader runtime clone seeding behavior is handled deliberately later.
Fix AOP proxy regeneration when Composer's runtime class map already contains generated proxy paths. GenerateProxies now maintains a source class map, filters generated .proxy.php entries out of harvested Composer maps, and rejects exact findFile() results that point back to generated proxies. Ast can receive the known source file path directly, avoiding a second lookup through a loader that may already have proxy paths installed. ProxyManager now records the source file used for each generated proxy so source-path drift from later class-map overrides forces regeneration even when mtimes would otherwise hide the change. Add coverage for deleted proxy regeneration, PSR-4 proxy path rejection after source map flushes, newer source regeneration, source-path changes, and loader restoration for controlled test fixtures.
Extract the repeated purge/sync skeleton command test preservation logic into a shared PreservesSkeletonFiles trait. Both command tests now snapshot the exact skeleton files they mutate, restore existing files with their parent directories, and delete files that did not exist before the test. This keeps the shared Testbench runtime skeleton stable across package-mode tests without duplicating file, directory, and symlink cleanup logic in each test class.
Define TESTBENCH_CORE in the Testbench binary after loading Composer's autoloader. This matches Orchestra's binary shape and makes is_testbench_cli() true for real Testbench CLI executions. Package commands are visible through the Testbench CLI and package manifest discovery can include the package-under-test metadata through the intended command path.
Replace the components-specific bin/paratest wrapper with raw ParaTest for the full framework suite. The Redis preflight wrapper no longer owns Redis setup; runtime Redis isolation now happens inside the testing helpers that actually touch Redis. phpunit.xml.dist supplies HYPERVEL_PARALLEL_TESTING so raw vendor/bin/paratest and composer test:parallel use the same framework parallel-testing setup. Add composer test:testbench for the scoped Testbench package-mode contract suite and wire it into composer check and composer fix so framework tests and package-mode behavior are both covered by the standard local gates.
Split the main tests workflow into clearer style, framework suite, and Testbench package-mode steps. Keep the framework suite on sequential PHPUnit so CI avoids ParaTest scheduling noise, while adding the scoped package:test --parallel tests/Testbench run that verifies the package-mode contract introduced by the new local gates.
Update maintainer and user-facing docs for the new test infrastructure split. AGENTS.md now points maintainers at composer test:parallel for the raw components framework suite and composer test:testbench for the scoped Testbench package-mode contract suite, including the Redis worker range caveat for ParaTest process counts. The Boost testing docs now describe Redis database allocation for tests using InteractsWithRedis, including REDIS_TEST_DB_MIN, REDIS_TEST_DB_MAX, REDIS_TEST_SECONDARY_DB, worker overflow behavior, and string token callback signatures. The Testbench docs and README document package-test parallel behavior and the intentional difference from Orchestra: Hypervel does not forward the parent Testbench CLI application's full runtime environment to child test processes; package/workbench env files are loaded through the runtime app clone instead.
Remove the Testbench-specific TESTBENCH_APP_BASE_PATH channel so package-test worker processes cannot inherit the parent CLI application's runtime clone identity. Package tests now keep the path ownership model explicit: BASE_PATH identifies the current process runtime clone, TESTBENCH_BASE_PATH is reserved for remote child handoff, and APP_BASE_PATH remains the user override. This removes the old Orchestra-style per-class env pointer and its lifecycle hooks because Hypervel's Testbench clones the selected skeleton as the process identity instead of switching source skeleton paths through shared env state. Update the package test command and parallel runner to stop emitting or hydrating TESTBENCH_APP_BASE_PATH. Update Workbench path resolution to honor only APP_BASE_PATH, delete the now-empty setup and teardown hooks, and document the deliberate Orchestra difference in the Testbench README and implementation plan. Add regression coverage proving package test command environments omit TESTBENCH_APP_BASE_PATH and stray values are ignored while APP_BASE_PATH still works as the explicit override.
Package-test workers are not Testbench CLI processes, but they still need the package root metadata that Testbench uses to build deterministic package manifests. Include root metadata when TESTBENCH_PACKAGE_TESTER is present so the first manifest builder in a worker sees the same providers as child Testbench commands.\n\nAdd subprocess coverage for package-tester workers, plain non-Testbench processes, and Testbench CLI processes. Each scenario writes to its own generated manifest file so CLI opcache cannot reuse a stale required path across cases.
Add --host and --port options to the shared server start command and apply them before the Swoole server configuration is built. Testbench's serve command now inherits the same behavior instead of relying on callers to seed HTTP_SERVER_HOST or HTTP_SERVER_PORT in the environment.\n\nValidate non-integer ports, reject address overrides when no HTTP server is configured, and cover the multi-server case by proving the override is applied to the HTTP server by type rather than by array position.
Add a minimal external package that consumes the local Testbench binary through Composer bin discovery and runs package:test --parallel from its own project root. The dogfood suite proves provider discovery, workbench provider loading, package configuration, remote CLI calls, and package-mode parallel execution outside the components repository.\n\nWire the dogfood suite into Composer and CI, ignore only its generated vendor, lock, and PHPUnit cache artifacts, and exclude its vendor directory from CS Fixer. Move Workbench namespace mappings in the root and split Testbench Composer manifests to autoload-dev so consumers are not polluted by test-only Workbench classes while local development and Testbench tests continue to autoload them.
Record the package-mode follow-up plan, its reviewed decisions, and the testing strategy used for the Testbench dogfood work. Remove the completed Testbench todo now that deterministic package-worker metadata discovery is implemented and covered.\n\nDocument the package-test manifest behavior and the new serve --host/--port flags in the Testbench README and Boost docs, keeping the wording aligned with the surrounding documentation and noting production should prefer durable server configuration.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis PR replaces the custom parallel test wrapper, centralizes Redis worker database selection, refactors Testbench runtime/env handling, adds ChangesParallel Redis isolation and test execution wiring
Testbench env ownership and runtime clone refactor
Server serve --host/--port options
AOP proxy source-path tracking fix
Dogfood Testbench package fixture
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Greptile SummaryThis PR restructures parallel testing and Testbench package-mode to make each concern first-class rather than bolt-on. Framework tests now run through raw ParaTest (no custom wrapper), Testbench package-mode tests run through the real Key changes:
Confidence Score: 5/5Safe to merge. All behavioral changes are intentional and well-documented; the redis.yml and tests.yml workflows are correctly updated to match the new env var contract. The changes make parallel testing and Testbench package mode first-class. Redis DB allocation, env scope, route file ownership, proxy source-path tracking, and skeleton cleanup races are all addressed with explicit contracts and tests. No silent regressions were found; the only API-surface change (getSecondaryRedisDb now requires REDIS_TEST_SECONDARY_DB) is reflected correctly in all callers and CI workflows. No files require special attention. src/testbench/src/Foundation/Console/TestCommand.php uses phpdotenv internal classes (flagged in a prior thread) but the logic itself is functionally correct. Important Files Changed
Reviews (2): Last reviewed commit: "fix(testing): address package-mode revie..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR reworks Hypervel’s parallel testing and Testbench “package mode” to behave like first-class, tool-native flows (raw ParaTest + real testbench package:test), while moving Redis isolation and package-mode env handling into runtime-owned, worker-safe paths. It also adds a standalone “dogfood” package to validate real consumer behavior outside the monorepo root.
Changes:
- Switch the components parallel test path to raw ParaTest and add a scoped
test:testbenchsuite that exercises real package-mode execution. - Move Redis parallel isolation to runtime allocation (worker DB ranges + explicit secondary DB) and update integration/tests/docs accordingly.
- Harden Testbench package-mode behavior (env ownership, runtime clone cleanup races, deterministic package manifest discovery, serve host/port overrides) and add a dogfood package + CI coverage.
Reviewed changes
Copilot reviewed 66 out of 69 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/Testing/ParallelRunnerTest.php | Adds regression coverage for string process tokens and ParaTest Options wiring. |
| tests/Testing/Console/TestCommandTest.php | Adds coverage for clearing env vars across all env stores. |
| tests/Testbench/WithWorkbenchTest.php | Updates workbench config expectations; verifies ignoring stray base-path env. |
| tests/Testbench/Foundation/PackageManifestTest.php | Adds coverage for filtering by specific package names. |
| tests/Testbench/Foundation/PackageManifestPackageTesterTest.php | New subprocess-backed tests for deterministic root metadata discovery gating. |
| tests/Testbench/Foundation/Fixtures/PackageManifest/build-manifest.php | New helper script to build a manifest in a fresh PHP process. |
| tests/Testbench/Foundation/EnvTest.php | Adds missing : void return types to tests. |
| tests/Testbench/Foundation/Console/TestCommandTest.php | Expands package-test env forwarding/ownership assertions. |
| tests/Testbench/Foundation/Console/SyncSkeletonCommandTest.php | Preserves/restores skeleton files to avoid cross-test contamination. |
| tests/Testbench/Foundation/Console/PurgeSkeletonCommandTest.php | Preserves/restores skeleton files for purge command tests. |
| tests/Testbench/Concerns/PreservesSkeletonFiles.php | New test helper trait to snapshot/restore skeleton files. |
| tests/Testbench/Concerns/DefineCacheRoutesTest.php | Adds coverage for owned route-file tracking and cleanup behavior. |
| tests/Testbench/CommanderServeTest.php | Updates serve smoke test to use --host/--port options and improves port selection. |
| tests/Testbench/BootstrapperTest.php | Adds coverage for runtime clone deletion races and env-file copy behavior. |
| tests/Server/ServerStartCommandTest.php | Updates server config shape and adds host/port override validation tests. |
| tests/Integration/Redis/RedisProxyIntegrationTest.php | Improves cleanup/isolation around select() and pipeline concurrency. |
| tests/Integration/Horizon/worker.php | Uses new Redis DB allocator for subprocess worker isolation. |
| tests/Foundation/Testing/Concerns/InteractsWithRedisParallelTest.php | Replaces older Redis token math tests with range-based allocation tests. |
| tests/Di/Bootstrap/GenerateProxiesTest.php | Adds coverage for proxy source-map drift and regeneration behavior; flushes new state. |
| src/testing/src/Console/TestCommandBase.php | Changes env cleanup implementation to delete keys across adapters. |
| src/testing/src/Concerns/RunsInParallel.php | Ensures parallel process tokens are resolved as strings. |
| src/testbench/testbench.yaml | Adds dont-discover to avoid auto-discovering hypervel/components in package mode. |
| src/testbench/src/TestCase.php | Removes workbench per-class base-path env setup/teardown hooks. |
| src/testbench/src/Foundation/PackageManifest.php | Adds package-tester gate to include root metadata in package-test workers. |
| src/testbench/src/Foundation/EnvironmentFile.php | New resolver for package/workbench env files with skeleton fallback. |
| src/testbench/src/Foundation/Console/TestCommand.php | Stops forwarding full parent env; forwards only declared Testbench YAML env + command vars. |
| src/testbench/src/Foundation/Console/ServeCommand.php | Passes input into the shared server start path. |
| src/testbench/src/Foundation/Console/Concerns/CopyTestbenchFiles.php | Switches env/config source resolution to the new EnvironmentFile helper. |
| src/testbench/src/Foundation/Bootstrap/SyncTestbenchCachedRoutes.php | Avoids glob() false handling by normalizing to an empty array. |
| src/testbench/src/Features/ParallelRunner.php | Removes TESTBENCH_APP_BASE_PATH injection into env. |
| src/testbench/src/Concerns/InteractsWithWorkbench.php | Removes reliance on TESTBENCH_APP_BASE_PATH; documents intentional divergence. |
| src/testbench/src/Concerns/HandlesRoutes.php | Ensures per-instance route-file ownership tracking + unique route file paths. |
| src/testbench/src/Concerns/CreatesApplication.php | Reads ParaTest token from raw superglobals when app/ParallelTesting aren’t available yet. |
| src/testbench/src/Bootstrapper.php | Improves runtime clone ownership, env-file copying in package mode, and deletion race tolerance. |
| src/testbench/README.md | Documents intentional divergences from Orchestra Testbench. |
| src/testbench/hypervel/config/session.php | Adjusts skeleton session config to respect SESSION_DRIVER env. |
| src/testbench/hypervel/.env.example | Updates skeleton defaults (session/cache related). |
| src/testbench/composer.json | Moves Workbench autoloading to autoload-dev. |
| src/testbench/bin/testbench | Defines TESTBENCH_CORE to align CLI behavior with Orchestra expectations. |
| src/server/src/Commands/ServerStartCommand.php | Adds --host/--port options and applies overrides to configured HTTP server before start. |
| src/foundation/src/Testing/RedisTestDatabases.php | New centralized allocator for Redis DB selection/ranges/secondary DB logic. |
| src/foundation/src/Testing/Concerns/InteractsWithRedis.php | Switches parallel Redis isolation to range-based allocator and removes preflight wrapper logic. |
| src/di/src/Bootstrap/GenerateProxies.php | Captures/merges a stable source class map and adds test-only flush hook. |
| src/di/src/Aop/ProxyManager.php | Tracks source path used for existing proxies and supports test-only flush. |
| src/di/src/Aop/Ast.php | Allows passing explicit source path for proxy generation. |
| src/boost/docs/testing.md | Documents parallel Redis DB range behavior; updates token type examples. |
| src/boost/docs/testbench.md | Documents serve --host/--port and notes package tests inherit parallel isolation behavior. |
| src/boost/docs/lifecycle.md | Notes serve host/port options apply before workers start. |
| src/boost/docs/installation.md | Documents serve host/port overrides. |
| src/boost/docs/deployment.md | Mentions serve host/port overrides and production guidance. |
| phpunit.xml.dist | Sets HYPERVEL_PARALLEL_TESTING=1 via config to enable parallel mode detection in ParaTest runs. |
| dogfood/testbench-package/workbench/config/dogfood.php | New dogfood workbench config fixture. |
| dogfood/testbench-package/workbench/app/Providers/WorkbenchServiceProvider.php | New dogfood workbench provider fixture. |
| dogfood/testbench-package/tests/PackageRuntimeTest.php | Dogfood tests proving provider discovery, workbench loading, remote commands, and parallel package mode. |
| dogfood/testbench-package/testbench.yaml | Dogfood Testbench config enabling workbench install/config discovery. |
| dogfood/testbench-package/src/DogfoodServiceProvider.php | Dogfood package provider and command registration. |
| dogfood/testbench-package/src/DogfoodProbeCommand.php | Dogfood command used to verify remote command execution in package runtime. |
| dogfood/testbench-package/phpunit.xml.dist | Dogfood PHPUnit config using AfterEachTestExtension. |
| dogfood/testbench-package/composer.json | Dogfood package metadata and scripts to run testbench package:test --parallel. |
| docs/plans/2026-07-06-testbench-package-mode-dogfood.md | Detailed plan/spec describing the new package-mode + dogfood + serve options behavior. |
| composer.json | Adds root bin metadata, new test scripts (test:testbench, test:dogfood), and switches parallel script to raw paratest. |
| bin/paratest | Removes the components-only ParaTest wrapper. |
| AGENTS.md | Updates agent guidance for new testing commands and Redis DB range requirements. |
| .php-cs-fixer.php | Excludes dogfood vendor directory from fixer traversal. |
| .gitignore | Ignores dogfood vendor/lock/cache artifacts. |
| .github/workflows/tests.yml | Splits CI into style + framework + Testbench package-mode + dogfood runs. |
| .github/workflows/redis.yml | Adjusts Redis env defaults for the new isolation scheme. |
| .env.example | Adds Redis parallel test DB range variables. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/testing/src/Concerns/RunsInParallel.php (1)
111-119: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReset the token resolver after each worker callback.
resolveTokenUsing()writes static state, but this loop never clears it. The last worker token can leak into later callbacks or any code that callsParallelTesting::token()after the loop finishes.Suggested fix
- tap($this->createApplication(), function ($app) use ($callback, $token) { - ParallelTesting::resolveTokenUsing(fn () => (string) $token); - - $callback($app); - })->flush(); + $app = $this->createApplication(); + + try { + ParallelTesting::resolveTokenUsing(fn () => (string) $token); + $callback($app); + $app->flush(); + } finally { + ParallelTesting::resolveTokenUsing(null); + }🤖 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 `@src/testing/src/Concerns/RunsInParallel.php` around lines 111 - 119, The `forEachProcess` loop in `RunsInParallel` leaves `ParallelTesting::resolveTokenUsing()` set after each worker callback, so the last token can leak into later work. Update the `forEachProcess` flow to restore or clear the token resolver immediately after `$callback($app)` finishes for each token. Keep the change localized around `ParallelTesting::resolveTokenUsing` and the `tap(...)->flush()` sequence so `ParallelTesting::token()` does not retain stale state after the loop.
🧹 Nitpick comments (3)
src/testbench/README.md (1)
8-8: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHeading name diverges from guideline wording.
As per coding guidelines, ported packages should note intentional differences under a heading
Differences From Laravel; this usesDifferences From Orchestra Testbenchinstead. Since Testbench's direct upstream is Orchestra (not Laravel core), this may be an intentional/acceptable exception, but flagging for consistency with the stated convention.🤖 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 `@src/testbench/README.md` at line 8, The README heading in the Testbench package does not match the documented convention for ported packages. Update the markdown heading from the current Orchestra-specific wording to the standard “Differences From Laravel” phrasing, or if this is intended as an exception, align the surrounding copy in the README to clearly follow the project’s convention consistently.Source: Coding guidelines
src/testbench/src/Concerns/CreatesApplication.php (1)
574-597: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSanitize
TEST_TOKENbefore using it inAPP_ROUTES_CACHE, for consistency withHandlesRoutes.
paraTestWorkerToken()only checks the token is a non-empty string, then interpolates it directly into a filesystem path (cache/routes-v7-test-{$token}.php). The sibling implementation inHandlesRoutes::testbenchRouteFilePath()reads the sameTEST_TOKENsource but strips unsafe characters viapreg_replace('/[^A-Za-z0-9_.-]/', '_', $token)before building a path. SinceTEST_TOKENis normally a numeric ParaTest worker id, this is low risk today, but the two call sites disagree on whether the token is trusted for path construction, and this path could later be consumed byjoin_paths(...)inHandlesRouteswithout validation.🛡️ Proposed fix to align token sanitization
protected function paraTestWorkerToken(): ?string { $token = $_SERVER['TEST_TOKEN'] ?? $_ENV['TEST_TOKEN'] ?? null; - return is_string($token) && $token !== '' ? $token : null; + if (! is_string($token) || $token === '') { + return null; + } + + return preg_replace('/[^A-Za-z0-9_.-]/', '_', $token) ?? null; }🤖 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 `@src/testbench/src/Concerns/CreatesApplication.php` around lines 574 - 597, The APP_ROUTES_CACHE path in configureParallelCachePaths uses paraTestWorkerToken() without sanitizing the TEST_TOKEN value, which is inconsistent with HandlesRoutes::testbenchRouteFilePath(). Update paraTestWorkerToken() or the cache path construction to normalize the token the same way as HandlesRoutes does (replace unsafe characters before interpolation) so the token is safe for filesystem paths and both implementations stay aligned.tests/Testbench/BootstrapperTest.php (1)
66-173: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared env/package-tester setup+restore boilerplate.
The correctness of these three tests checks out against
Bootstrapper::createRuntimeCopy/copyPackageEnvironmentFile(workbench-preference, skeleton fallback, and package-tester-off skip paths all match). However, each test repeats ~30 lines of near-identical$_SERVER/$_ENV/getenvcapture-and-restore logic forTEST_TOKENandTESTBENCH_PACKAGE_TESTER. Consider wrapping this in a single helper (e.g.withRuntimeCopyEnvironment(string $token, bool $packageTester, callable $assertions)) that handles setup/teardown internally, reducing ~90 lines of duplication to one reusable method.🤖 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/Testbench/BootstrapperTest.php` around lines 66 - 173, The three BootstrapperTest cases repeat the same TEST_TOKEN and TESTBENCH_PACKAGE_TESTER capture/setup/restore boilerplate, even though the assertions already cover the intended createRuntimeCopy and copyPackageEnvironmentFile behavior. Extract that repeated environment handling into one reusable helper in BootstrapperTest (for example, a wrapper that saves/restores $_SERVER, $_ENV, and getenv state while running a callback) and have it manage the temporary token/package-tester setup internally. Keep the per-test assertions and runtimePath cleanup, but remove the duplicated pre/post environment code from each test.
🤖 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 `@src/server/src/Commands/ServerStartCommand.php`:
- Around line 74-117: The port validation in ServerStartCommand::handle only
checks for an integer, so invalid TCP ports can still be applied to the server
config. Update the existing $port validation to reject values outside the valid
range (1–65535) before writing them into $serverConfig and calling $config->set,
and keep the error surfaced as an InvalidArgumentException from this command.
---
Outside diff comments:
In `@src/testing/src/Concerns/RunsInParallel.php`:
- Around line 111-119: The `forEachProcess` loop in `RunsInParallel` leaves
`ParallelTesting::resolveTokenUsing()` set after each worker callback, so the
last token can leak into later work. Update the `forEachProcess` flow to restore
or clear the token resolver immediately after `$callback($app)` finishes for
each token. Keep the change localized around
`ParallelTesting::resolveTokenUsing` and the `tap(...)->flush()` sequence so
`ParallelTesting::token()` does not retain stale state after the loop.
---
Nitpick comments:
In `@src/testbench/README.md`:
- Line 8: The README heading in the Testbench package does not match the
documented convention for ported packages. Update the markdown heading from the
current Orchestra-specific wording to the standard “Differences From Laravel”
phrasing, or if this is intended as an exception, align the surrounding copy in
the README to clearly follow the project’s convention consistently.
In `@src/testbench/src/Concerns/CreatesApplication.php`:
- Around line 574-597: The APP_ROUTES_CACHE path in configureParallelCachePaths
uses paraTestWorkerToken() without sanitizing the TEST_TOKEN value, which is
inconsistent with HandlesRoutes::testbenchRouteFilePath(). Update
paraTestWorkerToken() or the cache path construction to normalize the token the
same way as HandlesRoutes does (replace unsafe characters before interpolation)
so the token is safe for filesystem paths and both implementations stay aligned.
In `@tests/Testbench/BootstrapperTest.php`:
- Around line 66-173: The three BootstrapperTest cases repeat the same
TEST_TOKEN and TESTBENCH_PACKAGE_TESTER capture/setup/restore boilerplate, even
though the assertions already cover the intended createRuntimeCopy and
copyPackageEnvironmentFile behavior. Extract that repeated environment handling
into one reusable helper in BootstrapperTest (for example, a wrapper that
saves/restores $_SERVER, $_ENV, and getenv state while running a callback) and
have it manage the temporary token/package-tester setup internally. Keep the
per-test assertions and runtimePath cleanup, but remove the duplicated pre/post
environment code from each test.
🪄 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 Plus
Run ID: 5f1aa3bd-f646-4d86-ab22-6abf058dc729
📒 Files selected for processing (69)
.env.example.github/workflows/redis.yml.github/workflows/tests.yml.gitignore.php-cs-fixer.phpAGENTS.mdbin/paratestcomposer.jsondocs/plans/2026-07-05-parallel-redis-runtime-isolation.mddocs/plans/2026-07-06-testbench-package-mode-dogfood.mddogfood/testbench-package/composer.jsondogfood/testbench-package/phpunit.xml.distdogfood/testbench-package/src/DogfoodProbeCommand.phpdogfood/testbench-package/src/DogfoodServiceProvider.phpdogfood/testbench-package/testbench.yamldogfood/testbench-package/tests/PackageRuntimeTest.phpdogfood/testbench-package/workbench/app/Providers/WorkbenchServiceProvider.phpdogfood/testbench-package/workbench/config/dogfood.phpphpunit.xml.distsrc/boost/docs/deployment.mdsrc/boost/docs/installation.mdsrc/boost/docs/lifecycle.mdsrc/boost/docs/testbench.mdsrc/boost/docs/testing.mdsrc/di/src/Aop/Ast.phpsrc/di/src/Aop/ProxyManager.phpsrc/di/src/Bootstrap/GenerateProxies.phpsrc/foundation/src/Testing/Concerns/InteractsWithRedis.phpsrc/foundation/src/Testing/RedisTestDatabases.phpsrc/server/src/Commands/ServerStartCommand.phpsrc/testbench/README.mdsrc/testbench/bin/testbenchsrc/testbench/composer.jsonsrc/testbench/hypervel/.env.examplesrc/testbench/hypervel/config/session.phpsrc/testbench/src/Bootstrapper.phpsrc/testbench/src/Concerns/CreatesApplication.phpsrc/testbench/src/Concerns/HandlesRoutes.phpsrc/testbench/src/Concerns/InteractsWithWorkbench.phpsrc/testbench/src/Features/ParallelRunner.phpsrc/testbench/src/Foundation/Bootstrap/SyncTestbenchCachedRoutes.phpsrc/testbench/src/Foundation/Console/Concerns/CopyTestbenchFiles.phpsrc/testbench/src/Foundation/Console/ServeCommand.phpsrc/testbench/src/Foundation/Console/TestCommand.phpsrc/testbench/src/Foundation/EnvironmentFile.phpsrc/testbench/src/Foundation/PackageManifest.phpsrc/testbench/src/TestCase.phpsrc/testbench/testbench.yamlsrc/testing/src/Concerns/RunsInParallel.phpsrc/testing/src/Console/TestCommandBase.phptests/Di/Bootstrap/GenerateProxiesTest.phptests/Foundation/Testing/Concerns/InteractsWithRedisParallelTest.phptests/Integration/Horizon/worker.phptests/Integration/Redis/RedisProxyIntegrationTest.phptests/Server/ServerStartCommandTest.phptests/Testbench/BootstrapperTest.phptests/Testbench/CommanderServeTest.phptests/Testbench/Concerns/DefineCacheRoutesTest.phptests/Testbench/Concerns/PreservesSkeletonFiles.phptests/Testbench/Foundation/Console/PurgeSkeletonCommandTest.phptests/Testbench/Foundation/Console/SyncSkeletonCommandTest.phptests/Testbench/Foundation/Console/TestCommandTest.phptests/Testbench/Foundation/EnvTest.phptests/Testbench/Foundation/Fixtures/PackageManifest/build-manifest.phptests/Testbench/Foundation/PackageManifestPackageTesterTest.phptests/Testbench/Foundation/PackageManifestTest.phptests/Testbench/WithWorkbenchTest.phptests/Testing/Console/TestCommandTest.phptests/Testing/ParallelRunnerTest.php
💤 Files with no reviewable changes (3)
- src/testbench/src/Features/ParallelRunner.php
- src/testbench/src/TestCase.php
- bin/paratest
Tighten serve --port validation so command-line overrides reject non-numeric and out-of-range TCP ports before mutating server configuration. Clear the synthetic ParallelTesting token resolver and flush each temporary app in a finally block while running process callbacks, so normal and exceptional paths cannot leak worker tokens across the runner. Centralize Testbench ParaTest token sanitization in the pre-application token helper and reuse it for route file names, keeping route-cache and route-stash paths consistent. Deduplicate Bootstrapper runtime-copy environment setup in the package env tests while keeping fixture setup and assertions local to each test. Verified with focused PHPUnit files and composer fix.
Collaborator
Author
|
@greptileai @coderabbitai thanks. I went through the review comments with Claude as a second reviewer. Changes made:
Left unchanged:
Verified with focused PHPUnit runs and |
|
Tip For best results, initiate chat on the files or code changes.
On the "left unchanged" items — your rationale makes sense in each case:
I'll treat those three concerns as resolved based on your explanation. (=^･ω･^=) ﾉ ✏️ Learnings added
|
|
Thanks for the thorough follow-up. The changes address the substantive issues from the review:
The rationale for the unchanged items holds:
Nothing outstanding from my side. Looks good to merge. |
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.
Summary
This PR makes Hypervel's parallel testing and Testbench package mode behave like first-class paths instead of repo-specific special cases.
The full components suite now runs through raw ParaTest, with the parallel-testing flag supplied by PHPUnit config. Testbench package behavior is covered separately through a scoped
package:testsuite. This keeps framework tests out of package-tester semantics while still testing the real Testbench command path.Redis test isolation now happens at runtime, only when a test opts into the Redis testing helper. Each parallel worker gets a configured logical database, secondary Redis database usage is explicit, and the old components-only Redis preflight wrapper is gone.
Testbench package mode now has tighter env ownership. Package-test children receive command-owned variables and declared Testbench YAML env values, not the parent CLI application's whole loaded runtime env. The runtime skeleton still loads package/workbench env files through the normal Testbench path, so package authors get the expected behavior without parent-process leakage.
This also fixes the issues that fell out of exercising Testbench as a real package runner: route cache paths before setup, route file ownership, runtime clone cleanup races, AOP proxy source-map drift, package manifest discovery in workers, and a leaked app base path channel.
Finally, this adds a real dogfood package outside the components root. It installs Testbench through Composer, runs
vendor/bin/testbench package:test --parallel, proves provider discovery and workbench loading, and runs in CI.For more details, see:
docs/plans/2026-07-05-parallel-redis-runtime-isolation.mdanddocs/plans/2026-07-06-testbench-package-mode-dogfood.md.What changed
bin/paratestwrapper.REDIS_TEST_DB_MIN,REDIS_TEST_DB_MAX, andREDIS_TEST_SECONDARY_DBbehavior.package:test.TESTBENCH_COREto the Testbench binary so CLI package behavior matches Orchestra's.TESTBENCH_APP_BASE_PATHruntime channel.serve --hostandserve --portsupport at the shared server command layer.dogfood/testbench-packagefixture and CI coverage.Why
The old setup worked, but too much of it was accidental. A parent wrapper connected to Redis before the suite started, Testbench package mode was not exercised like a package would use it, and some parent CLI env values could leak into child workers.
The new shape puts each behavior where it belongs:
Testing
Ran the focused Redis, Testbench, server command, route-cache, package manifest, dogfood, and package-mode suites while building this.
Ran
composer fix, which covers code style, PHPStan, raw ParaTest, the scoped Testbench package-mode suite, and the dogfood package suite.Summary by CodeRabbit
servecommand--host/--portoptions for overriding the HTTP address.test:testbench,test:dogfood), with improved parallel execution behavior.serveoptions.