Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Uid] AddMockUuidFactory for deterministic UUID generation in tests#61807
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
nicolas-grekas left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think we shouldn't extend the current API of factories. Let's inherit from UuidFactory instead, and let's remove all the added methods, they're breaking the design of the factory API: v3+5 don't make sense mocking, v1/6/7 are all time-based and the abstraction can be kept, v4 is already what therandomBased method returns so we don't need new methods, etc.
Uh oh!
There was an error while loading.Please reload this page.
39e3304 to56eae5dCompareUh oh!
There was an error while loading.Please reload this page.
56eae5d to1074c35CompareUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
d09b379 toc307d8fComparenicolas-grekas left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Instead of a review, I thought it would be quicker to write the factory, so here is how I see this could be. If you could give it a try and add tests that'd be awesome!
<?php/* * This file is part of the Symfony package. * * (c) Fabien Potencier <fabien@symfony.com> * * For the full copyright and license information, please view the LICENSE * file that was distributed with this source code. */namespaceSymfony\Component\Uid\Factory;useSymfony\Component\Uid\TimeBasedUidInterface;useSymfony\Component\Uid\Uuid;useSymfony\Component\Uid\UuidV3;useSymfony\Component\Uid\UuidV4;useSymfony\Component\Uid\UuidV5;useSymfony\Component\Uid\UuidV7;class MockUuidFactoryextends UuidFactory{private\Iterator$sequence;/** * @param iterable<string|Uuid> $uuids */publicfunction__construct(iterable$uuids,privateUuid|string|null$timeBasedNode =null,privateUuid|string|null$nameBasedNamespace =null, ) {$this->sequence =match (true) {\is_array($uuids) =>new \ArrayIterator($uuids),$uuidsinstanceof \Iterator =>$uuids,$uuidsinstanceof \Traversable =>new \IteratorIterator($uuids), }; }publicfunctioncreate():Uuid {if (!$this->sequence->valid()) {thrownew \RuntimeException('No more UUIDs in sequence.'); }$uuid =$this->sequence->current();$this->sequence->next();return$uuidinstanceof Uuid ?$uuid : Uuid::fromString($uuid); }publicfunctionrandomBased():RandomBasedUuidFactory {if (!($uuid =$this->create())instanceof UuidV4) {thrownew \RuntimeException(\sprintf('Next UUID in sequence is not a UuidV4: "%s".',get_debug_type($uuid))); }returnnewclass($uuid)extends RandomBasedUuidFactory {publicfunction__construct(privateUuidV4$uuid, ) { }publicfunctioncreate():UuidV4 {return$this->uuid; } }; }publicfunctiontimeBased(Uuid|string|null$node =null):TimeBasedUuidFactory {if (!($uuid =$this->create())instanceof TimeBasedUidInterface) {thrownew \RuntimeException(\sprintf('Next UUID in sequence is not a TimeBasedUidInterface: "%s".',get_debug_type($uuid))); }if (\is_string($node ??=$this->timeBasedNode)) {$node = Uuid::fromString($node); }returnnewclass($uuid,$node)extends TimeBasedUuidFactory {publicfunction__construct(privateTimeBasedUidInterface$uuid,private ?Uuid$node =null, ) {if ($uuidinstanceof UuidV7) {$this->node =null; } }publicfunctioncreate(?\DateTimeInterface$time =null):Uuid&TimeBasedUidInterface {if (null !==$time &&$this->uuid->getDateTime() !=$time) {thrownew \RuntimeException(\sprintf('Next UUID in sequence does not match the expected time: "%s" != "%s".',$this->uuid->getDateTime()->format('@U.uT'),$time->format('@U.uT'))); }if (null !==$this->node &&$this->uuid->getNode() !==substr($this->node->toRfc4122(), -12)) {thrownew \RuntimeException(\sprintf('Next UUID in sequence does not match the expected node: "%s" != "%s".',$this->uuid->getNode(),substr($this->node->toRfc4122(), -12))); }return$this->uuid; } }; }publicfunctionnameBased(Uuid|string|null$namespace =null):NameBasedUuidFactory {if (!($uuid =$this->create())instanceof UuidV5 && !$uuidinstanceof UuidV3) {thrownew \RuntimeException(\sprintf('Next UUID in sequence is not a UuidV5 or UuidV3: "%s".',get_debug_type($uuid))); }$factory =parent::nameBased($namespace);returnnewclass ($uuid,$factory)extends NameBasedUuidFactory {publicfunction__construct(privateUuidV5|UuidV3$uuid,privateNameBasedUuidFactory$factory, ) { }publicfunctioncreate(string$name):UuidV5|UuidV3 {if ($this->uuid->toRfc4122() !==$this->factory->create($name)->toRfc4122()) {thrownew \RuntimeException(\sprintf('Next UUID in sequence does not match the expected named UUID: "%s" != "%s".',$this->uuid->toRfc4122(),$this->factory->create($name)->toRfc4122())); }return$this->uuid; } }; }}
Ah, I can already tell you I made a mistake: the sequence should be consumed in child factories, not in the main one ! |
@nicolas-grekas looks good I'll try you suggestion. And about the sequence I'll take care of it. |
e2df988 to1fa277bCompare2056709 tofce9b9dCompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
thanks for the new tests!
here are some more tweaks for the code and then we should be good to go!
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
3640d31 to8464f5eCompareMockUuidFactory for deterministic UUID generation in testsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Many thanks 🚀
8464f5e toadb84afCompareThank you@momito69. |
0c7ba55 intosymfony:7.4Uh oh!
There was an error while loading.Please reload this page.
In order to enjoy fully the MockUuidFactory, should we expose something similar to |
i don't know if it would be needed, but looking at the UuidFactory classes, i would suspect them to use Interfaces instead Like should MockUuidFactory extend from UuidFactory if none of the functions and properties are used? Same with |
We'd rather figure out a way to have Doctrine use a factory instead. |
if you use a custom id generator in the ORM, it can use any dependency it wants. And If you want to generate the id yourselves in the constructor of your entities, you cannot use dependency injection at all (as the instantiation is not managed by the DI component). But I don't think that's a reason to introduce a global state for accessing the configured factory. |
Indeed, I was doing in my entity |
Similar to the Clock component's ClockInterface and MockClock, I've made a MockUuidFactory and implemented an UuidFactoryInterface to have a way to mock UUID generation for testing purposes.
cc@OskarStark ,@alexandre-daubois