Harden Testing correctness, parallel ownership, and parity - #6
Harden Testing correctness, parallel ownership, and parity#6binaryfire wants to merge 18 commits into
Conversation
Track the exact tokens whose setup callbacks are entered and use that bounded local ownership set for teardown. Preserve the protected setup-loop extension point while rebuilding fresh applications for cleanup and retaining setup or runner failures over later teardown errors. Add counterfactual coverage for partial setup, custom token loops, unattempted tokens, runner suppression, application freshness, ordering, resolver restoration, cleanup exhaustion, and both primary-error precedence rules.
Run every registered process and test-case teardown callback in registration order even when an earlier callback fails, then rethrow the first failure. Keep setup callbacks fail-fast and retain the existing parallel-mode boundary and callback parameters. Expand focused coverage for single and competing teardown failures, callback order, first-error precedence, and the unchanged setup behavior.
Give artisan test one explicit ownership boundary spanning profile allocation, temporary PHPUnit configuration, process execution, reporting, and cleanup. Release every owned file and directory on all terminal paths while preserving the operation failure or first cleanup failure. Require complete configuration and profile writes instead of accepting failed or partial publication. Add deterministic coverage for allocation, signal, process, reporting, coverage, publication, and competing cleanup failures, including exact worker-clone file restoration.
Restore numeric-string forbidden-output keys before strict matcher calls and clear all shared expectation state plus the OutputStyle binding after every attempted command. Remove the behaviorally inert expectedTables state so one command cannot contaminate the next. Implement one-shot dd output through BufferedOutput, publish the real command exit code, and prevent destructor replay. Add sequential failure and reuse regressions plus a subprocess fixture proving captured output, exit reporting, stdout delivery, and exactly one execution, and document the public debugging API.
Add the shared rendered-HTML constraint used by TestView and TestComponent while preserving the separate raw ordered assertion path. Retain valid string zero, normalize Unicode whitespace, fall back safely for malformed bytes, reject textless expectations, and emit correctly composed PHPUnit diagnostics. Restore the current fluent View and Component assertion surface and truthful list annotations. Revalidate both Mailable ordered text callers with counterfactual zero-value coverage and document the complete component testing API.
Restore rendered-text, bulk JSON-path, canonicalized-path, missing-path, and missing flashed-input APIs on TestResponse. Preserve logged-exception, redirect, and JSON diagnostic precedence without allowing unsupported context values to replace the original assertion failure. Accept every valid JSON root, validate exact JSON whitespace, retain stored response-exception precedence, and memoize one decoded wrapper per response without disturbing streamed content. Preserve keyed-null session presence semantics while grouping ordinary diagnostics, add comprehensive counterfactual coverage, and update the canonical HTTP testing guide.
Require each discovered registrar to explicitly define a callable public static register method before invoking it. Keep class discovery and registration simple without reflection, instantiation, caching, or a new interface. Cover public instance methods, non-public static methods, abstract declarations, magic-only classes, and inherited public static registrars with contextual failure assertions.
Detect Closure expectations directly instead of constructing and filtering an intermediate collection. Preserve the existing matching contract while removing an unnecessary test-only allocation and an unqualified class-name string. Retain and type the complete fluent assertion coverage so scalar, array, nested, callable, and interaction behavior remains pinned.
Consume class_uses_recursive results directly instead of flipping the trait map before membership checks. This preserves database test behavior while removing a redundant allocation from test setup. Keep the focused database concern coverage fully typed and verify the existing migration and transaction classification paths remain unchanged.
Make touched Testing fixtures preserve whether process-global values existed and restore their exact prior value after every path, including falsey cache flags. Complete the remaining direct test method return types without changing providers or inherited lifecycle methods. Keep assertion, deprecation, cache, view, and parallel console coverage isolated under the existing framework test bases with no production reset hook or fixture manager.
Remove ext-json and ext-hash requirements and suggestions from the root and every affected active split manifest because supported PHP versions guarantee both facilities. Retain ext-filter and every other extension that a supported runtime may actually omit. Update package metadata regressions and completed design records so they assert the real platform boundary instead of preserving stale extension inventory. This changes no runtime behavior or package API.
Declare Testing's direct DOM, mbstring, DI, and Carbon boundaries and publish accurate ParaTest and PHPUnit suggestions. Record the supported ParaTest 7 getExitCode omission and current Laravel provenance without duplicating canonical user documentation. Add executable metadata coverage for root constraint alignment, provider discovery, direct dependencies, suggestion ranges, and the absence of guaranteed-core extension inventory across every active split manifest.
Route the active audit work unit to Testing, add collision-free testing-03 through testing-16 dependency rows, close every revalidated Testing-pending edge, and mark the package complete. Preserve the carried Sentry records and coroutine-08 obligation while restoring precise Database, View, Testbench, and Foundation ownership states. Record the completed Testing implementation, Mail assertion revalidation, metadata coverage boundaries, public documentation, API and performance effects, rejected machinery, and final verification. Remove stale later-Testing prose without rewriting historical implementation records.
Record the final evidence-backed design for parallel process ownership, teardown exhaustion, command resources, assertion parity, JSON roots, registrar validation, metadata, test typing, and fixture restoration. Include the retained core anti-overengineering rules, complete implementation and counterfactual test boundaries, Laravel API and ParaTest compatibility decisions, hot-path assessment, rejected alternatives, cross-package routing, and finalized audit-record requirements. The plan reflects the signed-off implementation without retaining superseded proposals or review history.
…ess-parity-lifecycles # Conflicts: # docs/plans/2026-07-12-0900-framework-coroutine-state-lifecycle-audit.md
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR updates Testing lifecycle behavior, assertions, command cleanup, package metadata, documentation, and audit records. It adds regression coverage for parallel execution, console state, HTML and JSON assertions, session diagnostics, profile writes, and manifest validation. ChangesTesting parity and lifecycle corrections
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 SummaryThe PR hardens testing infrastructure and restores assertion/API parity without changing application request paths.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/testing/src/Concerns/RunsInParallel.php | Tracks entered process tokens, preserves setup or runner failures, and tears down owned tokens using fresh applications. |
| src/testing/src/Console/TestCommandBase.php | Establishes exception-safe ownership and exhaustive cleanup for temporary configuration, coverage, and profiling resources. |
| src/testing/src/ParallelTesting.php | Exhausts process and test-case teardown callback lists while preserving the first cleanup failure. |
| src/testing/src/PendingCommand.php | Resets shared console expectations on all exits and adds one-shot buffered command debugging. |
| src/testing/src/TestResponse.php | Restores testing assertion APIs, accepts all valid JSON roots, and memoizes decoded response JSON. |
| src/testing/src/Constraints/SeeInHtml.php | Introduces normalized rendered-text matching with ordering, negation, and precise failure diagnostics. |
| src/testing/src/Profile/ExecutionFinishedSubscriber.php | Validates that serialized profile output is written completely before treating it as published. |
| src/testing/src/PHPUnit/TestStateRegistrars.php | Rejects registrar entries that are not callable public static registration methods. |
| src/foundation/src/Testing/Concerns/InteractsWithConsole.php | Updates console testing state and expectation handling for reusable pending commands. |
| src/mail/src/Mailable.php | Corrects mailable assertion behavior and diagnostics while preserving the public testing surface. |
Reviews (3): Last reviewed commit: "Harden Testing scratch fixture ownership" | Re-trigger Greptile
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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
🧹 Nitpick comments (7)
tests/Console/ArtisanCommandTest.php (1)
450-459: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude the subprocess error output in the assertion messages.
If the fixture fails to bootstrap, the current assertions report only a wrong exit code or missing text. The reason stays in stderr and is discarded. Pass
$process->getErrorOutput()as the assertion message so a failure is diagnosable in CI.♻️ Proposed change
try { $process->run(); - $this->assertSame(1, $process->getExitCode()); - $this->assertStringContainsString('fixture output', $process->getOutput()); - $this->assertStringContainsString('"exitCode" => 7', $process->getOutput()); + $failure = $process->getErrorOutput(); + + $this->assertSame(1, $process->getExitCode(), $failure); + $this->assertStringContainsString('fixture output', $process->getOutput(), $failure); + $this->assertStringContainsString('"exitCode" => 7', $process->getOutput(), $failure); $this->assertSame('1', file_get_contents($counter)); } finally { $filesystem->deleteDirectory($directory); }🤖 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/Console/ArtisanCommandTest.php` around lines 450 - 459, Update the assertions in the process execution test around `$process->run()` to pass `$process->getErrorOutput()` as their assertion message, so failures include subprocess stderr while preserving the existing expected values and cleanup behavior.tests/Testing/Console/TestCommandTest.php (3)
52-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake
tearDown()failure-safe.The three restoration steps run unguarded. If the
unlinkat line 55 or thefile_put_contentsat line 60 raises, the remaining steps andparent::tearDown()are skipped. The test app then keeps a modifiedphpunit.xmland a stale$_SERVER['argv'], which leaks into later tests in the same process.The repository already uses an exhaustive teardown pattern in
src/testbench/src/PHPUnit/TestCase.php. Apply the same pattern here.♻️ Proposed change
protected function tearDown(): void { - foreach (glob($this->app->basePath('.hypervel-phpunit-profile-*.xml')) ?: [] as $path) { - unlink($path); - } - - foreach ($this->originalConfigurationFiles as $path => [$existed, $contents]) { - if ($existed) { - file_put_contents($path, $contents); - } elseif (is_file($path)) { - unlink($path); - } - } - - if ($this->originalArguments[0]) { - $_SERVER['argv'] = $this->originalArguments[1]; - } else { - unset($_SERVER['argv']); - } - - parent::tearDown(); + $exception = null; + + try { + foreach (glob($this->app->basePath('.hypervel-phpunit-profile-*.xml')) ?: [] as $path) { + unlink($path); + } + } catch (Throwable $throwable) { + $exception = $throwable; + } + + try { + foreach ($this->originalConfigurationFiles as $path => [$existed, $contents]) { + if ($existed) { + file_put_contents($path, $contents); + } elseif (is_file($path)) { + unlink($path); + } + } + } catch (Throwable $throwable) { + $exception ??= $throwable; + } + + if ($this->originalArguments[0]) { + $_SERVER['argv'] = $this->originalArguments[1]; + } else { + unset($_SERVER['argv']); + } + + parent::tearDown(); + + if ($exception !== null) { + throw $exception; + } }🤖 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/Testing/Console/TestCommandTest.php` around lines 52 - 73, Update TestCommandTest::tearDown() to use the repository’s exhaustive teardown pattern from PHPUnit\TestCase: execute each cleanup/restoration step independently so failures in profile-file deletion or configuration restoration do not prevent argv restoration or parent::tearDown(). Preserve all existing cleanup behavior while ensuring every step is attempted.
406-425: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard the signal test on the
pcntlandposixextensions.
TestCommandBaserethrows the signal exception only whenextension_loaded('pcntl')is true (line 114 ofsrc/testing/src/Console/TestCommandBase.php). Ifpcntlis absent, the exception is swallowed, no throwable reaches the catch, and$this->fail(...)at line 418 runs. The child process also callsposix_kill, which requiresext-posix.Add a skip so the test reports "skipped" instead of a false failure on a build without those extensions.
♻️ Proposed change
#[Test] public function itCleansEveryOwnedResourceAfterANonInterruptSignal(): void { + if (! extension_loaded('pcntl') || ! extension_loaded('posix')) { + $this->markTestSkipped('Signal handling requires the pcntl and posix extensions.'); + } + $this->writePhpunitConfiguration();🤖 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/Testing/Console/TestCommandTest.php` around lines 406 - 425, Guard itCleansEveryOwnedResourceAfterANonInterruptSignal with a test skip requiring both the pcntl and posix extensions before invoking the signal harness, so environments lacking either extension report the test as skipped rather than reaching the failure assertion.
379-382: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueRestore restrictive permissions instead of
0777.The
chmodat line 380 only needs to make the temp directory writable soremoveDirectory()can delete it.0700achieves that and avoids granting group and world write access. Static analysis flags the current value.♻️ Proposed change
} finally { - chmod($basePath, 0777); + chmod($basePath, 0700); $this->removeDirectory($basePath); }🤖 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/Testing/Console/TestCommandTest.php` around lines 379 - 382, Update the cleanup block in TestCommandTest’s finally handler to change the chmod mode from 0777 to 0700 before calling removeDirectory(), preserving the existing cleanup flow while avoiding unnecessary group and world permissions.Source: Linters/SAST tools
tests/Mail/MailMailableAssertionsTest.php (1)
246-254: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe HTML variant depends on the position of unrelated markup.
The stub HTML contains
0inXHTML 1.0,xhtml1-transitional.dtd, andinitial-scale=1.0. All of those precede<li>Sixth Item</li>. The assertion fails only because no0appears afterSixth Item. If the stub head or body changes, the test can start passing for the wrong reason or stop failing at all.The text variant at lines 236-244 is not affected, because the text stub contains no
0.Add a
0to the stub body afterSixth Itemand assert the positive ordered case as well, or state the dependency in a comment.🤖 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/Mail/MailMailableAssertionsTest.php` around lines 246 - 254, Update the HTML fixture used by the mailable assertion tests so its body includes a standalone “0” after “Sixth Item”, then extend testMailableOrderedHtmlAssertionsDoNotSkipStringZero to verify the valid ordered sequence succeeds while retaining coverage for the invalid ordering case. Avoid relying on unrelated head markup to exercise the string-zero behavior.src/testing/src/Console/TestCommandBase.php (1)
546-556: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReset
temporaryConfigurationFilebefore throwing on a failed write.The property is assigned at line 546 before the write. If the write fails, the property keeps a path that does not exist. A later call to
profileConfigurationFile()on the same instance returns that stale path at line 510 instead of retrying. The currenthandle()flow propagates the exception, so this is not reachable today, but the state is inconsistent.♻️ Proposed change
- $this->temporaryConfigurationFile = dirname($file) . DIRECTORY_SEPARATOR . '.hypervel-phpunit-profile-' + $temporaryConfigurationFile = dirname($file) . DIRECTORY_SEPARATOR . '.hypervel-phpunit-profile-' . getmypid() . '-' . bin2hex(random_bytes(6)) . '.xml'; - $written = @$document->save($this->temporaryConfigurationFile); + $written = @$document->save($temporaryConfigurationFile); if ($written === false) { throw new RuntimeException(sprintf( 'Unable to write temporary PHPUnit configuration [%s].', - $this->temporaryConfigurationFile, + $temporaryConfigurationFile, )); } - return $this->temporaryConfigurationFile; + return $this->temporaryConfigurationFile = $temporaryConfigurationFile;🤖 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/Console/TestCommandBase.php` around lines 546 - 556, Reset temporaryConfigurationFile to its unset/null state in the failed-write branch before throwing RuntimeException, so profileConfigurationFile() cannot return the stale path and can retry on the same TestCommandBase instance.src/testing/src/PendingCommand.php (1)
437-443: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueUse strict mode for
array_search.The
!== falsecomparison correctly fixes the falsey-key case. The search itself still uses loose comparison. The stored values are booleans today, so behavior does not change, but strict mode removes the dependency on that invariant.♻️ Proposed change
- if (($output = array_search(true, $this->test->unexpectedOutput)) !== false) { + if (($output = array_search(true, $this->test->unexpectedOutput, true)) !== false) { $this->test->fail('Output "' . $output . '" was printed.'); } - if (($output = array_search(true, $this->test->unexpectedOutputSubstrings)) !== false) { + if (($output = array_search(true, $this->test->unexpectedOutputSubstrings, true)) !== false) { $this->test->fail('Output "' . $output . '" was printed.'); }🤖 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/PendingCommand.php` around lines 437 - 443, Use strict mode in both array_search calls within the PendingCommand output checks by passing true as the third argument, while preserving the existing !== false handling and failure messages.
🤖 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/testing/src/PHPUnit/TestStateRegistrars.php`:
- Around line 133-135: Update the registrar validation around the method_exists
and is_callable checks to inspect the public static register method’s signature
and reject any required parameters before invocation. Ensure registrars with
optional parameters remain valid, while required-parameter cases throw the
existing declared RuntimeException instead of reaching $class::register().
---
Nitpick comments:
In `@src/testing/src/Console/TestCommandBase.php`:
- Around line 546-556: Reset temporaryConfigurationFile to its unset/null state
in the failed-write branch before throwing RuntimeException, so
profileConfigurationFile() cannot return the stale path and can retry on the
same TestCommandBase instance.
In `@src/testing/src/PendingCommand.php`:
- Around line 437-443: Use strict mode in both array_search calls within the
PendingCommand output checks by passing true as the third argument, while
preserving the existing !== false handling and failure messages.
In `@tests/Console/ArtisanCommandTest.php`:
- Around line 450-459: Update the assertions in the process execution test
around `$process->run()` to pass `$process->getErrorOutput()` as their assertion
message, so failures include subprocess stderr while preserving the existing
expected values and cleanup behavior.
In `@tests/Mail/MailMailableAssertionsTest.php`:
- Around line 246-254: Update the HTML fixture used by the mailable assertion
tests so its body includes a standalone “0” after “Sixth Item”, then extend
testMailableOrderedHtmlAssertionsDoNotSkipStringZero to verify the valid ordered
sequence succeeds while retaining coverage for the invalid ordering case. Avoid
relying on unrelated head markup to exercise the string-zero behavior.
In `@tests/Testing/Console/TestCommandTest.php`:
- Around line 52-73: Update TestCommandTest::tearDown() to use the repository’s
exhaustive teardown pattern from PHPUnit\TestCase: execute each
cleanup/restoration step independently so failures in profile-file deletion or
configuration restoration do not prevent argv restoration or parent::tearDown().
Preserve all existing cleanup behavior while ensuring every step is attempted.
- Around line 406-425: Guard itCleansEveryOwnedResourceAfterANonInterruptSignal
with a test skip requiring both the pcntl and posix extensions before invoking
the signal harness, so environments lacking either extension report the test as
skipped rather than reaching the failure assertion.
- Around line 379-382: Update the cleanup block in TestCommandTest’s finally
handler to change the chmod mode from 0777 to 0700 before calling
removeDirectory(), preserving the existing cleanup flow while avoiding
unnecessary group and world permissions.
🪄 Autofix
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: CHILL
Plan: Pro Plus
Run ID: f2165d37-e978-4cc5-99aa-7e51a67fb131
📒 Files selected for processing (68)
composer.jsondocs/plans/2026-07-01-0915-fortify-passkeys-port.mddocs/plans/2026-07-12-0900-framework-coroutine-state-lifecycle-audit.mddocs/plans/2026-07-12-0915-framework-coroutine-state-lifecycle-audit-ledger.mddocs/plans/2026-08-05-1615-auth-correctness-lifecycle-and-current-parity.mddocs/plans/2026-08-05-2352-routing-correctness-current-parity-and-cache-lifecycles.mddocs/plans/2026-08-07-1302-sanctum-correctness-cache-settlement-and-current-parity.mddocs/plans/2026-08-07-2205-passkeys-correctness-security-and-maintenance.mddocs/plans/2026-08-08-1443-telescope-correctness-current-parity-and-watcher-lifecycles.mddocs/plans/2026-08-08-2225-testing-correctness-parallel-ownership-and-current-parity.mdsrc/auth/composer.jsonsrc/boost/docs/console-tests.mdsrc/boost/docs/http-tests.mdsrc/boost/docs/views.mdsrc/broadcasting/composer.jsonsrc/encryption/composer.jsonsrc/filesystem/composer.jsonsrc/fortify/composer.jsonsrc/foundation/src/Testing/Concerns/InteractsWithConsole.phpsrc/horizon/composer.jsonsrc/mail/src/Mailable.phpsrc/passkeys/composer.jsonsrc/routing/composer.jsonsrc/sanctum/composer.jsonsrc/socialite/composer.jsonsrc/telescope/composer.jsonsrc/testing/README.mdsrc/testing/composer.jsonsrc/testing/src/AssertableJsonString.phpsrc/testing/src/Concerns/RunsInParallel.phpsrc/testing/src/Concerns/TestDatabases.phpsrc/testing/src/Console/TestCommandBase.phpsrc/testing/src/Constraints/SeeInHtml.phpsrc/testing/src/Constraints/SeeInOrder.phpsrc/testing/src/Fluent/Concerns/Matching.phpsrc/testing/src/PHPUnit/TestStateRegistrars.phpsrc/testing/src/ParallelTesting.phpsrc/testing/src/PendingCommand.phpsrc/testing/src/Profile/ExecutionFinishedSubscriber.phpsrc/testing/src/TestComponent.phpsrc/testing/src/TestResponse.phpsrc/testing/src/TestResponseAssert.phpsrc/testing/src/TestView.phptests/Auth/PackageMetadataTest.phptests/Console/ArtisanCommandTest.phptests/Console/Fixtures/PendingCommandDdFixture.phptests/Horizon/PackageMetadataTest.phptests/Mail/MailMailableAssertionsTest.phptests/Passkeys/PackageMetadataTest.phptests/Routing/PackageMetadataTest.phptests/Sanctum/PackageMetadataTest.phptests/Telescope/PackageMetadataTest.phptests/Testing/AssertTest.phptests/Testing/Concerns/InteractsWithDeprecationHandlingTest.phptests/Testing/Concerns/TestCachesTest.phptests/Testing/Concerns/TestDatabasesTest.phptests/Testing/Concerns/TestViewsTest.phptests/Testing/Console/TestCommandTest.phptests/Testing/Fluent/AssertTest.phptests/Testing/PHPUnit/TestStateRegistrarsTest.phptests/Testing/PackageMetadataTest.phptests/Testing/ParallelConsoleOutputTest.phptests/Testing/ParallelRunnerTest.phptests/Testing/ParallelTestingTest.phptests/Testing/Profile/ExecutionFinishedSubscriberTest.phptests/Testing/SeeInHtmlTest.phptests/Testing/TestResponseTest.phptests/Testing/TestViewTest.php
💤 Files with no reviewable changes (23)
- composer.json
- src/routing/composer.json
- src/passkeys/composer.json
- tests/Horizon/PackageMetadataTest.php
- tests/Routing/PackageMetadataTest.php
- src/sanctum/composer.json
- src/auth/composer.json
- src/socialite/composer.json
- tests/Sanctum/PackageMetadataTest.php
- docs/plans/2026-08-07-2205-passkeys-correctness-security-and-maintenance.md
- tests/Auth/PackageMetadataTest.php
- src/telescope/composer.json
- src/encryption/composer.json
- docs/plans/2026-08-05-2352-routing-correctness-current-parity-and-cache-lifecycles.md
- src/fortify/composer.json
- docs/plans/2026-08-05-1615-auth-correctness-lifecycle-and-current-parity.md
- docs/plans/2026-08-08-1443-telescope-correctness-current-parity-and-watcher-lifecycles.md
- docs/plans/2026-07-01-0915-fortify-passkeys-port.md
- src/foundation/src/Testing/Concerns/InteractsWithConsole.php
- src/horizon/composer.json
- src/broadcasting/composer.json
- tests/Passkeys/PackageMetadataTest.php
- tests/Telescope/PackageMetadataTest.php
Use strict boolean searches when verifying forbidden command output so the implementation no longer depends on loose comparison semantics. Make the test command fixture teardown exhaustive across partial setup, generated profile files, configuration restoration, argv restoration, and parent cleanup while preserving the first failure. Restrict the permission fixture to owner access without changing its failure trigger. Include subprocess stderr in every PendingCommand debug regression assertion so bootstrap failures remain diagnosable in CI.
Clarify that phpstan.types.neon.dist owns only the committed types fixtures and must not be redirected at source or test files. This prevents targeted analysis from overriding the configuration boundary and reporting irrelevant level-max errors for code outside the type-contract suite.
|
@coderabbitai Follow-up is complete. Applied:
Left unchanged after tracing the relevant paths:
The affected test files, formatter, repository PHPStan analyses, and diff checks are green. Separately, the static-analysis guide now records that |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- changed files relevant to follow-up ---'
git diff --name-only HEAD^..HEAD 2>/dev/null | rg '(^src/testing/src/PendingCommand\.php$|^tests/Console/ArtisanCommandTest\.php$|^tests/Testing/Console/TestCommandTest\.php$|phpstan\.types\.neon\.dist|static-analysis|StaticAnalysis)' || true
echo '--- PendingCommand outline ---'
ast-grep outline src/testing/src/PendingCommand.php --items all --view expanded 2>/dev/null || true
echo '--- PendingCommand strict lookup and cleanup context ---'
rg -n -C 5 -P '(array_key_exists|expectsOutput|expectsOutputToContain|expectsOutputToNotContain|expectsOutputToNotContain|finally)' src/testing/src/PendingCommand.php || true
echo '--- dd regression assertions ---'
rg -n -C 4 -P '(PendingCommandDdFixture|stderr|dd\(|exec\(|proc_open)' tests/Console/ArtisanCommandTest.php || true
echo '--- TestCommand teardown outline ---'
ast-grep outline tests/Testing/Console/TestCommandTest.php --items all --view expanded 2>/dev/null || true
echo '--- TestCommand teardown and fixture permissions context ---'
rg -n -C 6 -P '(function tearDown|chmod|0700|mkdir|cleanup|argv|parent::tearDown|Throwable)' tests/Testing/Console/TestCommandTest.php || true
echo '--- static-analysis guidance references ---'
rg -n -C 4 -P 'phpstan\.types\.neon\.dist|types/.*fixtures|fixtures.*types' . -g '*.md' -g '*.neon*' -g '*.dist' || trueLength of output: 36069 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- PendingCommand expectation verification ---'
sed -n '404,470p' src/testing/src/PendingCommand.php
echo '--- PendingCommand output mock matching ---'
sed -n '488,566p' src/testing/src/PendingCommand.php
echo '--- PendingCommand dd subprocess assertions ---'
sed -n '428,485p' tests/Console/ArtisanCommandTest.php
echo '--- failed-publication fixture setup and cleanup ---'
sed -n '360,407p' tests/Testing/Console/TestCommandTest.php
echo '--- complete TestCommandTest teardown ---'
sed -n '24,97p' tests/Testing/Console/TestCommandTest.phpLength of output: 12593
The forbidden-output checks use strict lookups and retain numeric-string handling. The The unchanged registrar, temporary configuration, signal, Mailable, and documentation decisions are justified by the traced behavior and repository conventions. No further action is required. 🐇 ✅ ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/Console/ArtisanCommandTest.php (1)
435-438: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPreserve owner-only permissions for the fixture directory.
The test deletes the directory returned by
ParallelTesting::tempDir()and recreates it without an explicit mode. Pass0700soexecutions.txtstays private on shared test hosts.Preserve the directory mode
- $filesystem->makeDirectory($directory); + $filesystem->makeDirectory($directory, 0700);🤖 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/Console/ArtisanCommandTest.php` around lines 435 - 438, Update the fixture setup around ParallelTesting::tempDir and Filesystem::makeDirectory to explicitly create the directory with owner-only mode 0700, preserving the existing deletion and recreation flow.
🤖 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/Testing/Console/TestCommandTest.php`:
- Around line 381-384: Update the temporary fixture path in the test around
$basePath to use Testbench’s disposable BASE_PATH or ParallelTesting::tempDir()
instead of sys_get_temp_dir(). Preserve the unique per-test suffix and existing
directory creation behavior.
- Around line 29-30: Update the $originalArguments property type in the test
class to use array<int, string>|null instead of mixed, reflecting the known argv
value type while preserving its nullable state.
---
Outside diff comments:
In `@tests/Console/ArtisanCommandTest.php`:
- Around line 435-438: Update the fixture setup around ParallelTesting::tempDir
and Filesystem::makeDirectory to explicitly create the directory with owner-only
mode 0700, preserving the existing deletion and recreation flow.
🪄 Autofix
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: CHILL
Plan: Pro Plus
Run ID: 57c95ab1-aeb9-41ae-a116-a1b81f30b5ee
📒 Files selected for processing (4)
AGENTS.mdsrc/testing/src/PendingCommand.phptests/Console/ArtisanCommandTest.phptests/Testing/Console/TestCommandTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- src/testing/src/PendingCommand.php
Clarify that any isolated scratch directory should use ParallelTesting::tempDir(), regardless of the base test case. Move every TestCommandTest scratch project onto worker- and process-owned paths. Reuse stable, delete-first paths where leftovers remain writable, retain a unique path for the deliberately read-only fixture, and keep all fixture directories owner-only. This removes group-writable executable test content without adding shared fixture machinery. Document the exact CLI argv snapshot as a nullable string list while preserving absent-versus-present state. The focused TestCommand suite and formatter both pass.
|
@coderabbitai Follow-up complete in The The sweep also found an executable profile project created with The repository guidance now makes scratch-directory ownership independent of the base test class. The focused TestCommand suite and the formatter are green. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
commit="14e3fa520"
echo '--- commit summary ---'
git show --no-ext-diff --format=fuller --stat "$commit"
echo '--- changed paths ---'
git diff-tree --no-commit-id --name-status -r "$commit"
echo '--- relevant diff ---'
git show --no-ext-diff --format= -- \
"$commit" -- \
tests/Testing/Console/TestCommandTest.php \
tests/Console/ArtisanCommandTest.php \
AGENTS.md
echo '--- TestCommandTest outline ---'
ast-grep outline tests/Testing/Console/TestCommandTest.php --items all --view expanded
echo '--- scratch directories, argv snapshots, and permissions ---'
rg -n -C 5 -P '(argv|ParallelTesting::tempDir|mkdir\s*\(|chmod\s*\(|0[0-7]{3}|scratch|project)' \
tests/Testing/Console/TestCommandTest.php tests/Console/ArtisanCommandTest.php AGENTS.mdLength of output: 29363
Commit
I verified the diff and implementation. I did not run the focused suite or formatter in this sandbox. No further action is required. ✏️ Learnings added
You are interacting with an AI system. |
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Summary
This PR completes the Testing correctness and parity work. It makes parallel process ownership failure-safe, exhausts teardown work without hiding the primary failure, closes temporary test-command resource leaks, restores current Laravel testing APIs, and fixes assertion paths that could skip valid values or produce misleading diagnostics.
It also makes
PendingCommandreusable after failures, adds one-shot command debugging, accepts every valid JSON root while memoizing response decoding, validates test-state registrars at discovery time, restores exact test fixture state, and corrects package metadata. Guaranteed PHP core facilities are no longer declared as optional Composer extensions.No application request path changes. The runtime changes are confined to test execution and assertions, with repeated JSON decoding replaced by a per-response cache.
For more details, see:
docs/plans/2026-08-08-2225-testing-correctness-parallel-ownership-and-current-parity.mdParallel ownership and cleanup
Test command resources
artisan testone explicit ownership boundary for temporary configuration, profile, and coverage resources.Assertions and response parity
"0"expectations, malformed byte behavior, Unicode whitespace normalization, ordering, negation, and precise PHPUnit diagnostics.nullJSON roots.Console testing
PendingCommand::dd()with buffered output, the real exit code, and one-shot execution so destruction cannot rerun the command.Metadata and maintenance
ext-jsonandext-hashdeclarations from active manifests because supported PHP versions guarantee both facilities.Compatibility and performance
Supported Laravel testing APIs are restored or extended without changing their public signatures. Hypervel-specific coroutine execution, streamed responses, HEAD behavior, worker-local Testbench clones, and centralized static cleanup remain intact. The existing ParaTest 7-specific omission of Laravel's
getExitCode()helper remains documented because the supported runner already returns the final exit code directly.The parallel bookkeeping is a small local token list bounded by the configured process count. Exhaustive work occurs only during teardown or failure handling. There are no new locks, registries, retries, context slots, production middleware, or application hot-path operations.
Validation
composer fix.0.4changes.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores