diff --git a/CLAUDE.md b/CLAUDE.md index 8c5ec9291..8da7f8a6e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -228,6 +228,14 @@ A few harness details bite when writing an integration test against a real branc never executed it. Let the run finish, or kill it (inside the container — see the gotcha above) and start again. `git stash push -- ` is how to get one change verified on its own when two of them are in the tree. +- **A model factory that stamps `time()` cannot be compared against one the test built.** + `AccountUseCases::create()` and `::updatePassword()` stamp `passDate` with `time()`, and five + expectations in `AccountTest` built theirs by calling the same factory — so whenever the second + ticked between the two calls the models differed by one and CI went red in whichever pull request + happened to be open. It is not reproducible on demand, which is what made it read as a mystery + rather than a bug. `anAccountStampedNow()` compares everything else exactly and takes only + `passDate` from the actual, after checking it is a timestamp from the last few seconds. Injecting + `sleep(1)` before the write is how to reproduce it, and how to show a fix works. - **Faker's `randomNumber($n)` includes zero**, and forms read a zero id as "not given". A fixture drawing a group or profile id that way fails about one run in a hundred, on CI, in whichever pull request happened to be open. Use `numberBetween(1, …)`. diff --git a/tests/Unit/Application/Account/Services/AccountTest.php b/tests/Unit/Application/Account/Services/AccountTest.php index 9dceff6a1..ee69114eb 100644 --- a/tests/Unit/Application/Account/Services/AccountTest.php +++ b/tests/Unit/Application/Account/Services/AccountTest.php @@ -1144,7 +1144,7 @@ public function testEditPassword() ); $this->accountRepository->expects(self::once())->method('editPassword') - ->with($id, AccountModel::updatePassword($accountUpdateDto)); + ->with($id, self::anAccountStampedNow(AccountModel::updatePassword($accountUpdateDto))); $this->account->editPassword($id, $accountUpdateDto); } @@ -1186,7 +1186,7 @@ public function testEditPasswordDefaultsOmittedUserEditIdToTheCurrentSessionUser ); $this->accountRepository->expects(self::once())->method('editPassword') - ->with($id, AccountModel::updatePassword($expectedDto)); + ->with($id, self::anAccountStampedNow(AccountModel::updatePassword($expectedDto))); $this->account->editPassword($id, $accountUpdateDto); } @@ -1333,7 +1333,7 @@ public function testCreate() $encryptedDto = $accountCreateDto->withEncryptedPassword($encryptedPassword); $this->accountRepository->expects(self::once())->method('create') - ->with(AccountModel::create($encryptedDto)) + ->with(self::anAccountStampedNow(AccountModel::create($encryptedDto))) ->willReturn(new QueryResult(null, 0, $id)); $this->accountItemsService->expects(self::once())->method('addItems') @@ -1385,7 +1385,7 @@ public function testCreateCannotChangePermissions() $encryptedDto = $accountCreateDto->withEncryptedPassword($encryptedPassword); $this->accountRepository->expects(self::once())->method('create') - ->with(AccountModel::create($encryptedDto)) + ->with(self::anAccountStampedNow(AccountModel::create($encryptedDto))) ->willReturn(new QueryResult(null, 0, $id)); $this->accountItemsService->expects(self::once())->method('addItems') @@ -1568,7 +1568,7 @@ public function testCreateDefaultsOmittedOwnerAndGroupToTheCreatingUser() ->withEncryptedPassword($encryptedPassword); $this->accountRepository->expects(self::once())->method('create') - ->with(AccountModel::create($expectedDto)) + ->with(self::anAccountStampedNow(AccountModel::create($expectedDto))) ->willReturn(new QueryResult(null, 0, $id)); $this->accountItemsService->expects(self::once())->method('addItems') @@ -1746,6 +1746,34 @@ public function testIncrementDecryptCounterNoRows() $this->assertFalse($this->account->incrementDecryptCounter($id)); } + /** + * The account the service is expected to write, give or take the second it was stamped in. + * + * `AccountUseCases::create()` and `::updatePassword()` stamp `passDate` with `time()`, and a + * test that builds its expectation by calling the same factory calls `time()` a moment before + * the production code does. Whenever the second ticks between the two the models differ by one + * and the test fails — a real failure on CI, roughly once in a few hundred runs, in whichever + * pull request happened to be open. It is not the code being wrong, and it is not reproducible + * on demand, which is what made it read as a mystery. + * + * Everything else is still compared exactly: only `passDate` is taken from the actual model, + * and only after it has been checked for being a timestamp from the last few seconds — so a + * factory that stopped stamping it, or stamped something else, still fails. + */ + private static function anAccountStampedNow(AccountModel $expected): Callback + { + return self::callback(static function (AccountModel $actual) use ($expected): bool { + $now = time(); + + self::assertGreaterThanOrEqual($now - 10, $actual->getPassDate()); + self::assertLessThanOrEqual($now, $actual->getPassDate()); + + self::assertEquals($expected->mutate(['passDate' => $actual->getPassDate()]), $actual); + + return true; + }); + } + protected function setUp(): void { parent::setUp();