Add the PHPUnit adapter over property-testing-core - #1
Conversation
A fluent `forAll()` trait on top of the framework-agnostic engine: generation, integrated shrinking, the regression corpus and the event model all come from `rasuvaeff/property-testing-core` ^0.1, and this package only adapts them to PHPUnit — one AssertionFailedError carrying the original throwable as previous, `Assume` reported as a discard inside the property rather than a skipped test, the distribution report and discard warning, and a verbose listener. Environment parity with the Testo adapter is a tested contract, not a claim: PROPERTY_RUNS, PROPERTY_SEED, PROPERTY_VERBOSE and PROPERTY_DB behave identically, and a corpus written by one adapter is read by the other. No `#[Property]` attribute in 0.1 on purpose: PHPUnit's public extension API observes execution but offers no supported way to intercept and replace a test method call, and this adapter does not reach into PHPUnit internals. Verification: composer build green against core 0.1.0 from Packagist, psalm level 1 clean (two narrow issueHandlers at the PHPUnit boundary), mutation 104/113 with nine equivalents, bin/package-audit 0 errors / 0 warnings, zizmor --persona=auditor clean.
|
Warning Review limit reached
Next review available in: 17 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughAdds a PHPUnit property-testing adapter with fluent configuration, environment support, regression replay, reporting, verbose tracing, tests, documentation, package tooling, CI workflows, and release automation. ChangesPHPUnit Property Testing Adapter
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PHPUnitTest as PHPUnit TestCase
participant PropertyTesting
participant PropertyCheck
participant PropertyRunner
participant VerboseListener
PHPUnitTest->>PropertyTesting: call forAll(generators)
PropertyTesting->>PropertyCheck: create configured property check
PHPUnitTest->>PropertyCheck: call check(property)
PropertyCheck->>VerboseListener: attach verbose listener when enabled
PropertyCheck->>PropertyRunner: execute generated cases
PropertyRunner-->>VerboseListener: emit attempt and shrink events
PropertyRunner-->>PropertyCheck: return result or failure
PropertyCheck-->>PHPUnitTest: record assertion or throw failure
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
src/PhpUnit/VerboseListener.php (1)
85-94: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueType the key parameter as
string.The docblock declares
array<string, mixed>, soarray_keys($arguments)returnslist<string>. Declaringmixed $nameand then concatenating it weakens the guarantee that the trace key is a string. Usestring $name.As per coding guidelines: "Use `declare(strict_types=1)`, explicit types, `final` classes, `readonly` where state allows".♻️ Proposed typing change
$pairs = array_map( - static fn(mixed $value, mixed $name): string => $name . '=' . ValueRenderer::render($value), + static fn(mixed $value, string $name): string => $name . '=' . ValueRenderer::render($value), $arguments, array_keys($arguments), );🤖 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/PhpUnit/VerboseListener.php` around lines 85 - 94, Update the key parameter in the formatArguments array_map callback from mixed to string, matching the declared array<string, mixed> contract and ensuring trace keys are explicitly typed as strings.Source: Coding guidelines
examples/SortPropertyTest.php (1)
54-70: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe median property is a tautology.
For any sorted list,
$sorted[intdiv(count($sorted), 2)]is always between$sorted[0]and$sorted[count($sorted) - 1]. Both assertions hold for every input, so this property cannot be falsified by any generator. It still demonstratesAssume::that(), which is the stated purpose, but consider a property that can actually fail, for example that the median of the sorted list equals the median of the unsorted input passed throughself::sorted().🤖 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 `@examples/SortPropertyTest.php` around lines 54 - 70, Replace the tautological assertions in testMedianStaysBetweenMinAndMax with a property that compares the median derived from the sorted values against the median of the original unsorted input after applying self::sorted(). Preserve the existing Assume::that(count($values) > 1) demonstration and generator setup.tests/EnvironmentParityTest.php (1)
179-185: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the unused catch variable.
$failureis never read in this block. PHP 8 allows a non-capturing catch, andAdapterDetailsTestline 305 already uses that form.♻️ Proposed change
- } catch (AssertionFailedError $failure) { + } catch (AssertionFailedError) { // Recorded. }🤖 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/EnvironmentParityTest.php` around lines 179 - 185, Update the catch block around runFalsifiableProperty in EnvironmentParityTest to use PHP 8’s non-capturing catch syntax, removing the unused $failure variable while preserving the existing AssertionFailedError handling.tests/AdapterDetailsTest.php (1)
62-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the assertion so it actually pins the reindexing.
The test name states that named variadic arguments must not leak string keys.
assertNotSame([], $listener->events)passes with or withoutarray_values()inPropertyCheck::listeners(), because the engine iterates the array either way. This matches the escapedUnwrapArrayValuesmutant reported onsrc/PhpUnit/PropertyCheck.phpline 154. Assert the list shape directly.♻️ Proposed assertion
- self::assertNotSame([], $listener->events); + self::assertNotSame([], $listener->events); + self::assertSame(array_keys($listener->events), range(0, count($listener->events) - 1));The reindexing itself is only observable inside the engine, so consider also asserting on a value the engine derives from the listener list if core exposes one.
🤖 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/AdapterDetailsTest.php` around lines 62 - 77, Strengthen testListenersAcceptNamedArgumentsWithoutLeakingStringKeys by asserting the listener-derived event data has the expected reindexed list shape, rather than only checking that events are non-empty. Use an assertion that fails when PropertyCheck::listeners() omits array_values() and preserves the named argument’s string key, while retaining the existing property check.Source: Linters/SAST tools
src/PhpUnit/PropertyCheck.php (1)
287-300: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConfirm the intended behavior for out-of-range
PROPERTY_RUNS.
(int) $envsaturates toPHP_INT_MAXfor a digit string above the integer range.PROPERTY_RUNS=999999999999999999999therefore passes validation and configures an effectively unbounded run count. If you want a hard rejection, compare the string against(string) PHP_INT_MAXlength or usefilter_var($env, FILTER_VALIDATE_INT).♻️ Optional stricter parse
- if (preg_match('/^\d+\z/', $env) !== 1 || (int) $env < 1) { + $runs = filter_var($env, FILTER_VALIDATE_INT); + + if (preg_match('/^\d+\z/', $env) !== 1 || !is_int($runs) || $runs < 1) { throw new \InvalidArgumentException(sprintf('PROPERTY_RUNS must be a positive integer, got "%s"', $env)); } - return (int) $env; + return $runs;🤖 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/PhpUnit/PropertyCheck.php` around lines 287 - 300, Update PropertyCheck::envRuns() to reject digit strings outside PHP’s integer range instead of allowing values that cast to PHP_INT_MAX; use a range-safe integer validation approach such as FILTER_VALIDATE_INT or an explicit comparison against PHP_INT_MAX, while preserving acceptance of positive in-range values and the existing exception behavior.
🤖 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 `@AGENTS.md`:
- Around line 122-124: Update the Infection guidance in AGENTS.md to document
minMsi as 90 instead of 85, matching the configured threshold in
infection.json5.
In `@examples/README.md`:
- Around line 14-16: Update the documented PHPUnit command in the README to
invoke PHPUnit through the Composer Docker image correctly, using either an
explicit PHP entrypoint or composer exec while preserving the existing volume,
working directory, and test target.
In `@psalm.xml`:
- Around line 28-34: Remove the PHPUnit constructor suppressions for
PHPUnit\Framework\Exception::__construct and
PHPUnit\Framework\AssertionFailedError::__construct from the InternalMethod
errorLevel block in psalm.xml, retaining only AssertionFailedError and
TestCase::addToAssertionCount allowances. If this exposes a Psalm boundary
violation, adjust the related throw/catch code rather than adding new
suppressions.
In `@rector.php`:
- Around line 20-30: Update the comment above the dead-code skip rules to remove
the obsolete #[Property] rationale and unsupported attribute claim. Describe
only the actual reflection-based contract requiring generator methods and stub
test bodies to remain, or omit the explanatory comment while preserving the
existing skip configuration.
In `@tests/AdapterDetailsTest.php`:
- Around line 141-147: Update the report regex in the test’s assertSame call to
accept the platform-specific line terminator emitted by
PropertyCheck::reportClassifications, using PHP_EOL rather than hardcoding \n.
Preserve the existing distribution and percentage assertions.
In `@tests/PropertyCheckTest.php`:
- Around line 25-31: Add the same PROPERTY_* environment isolation used by
AdapterDetailsTest and EnvironmentParityTest to PropertyCheckTest: call
Env::isolateProperty() during setUp and restore the captured environment during
tearDown. Preserve the existing test behavior while ensuring tests use their
unseeded defaults or explicitly pinned seeds rather than ambient configuration.
---
Nitpick comments:
In `@examples/SortPropertyTest.php`:
- Around line 54-70: Replace the tautological assertions in
testMedianStaysBetweenMinAndMax with a property that compares the median derived
from the sorted values against the median of the original unsorted input after
applying self::sorted(). Preserve the existing Assume::that(count($values) > 1)
demonstration and generator setup.
In `@src/PhpUnit/PropertyCheck.php`:
- Around line 287-300: Update PropertyCheck::envRuns() to reject digit strings
outside PHP’s integer range instead of allowing values that cast to PHP_INT_MAX;
use a range-safe integer validation approach such as FILTER_VALIDATE_INT or an
explicit comparison against PHP_INT_MAX, while preserving acceptance of positive
in-range values and the existing exception behavior.
In `@src/PhpUnit/VerboseListener.php`:
- Around line 85-94: Update the key parameter in the formatArguments array_map
callback from mixed to string, matching the declared array<string, mixed>
contract and ensuring trace keys are explicitly typed as strings.
In `@tests/AdapterDetailsTest.php`:
- Around line 62-77: Strengthen
testListenersAcceptNamedArgumentsWithoutLeakingStringKeys by asserting the
listener-derived event data has the expected reindexed list shape, rather than
only checking that events are non-empty. Use an assertion that fails when
PropertyCheck::listeners() omits array_values() and preserves the named
argument’s string key, while retaining the existing property check.
In `@tests/EnvironmentParityTest.php`:
- Around line 179-185: Update the catch block around runFalsifiableProperty in
EnvironmentParityTest to use PHP 8’s non-capturing catch syntax, removing the
unused $failure variable while preserving the existing AssertionFailedError
handling.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b7bdc224-5853-4ce8-a997-814320240a98
📒 Files selected for processing (32)
.editorconfig.gitattributes.github/ISSUE_TEMPLATE/bug_report.yml.github/ISSUE_TEMPLATE/config.yml.github/ISSUE_TEMPLATE/feature_request.yml.github/dependabot.yml.github/workflows/build.yml.github/workflows/release.yml.github/workflows/static-analysis.yml.github/workflows/zizmor.yml.gitignore.php-cs-fixer.phpAGENTS.mdCHANGELOG.mdCLAUDE.mdMakefilecomposer.jsonexamples/README.mdexamples/SortPropertyTest.phpinfection.json5llms.txtphpunit.xmlpsalm.xmlrector.phpsrc/PhpUnit/PropertyCheck.phpsrc/PhpUnit/PropertyTesting.phpsrc/PhpUnit/VerboseListener.phptests/AdapterDetailsTest.phptests/EnvironmentParityTest.phptests/PropertyCheckTest.phptests/Support/Env.phptests/Support/RecordingListener.php
| ```bash | ||
| docker run --rm -v "$PWD":/app -w /app composer:2 vendor/bin/phpunit examples/SortPropertyTest.php | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Compare the documented example command with the Makefile invocations.
fd -t f 'Makefile' -d 2 | xargs -I{} sh -c 'echo "== {} =="; rg -n "phpunit|composer:2|entrypoint" {}'Repository: rasuvaeff/property-testing-phpunit
Length of output: 433
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== README excerpt =="
sed -n '1,40p' examples/README.md
echo
echo "== Makefile relevant lines =="
sed -n '1,120p' Makefile
echo
echo "== dockerfile/composer config files =="
git ls-files | sed -n '/^Dockerfile$/p; /^composer\.json$/p; /^compose.*\.ya?ml$/p; /^\..*composer.*$/p'Repository: rasuvaeff/property-testing-phpunit
Length of output: 3643
Use an entrypoint or composer exec for the documented PHPUnit command.
composer:2 has Composer as its entrypoint, so docker run … composer:2 vendor/bin/phpunit … runs Composer instead of PHPUnit. Use --entrypoint php or composer exec so the README example matches the working composer:2 Docker workflow.
🤖 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 `@examples/README.md` around lines 14 - 16, Update the documented PHPUnit
command in the README to invoke PHPUnit through the Composer Docker image
correctly, using either an explicit PHP entrypoint or composer exec while
preserving the existing volume, working directory, and test target.
Source: Learnings
| rewind($stdout); | ||
| $report = (string) stream_get_contents($stdout); | ||
| rewind($stderr); | ||
|
|
||
| // "always" hits every check, so it leads regardless of insertion | ||
| // order; percentages are integer-rounded of count/checks. | ||
| self::assertSame(1, preg_match('/^Property "\w+" distribution: always 100% \(50\/50\), rare \d+% \(\d+\/50\)\n$/', $report)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Fix the Windows build failure: the regex hardcodes \n.
PropertyCheck::reportClassifications terminates the report line with PHP_EOL. On Windows PHP_EOL is \r\n, and php://memory does not translate line endings. The pattern requires ) immediately followed by \n, so the match returns 0 and the Windows job fails at this line. The sibling tests at lines 195, 263, and 285 already compare against PHP_EOL and stay green.
Make the terminator platform-independent.
🐛 Proposed fix
- self::assertSame(1, preg_match('/^Property "\w+" distribution: always 100% \(50\/50\), rare \d+% \(\d+\/50\)\n$/', $report));
+ self::assertSame(1, preg_match('/^Property "\w+" distribution: always 100% \(50\/50\), rare \d+% \(\d+\/50\)\R\z/', $report));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rewind($stdout); | |
| $report = (string) stream_get_contents($stdout); | |
| rewind($stderr); | |
| // "always" hits every check, so it leads regardless of insertion | |
| // order; percentages are integer-rounded of count/checks. | |
| self::assertSame(1, preg_match('/^Property "\w+" distribution: always 100% \(50\/50\), rare \d+% \(\d+\/50\)\n$/', $report)); | |
| rewind($stdout); | |
| $report = (string) stream_get_contents($stdout); | |
| rewind($stderr); | |
| // "always" hits every check, so it leads regardless of insertion | |
| // order; percentages are integer-rounded of count/checks. | |
| self::assertSame(1, preg_match('/^Property "\w+" distribution: always 100% \(50\/50\), rare \d+% \(\d+\/50\)\R\z/', $report)); |
🧰 Tools
🪛 GitHub Actions: build / 0_Windows.txt
[error] 147-147: PHPUnit test Rasuvaeff\PropertyTesting\PhpUnit\Tests\AdapterDetailsTest::testTheDistributionReportLineIsExactAndSortedByCount failed: expected value 1 but received 0. The composer test/phpunit command exited with code 1.
🪛 GitHub Actions: build / Windows
[error] 147-147: PHPUnit test Rasuvaeff\PropertyTesting\PhpUnit\Tests\AdapterDetailsTest::testTheDistributionReportLineIsExactAndSortedByCount failed: expected 1 but got 0. The phpunit command failed with exit code 1.
🤖 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/AdapterDetailsTest.php` around lines 141 - 147, Update the report regex
in the test’s assertSame call to accept the platform-specific line terminator
emitted by PropertyCheck::reportClassifications, using PHP_EOL rather than
hardcoding \n. Preserve the existing distribution and percentage assertions.
Source: Pipeline failures
…boundary - PropertyCheck/VerboseListener: report lines now use a literal "\n" instead of PHP_EOL. This CLI output is machine-greppable; PHP_EOL is \r\n on Windows and broke every test (and would break every downstream tool) that matches a line with a plain-LF pattern. Caught by the Windows CI job. - .php-cs-fixer.php: drop /benchmarks from the Finder. This package has no benchmarks/ and never will without pulling in a Testo-specific #[Bench] runner it deliberately doesn't depend on; pointing Finder at a directory that doesn't exist broke cs/build on every PHP job and prefer-lowest. Documented the omission in AGENTS.md. - tests/PropertyCheckTest.php: isolate PROPERTY_* the same way the other two suites already do (setUp/tearDown via Support/Env) — this one was missed. - psalm.xml: drop the redundant Exception::__construct suppression, keep AssertionFailedError::__construct. Verified empirically that Psalm keys InternalMethod suppression to the class named at the `new` call site, not to the class that actually declares the constructor (which is what its own diagnostic text names) — documented the gotcha in both psalm.xml and AGENTS.md so it doesn't get "simplified" back. - rector.php: the three dead-code skips (reflection-driven fixtures) were copied from the Testo/core templates verbatim but don't apply here — this adapter has no <method>Generators()-by-reflection convention, forAll() takes generators as a plain argument. Verified rector finds nothing to remove without them; kept only the actually-needed RemoveUselessVarTagRector skip (@var mixed suppressing Psalm's MixedAssignment). Reviewed and kept as-is: examples/README.md's documented `docker run ... composer:2 vendor/bin/phpunit ...` command runs phpunit correctly — verified live (composer:2's entrypoint execs a non-composer command directly).
…t source The previous commit made PropertyCheck emit a literal "\n" for its report lines, but three AdapterDetailsTest assertions still built their EXPECTED string with `. PHP_EOL` — so on Windows the source now emits \n while the test still wants \r\n, and the Windows job failed the other direction than before. Same root cause (report lines must be \n everywhere, source and test), same fix, the other half of it. Grepped the whole tree this time — no PHP_EOL left outside vendor/.
1 participant
Brings the package to its 0.1.0 state: a fluent
forAll()trait on top ofrasuvaeff/property-testing-core^0.1.What this package owns: only the PHPUnit boundary — one
AssertionFailedErrorcarrying the original throwable asprevious,Assumereported as a discard inside the property rather than a skipped PHPUnit test, the distribution report and discard warning, and a verbose listener. Generation, integrated shrinking, the regression corpus and the event model all come from core; there is no algorithm here.Environment parity with the Testo adapter is a tested contract, not a claim:
PROPERTY_RUNS,PROPERTY_SEED,PROPERTY_VERBOSEandPROPERTY_DBbehave identically, and a corpus written by one adapter is read by the other (EnvironmentParityTest).No
#[Property]attribute in 0.1, deliberately. PHPUnit's public extension API observes execution but offers no supported way to intercept and replace a test method call; this adapter does not reach into PHPUnit internals. An attribute can follow only if it becomes implementable on documented API.The test suite isolates itself from the ambient environment in both directions:
setUpclears everyPROPERTY_*variable andtearDownrestores the caller's original values instead of deleting them.Verification:
composer buildgreen against core 0.1.0 resolved from Packagist, psalm level 1 clean (two narrowissueHandlersat the PHPUnit boundary), mutation 104/113 with nine equivalent survivors (gate 90 — 100 is unreachable without ignores, which are forbidden here),bin/package-audit0 errors / 0 warnings,zizmor --persona=auditorclean. TheBackward compatibilityjob skips by design — there is no tag yet.Summary by CodeRabbit