diff --git a/CHANGELOG.md b/CHANGELOG.md index 7cbba9b..cedd52a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,19 @@ # 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. +- 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 - `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/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/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/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(); + } + } } 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 {