From d0e1a0584011ed91337e0c671586ea53bf6eb058 Mon Sep 17 00:00:00 2001 From: "v.razuvaev" Date: Thu, 20 Aug 2026 21:54:04 +0300 Subject: [PATCH 1/2] Warn on a helper-derived property id; reject DSN credentials - forAll() derives the property id from its caller; called from a helper (not the test method) that id names the helper, so every test using the helper shares one corpus entry and overwrites the others' counterexample. It now warns on stderr, like a closure-derived id already did; ->id() silences it. The signal is a mismatch between the derived method and TestCase::name(). - RedisDsn rejects a DSN carrying userinfo instead of dropping it (the connection would otherwise go without AUTH); the error never echoes the DSN. Fixes #14 Fixes #15 --- CHANGELOG.md | 11 +++++ README.md | 4 +- README.ru.md | 3 +- psalm.xml | 8 ++++ src/PhpUnit/PropertyCheck.php | 25 ++++++++++- src/PhpUnit/PropertyTesting.php | 4 ++ src/PhpUnit/RedisDsn.php | 15 ++++++- tests/EnvironmentParityTest.php | 73 +++++++++++++++++++++++++++++++-- tests/PropertyCheckTest.php | 2 + tests/RedisDsnTest.php | 23 +++++++++++ 10 files changed, 161 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7cbba9b..666ad8c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,16 @@ # Changelog +## 0.5.2 — 2026-08-20 + +- `forAll()` called from a helper (not directly in the test method) now warns + on stderr, the way a closure-derived id already did. Its id names the helper, + so every test using that helper shares one corpus entry and overwrites the + others' counterexample — a stable-looking id that is silently wrong. Pin it + with `->id()` to silence the warning. +- `PROPERTY_DB` with credentials in its userinfo (`redis://user:pass@host`) is + rejected instead of silently dropped — `parse_url` would discard them and the + connection would go without AUTH. The error never echoes the DSN. + ## 0.5.1 — 2026-08-20 - `PROPERTY_DB` with a non-`redis` URI scheme is now a configuration error diff --git a/README.md b/README.md index 0227f6f..a8c5d78 100644 --- a/README.md +++ b/README.md @@ -233,7 +233,9 @@ deleted when the job ends. The Redis form is the same corpus, in the same document, shared. It needs `ext-redis` or `predis/predis`; neither installed is an error rather than a silent fall back to the filesystem. A `PROPERTY_DB` with any other scheme — a `rediss://` typo, another backend — is likewise an error, -never a directory named after the scheme. +never a directory named after the scheme. Credentials in the DSN +(`redis://user:pass@host`) are rejected rather than silently dropped; configure +Redis AUTH out of band. The corpus format is exactly the one `rasuvaeff/property-testing` 2.8 wrote — a corpus recorded under Testo (or under 2.x) replays here and vice versa. On diff --git a/README.ru.md b/README.ru.md index 3d9f6f7..0f5d0d9 100644 --- a/README.ru.md +++ b/README.ru.md @@ -232,7 +232,8 @@ PROPERTY_DB=redis://redis:6379/suite-a: vendor/bin/phpunit # общий се документе, но общий. Нужен `ext-redis` или `predis/predis`; отсутствие обоих — ошибка, а не тихий откат на файловую систему. `PROPERTY_DB` с любой другой схемой — опечатка `rediss://`, другой бэкенд — тоже ошибка, а не каталог с -именем схемы. +именем схемы. Учётные данные в DSN (`redis://user:pass@host`) отклоняются, а не +молча отбрасываются; настраивайте Redis AUTH отдельно. Формат корпуса — ровно тот, что писал `rasuvaeff/property-testing` 2.8: корпус, записанный под Testo (или под 2.x), реплеится здесь, и наоборот. При diff --git a/psalm.xml b/psalm.xml index 45edfd9..d135e4c 100644 --- a/psalm.xml +++ b/psalm.xml @@ -38,6 +38,14 @@ + + diff --git a/src/PhpUnit/PropertyCheck.php b/src/PhpUnit/PropertyCheck.php index 13abcc8..ee15f4c 100644 --- a/src/PhpUnit/PropertyCheck.php +++ b/src/PhpUnit/PropertyCheck.php @@ -98,6 +98,7 @@ public function __construct( private string $id, private string $name, private readonly array $generators, + private bool $idDerivedIndirectly = false, ) {} /** @@ -120,6 +121,9 @@ public function id(string $id): self { $this->id = $id; $this->name = $id; + // Pinned explicitly: the derivation no longer matters, so drop the + // indirect-derivation warning. + $this->idDerivedIndirectly = false; return $this; } @@ -475,7 +479,7 @@ private function reportClassifications(RunStatistics $statistics): void */ private function warnOnUnstableId(): void { - $warning = PropertyId::unstableWarning($this->id); + $warning = PropertyId::unstableWarning($this->id) ?? $this->indirectIdWarning(); if ($warning === null) { return; @@ -484,6 +488,25 @@ private function warnOnUnstableId(): void fwrite($this->stderr, $warning . "\n"); } + /** + * `forAll()` was called from a helper (or otherwise not the running test + * method), so its derived id names the helper, not the test — every caller + * of that helper shares one corpus entry and overwrites the others' + * counterexample. `PropertyId::unstableWarning()` misses this because the id + * is a stable-looking `Class::method`, just the wrong method. + */ + private function indirectIdWarning(): ?string + { + if (!$this->idDerivedIndirectly) { + return null; + } + + return sprintf( + 'Property id "%s" was derived from a helper, not the test method that ran, so every call site shares one corpus entry: call forAll() directly in the test method or pass an explicit property id with ->id()', + $this->id, + ); + } + private function warnOnExcessiveSkips(RunStatistics $statistics): void { $skips = $statistics->discards; diff --git a/src/PhpUnit/PropertyTesting.php b/src/PhpUnit/PropertyTesting.php index 51a7752..827f49d 100644 --- a/src/PhpUnit/PropertyTesting.php +++ b/src/PhpUnit/PropertyTesting.php @@ -45,6 +45,10 @@ final protected function forAll(array $generators = []): PropertyCheck id: static::class . '::' . $method, name: $method, generators: $generators, + // Derived from something other than the running test method — a + // helper or a closure — so the id is shared across call sites and + // collides in the corpus. PropertyCheck warns unless id() pins it. + idDerivedIndirectly: $method !== $this->name(), ); } } diff --git a/src/PhpUnit/RedisDsn.php b/src/PhpUnit/RedisDsn.php index c6e5e48..c0579df 100644 --- a/src/PhpUnit/RedisDsn.php +++ b/src/PhpUnit/RedisDsn.php @@ -45,14 +45,27 @@ public function toPredisParameters(): array } /** - * @param string $dsn The value of `PROPERTY_DB`, already known to start with `redis://`. + * @param string $dsn The value of `PROPERTY_DB`, already known to use the `redis` scheme. */ public static function parse(string $dsn): self { $parts = parse_url($dsn); + + if (is_array($parts) && (isset($parts['user']) || isset($parts['pass']))) { + // Reject credentials rather than drop them silently: parse_url would + // discard the userinfo, so the connection would go without AUTH + // while the operator believes it authenticated. The message never + // echoes the DSN — it would carry the password into the CI log. + throw new \InvalidArgumentException( + 'PROPERTY_DB carries credentials in its userinfo, which is not supported; configure Redis AUTH out of band', + ); + } + $host = is_array($parts) ? ($parts['host'] ?? null) : null; if (!is_string($host) || $host === '') { + // Safe to quote the DSN: a credentialed one was already rejected + // above, so whatever reaches here carries no userinfo. throw new \InvalidArgumentException(sprintf( 'PROPERTY_DB="%s" is not a usable Redis DSN; expected redis://host[:port][/key-prefix]', $dsn, diff --git a/tests/EnvironmentParityTest.php b/tests/EnvironmentParityTest.php index 3c582c2..00c555d 100644 --- a/tests/EnvironmentParityTest.php +++ b/tests/EnvironmentParityTest.php @@ -474,6 +474,72 @@ public function testANamedPropertyWarnsAboutNothing(): void self::assertSame('', (string) stream_get_contents($stderr)); } + public function testAHelperDerivedIdIsReportedOnStderr(): void + { + $stderr = fopen('php://memory', 'w+'); + self::assertIsResource($stderr); + + // forAll() is called inside checkViaHelper(), so the derived id names + // the helper, not this test method — every test using the helper would + // share one corpus entry. + $this->checkViaHelper($stderr); + + rewind($stderr); + $warning = (string) stream_get_contents($stderr); + + self::assertMatchesRegularExpression( + '/^Property id "[^"]*::checkViaHelper" was derived from a helper, not the test method that ran, .*->id\(\)\n$/s', + $warning, + ); + } + + public function testAHelperDerivedIdIsSilencedByAnExplicitId(): void + { + $stderr = fopen('php://memory', 'w+'); + self::assertIsResource($stderr); + + $this->checkViaHelper($stderr, 'pinned::property'); + + rewind($stderr); + + self::assertSame('', (string) stream_get_contents($stderr)); + } + + public function testADirectCallDerivesAStableIdWithoutWarning(): void + { + $stderr = fopen('php://memory', 'w+'); + self::assertIsResource($stderr); + + $this->forAll(['value' => Gen::intBetween(0, 10)]) + ->runs(3) + ->output(STDOUT, $stderr) + ->check(static function (int $value): void { + self::assertGreaterThanOrEqual(0, $value); + }); + + rewind($stderr); + + self::assertSame('', (string) stream_get_contents($stderr)); + } + + /** + * @param resource $stderr + */ + private function checkViaHelper($stderr, ?string $id = null): void + { + $check = $this->forAll(['value' => Gen::intBetween(0, 10)]) + ->runs(3) + ->output(STDOUT, $stderr); + + if ($id !== null) { + $check->id($id); + } + + $check->check(static function (int $value): void { + self::assertGreaterThanOrEqual(0, $value); + }); + } + public function testEdgeCasesOffKeepsBoundaryValuesOutOfTheRun(): void { // The knob's whole purpose: a property that cannot use the edges would @@ -626,9 +692,10 @@ private function runFalsifiableProperty( // practically impossible, so falsification is certain either way. $check = $this->forAll(['value' => Gen::intBetween(0, 10_000)])->runs(100); - if ($id !== null) { - $check->id($id); - } + // Pin the id: forAll() here is one level down from the test method, so + // its derived id would be flagged as helper-derived. Default to exactly + // what it would have derived, so the corpus key is unchanged. + $check->id($id ?? self::class . '::runFalsifiableProperty'); if ($listener instanceof RecordingListener) { $check->listeners($listener); diff --git a/tests/PropertyCheckTest.php b/tests/PropertyCheckTest.php index c062033..eec3b97 100644 --- a/tests/PropertyCheckTest.php +++ b/tests/PropertyCheckTest.php @@ -485,6 +485,8 @@ private function falsifiedOriginal(): mixed { try { $this->forAll(['value' => Gen::intBetween(0, 100_000)]) + // Pinned: forAll() is one level down from the test method here. + ->id(self::class . '::falsifiedOriginal') ->runs(100) ->seed(2026) ->check(static function (int $value): void { diff --git a/tests/RedisDsnTest.php b/tests/RedisDsnTest.php index 7ca1e10..7fc814b 100644 --- a/tests/RedisDsnTest.php +++ b/tests/RedisDsnTest.php @@ -57,6 +57,29 @@ public function testADsnWithoutAHostIsAConfigurationError(): void } } + #[DataProvider('credentialledProvider')] + public function testADsnWithCredentialsIsRejectedWithoutEchoingThePassword(string $dsn): void + { + try { + RedisDsn::parse($dsn); + + self::fail('expected an InvalidArgumentException'); + } catch (\InvalidArgumentException $e) { + self::assertStringContainsString('credentials', $e->getMessage()); + self::assertStringNotContainsString('s3cret', $e->getMessage()); + } + } + + /** + * @return iterable + */ + public static function credentialledProvider(): iterable + { + yield 'user and password' => ['redis://user:s3cret@redis:6379']; + yield 'password only' => ['redis://:s3cret@redis:6379']; + yield 'user only' => ['redis://user@redis:6379']; + } + #[DataProvider('malformedProvider')] public function testAMalformedDsnIsAConfigurationError(string $dsn): void { From 68d8553cf8f044e624d896921647b096a5e8b80c Mon Sep 17 00:00:00 2001 From: "v.razuvaev" Date: Thu, 20 Aug 2026 21:57:05 +0300 Subject: [PATCH 2/2] Memoize the corpus per PROPERTY_DB, matching the Testo adapter Resolving runs once per property; a Redis suite otherwise built a client and opened a connection per property. Keeps corpus resolution at parity with property-testing-testo 0.6.2. --- CHANGELOG.md | 3 +++ src/PhpUnit/CorpusFromEnv.php | 14 ++++++++++++++ tests/CorpusFromEnvTest.php | 16 ++++++++++++++++ 3 files changed, 33 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 666ad8c..cedd52a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,9 @@ - `PROPERTY_DB` with credentials in its userinfo (`redis://user:pass@host`) is rejected instead of silently dropped — `parse_url` would discard them and the connection would go without AUTH. The error never echoes the DSN. +- The resolved corpus is memoized per `PROPERTY_DB` value, so a suite sharing a + Redis corpus builds one client (and opens one connection) rather than one per + property. Mirrors the Testo adapter. ## 0.5.1 — 2026-08-20 diff --git a/src/PhpUnit/CorpusFromEnv.php b/src/PhpUnit/CorpusFromEnv.php index ccd11ae..700bb0a 100644 --- a/src/PhpUnit/CorpusFromEnv.php +++ b/src/PhpUnit/CorpusFromEnv.php @@ -36,6 +36,15 @@ final class CorpusFromEnv */ private const string SCHEME_PATTERN = '#^([a-zA-Z][a-zA-Z0-9+.\-]*)://#'; + /** + * One corpus per distinct `PROPERTY_DB` value. Resolving happens once per + * property, so without this a suite sharing a Redis corpus would build a + * client — and open a connection on first recall — for every property. + * + * @var array + */ + private static array $cache = []; + private function __construct() { // Static helper; not instantiable. @@ -53,6 +62,11 @@ public static function resolve(): ?Corpus return null; } + return self::$cache[$dsn] ??= self::build($dsn); + } + + private static function build(string $dsn): Corpus + { if (preg_match(self::SCHEME_PATTERN, $dsn, $matches) !== 1) { return new FilesystemCorpus($dsn); } diff --git a/tests/CorpusFromEnvTest.php b/tests/CorpusFromEnvTest.php index ca3b1ab..5b36e49 100644 --- a/tests/CorpusFromEnvTest.php +++ b/tests/CorpusFromEnvTest.php @@ -172,4 +172,20 @@ public function testTheRedisSchemeIsMatchedCaseInsensitively(): void $restore(); } } + + /** + * Resolving runs once per property, so the same `PROPERTY_DB` must hand back + * the same corpus — otherwise a Redis suite builds a client, and opens a + * connection on first recall, for every property. + */ + public function testResolvesTheSameCorpusInstanceForTheSameDsn(): void + { + $restore = Env::set('PROPERTY_DB', sys_get_temp_dir() . '/property-testing-phpunit-memoized'); + + try { + self::assertSame(CorpusFromEnv::resolve(), CorpusFromEnv::resolve()); + } finally { + $restore(); + } + } }