Unit testing value objects with internal datetime calculation about php-ddd HOT 14 OPEN

I stick to the rule that, if an object needs information from outside the application (an API response, the current time, etc.) there are four options:

• It should become a service class with an abstraction (so you can control the outcome in a test environment)
• If it's already a service, it should get an abstraction injected for fetching that information.
• If it's not a service but a value object, entity, etc. it should get the information passed to it as a method argument, because that's the only non-hacky way to make its behavior predictable.
• Less conventional, but totally an option: pass the service abstraction as an argument to the entity, value object, etc. can call it to get the information.

I'm not saying you should stick to my rule, but I find that things will just be hacky if you don't do it (or that you'll eventually wish you had introduced the abstraction sooner). If you don't do it, you'll have an indirect/hidden dependency anyway.

from php-ddd.

The time() function call is essentially a service call pretending to be a normal calculation without dependencies. The language and its API just make it feel so. In fact, the current time does not come from your application. It comes from an external service, called system clock, and it's not controlled by your application. So you are kind of already using a service to get the current time, except this service stinks because its design and the way it's used prevent testability.

My suggestion: It should be treated the same way you would treat any other external service dependency: part of the infrastructure and behind an interface that can be substituted during testing.

It's no different from an imaginary get_current_cpu_usage() function or even a get_current_weather() function that obviously calls an external service not under your control.

from php-ddd.

In my opinion,

generating an UUID v4 within a domain object is a violation of the definition of a Domain model (because it relies on an external resource, being a RNG).

Following strictly the rule would mean to provide the uuid string to the object :

class FooId
{
   private function __construct(private string $value)
   {
   }
   
   public static function fromString(string $value): self
   {
       if (!uuid_valid($value)) {
          throw new \InvalidArgumentException;
       }
   }
}

However, because it is more convenient, and because we hardly ever need to have predictable uuid value, I tend to agree that generating them within Domain object is ok'ish.

On the other hand, dealing with time is a whole different matter, as we almost always need to have predictable date value. First reason being that having a test suite relying on real time would lead to test failure just because... time has passed (in real life), which would be very painful to maintain.

Which is why I personally avoid to depend on the RTC within Domain model (by injecting instances of DateTime, instead of constructing the object within the model, which is where the RTC is questioned by php, for instance).

from php-ddd.

I must confess I did not know about Carbon. Since it extends the existing DateTime object it would mean less change to my codebase.

// Don't really want this to happen so mock now
Carbon::setTestNow(Carbon::createFromDate(2000, 1, 1));

// comparisons are always done in UTC
if (Carbon::now()->gte($internetWillBlowUpOn)) {
    die();
}

// Phew! Return to normal behaviour
Carbon::setTestNow();

from php-ddd.

I might be missing something:
What's the reason for the expiration date to be created outside the VerificationToken VO? I would tend to do this inside VerificationToken::generate and change the $expiresAt parameter by a $registratedAt one.

For the last part, with the registration date as a parameter or a clock service, this is where I head when I'm dealing with dates.

Though the code looks fine I'm just worried about adding "behaviour & data" that is only required for testing and not originally in the domain logic.

You could see it the way around. When passing data (as a DateTime or as a DateTime provider service), you're making the concept of a registration date more explicit.

If the reflection is what's bothering you, you could use the namespace technique for mocking.

from php-ddd.

Why do you explicitly not want to create a service? Now I see a value object that is able to create its own token with a signing key that is hard coded and a reference to the current time via a direct time() call. It makes the whole thing hard to test (if you would want to test the signing).

In my opinion a value object should not hold logic.

I feel that you are forcing yourself in a situation where you meet some standard while giving in on other standards like separation of concern.

from php-ddd.

@swyrazik that was very eloquently augmented, thanks!

from php-ddd.

Recently I came across the @brick DateTime library and I gave it a try.

• https://github.com/brick/date-time

This is my current example:

final class Subscription extends AggregateRoot
{
    private DateTimeImmutable $expiresAt;

    public function cancel(TerminationReason $reason, ?string $feedback): void
    {
        $this->expiresAt = ...;
    }

    public function markAsExpired(): void
    {
        $now = new DateTimeImmutable();

        if ($now < $this->expiresAt) {
            throw new \DomainException('Subscription has not expired yet.');
        }

        // record events
    }
}

As mentioned by @matthiasnoback the non-hacky way would be passing the date to the method.

If it's not a service but a value object, entity, etc. it should get the information passed to it as a method argument, because that's the only non-hacky way to make its behavior predictable.

    public function markAsExpired(DateTimeImmutable $someDateWhereAnExternalCronjobTaskStarted): void
    {
        if ($someDateWhereACronjobTaskStarted < $this->expiresAt) {
            throw new \DomainException('Subscription has not expired yet.');
        }
    }

Here is the "brick"-approach:

final class Subscription extends AggregateRoot
{
    public function markAsExpired(): void
    {
        $now = LocalDateTime::now(TimeZone::utc());

        if ($now->isBefore($this->expiresAt)) {
            throw new \DomainException('Subscription has not expired yet.');
        }

        // record events
    }
}

And for testing:

...you can change the default clock for all date-time classes. All methods such as now(), unless provided with an explicit Clock, will use the default clock you provide.

• https://github.com/brick/date-time#clocks

final class SubscriptionTest extends TestCase
{
    /** @test */
    public function givenACancelledSubscriptionWhenMarkingAsExpiredItRecordsEvents(): void
    {
        DefaultClock::set(new FixedClock(Instant::of(1630483200))); // 01.09.2021 08:00:00

        $profile = $this->completedProfile();
        $profile->cancelSubscription(TerminationReason::fromString('no_need'), null);

        self::extractRecordedEvents($profile);

        DefaultClock::set(new FixedClock(Instant::of(1633046400))); // 01.10.2021 00:00:00
        $profile->markAsExpired();

        // some events recorded assertions
    }
}

A trade-off is relying on hard-coded LocalDateTime and LocalDate classes. Though it can always be converted back to a DateTime`. A clock instance is preferred for type-hinting and easier to replace.

But passing a clock interface to the aggregate root of my example feels odd - almost like a "dependency injection".

Thoughts? Any experiences with other libraries?

@Slamdunk recommends this library by @lcobucci :

• https://github.com/lcobucci/clock

@danilopolani mentioned the classic Carbon library by @briannesbitt :

• https://github.com/briannesbitt/Carbon

from php-ddd.

IMO, you get the same dependency on an external service whether you're using an argument or a static method. The main difference being that the former is more explicit, which I personally tend to prefer (making the implicit, explicit).

I would stick with either injecting the expiration date, or a clock service that will provide it, to the cancel method.

Nonetheless, it seems fine, though overkill at first sight, to use a clock service implementation relying on Carbon (I haven't used it yet).

from php-ddd.

IMHO, solving the issue by introducing coupling doesn't really solve the issue, it creates other issues (on the long run, sure):

1. Tests will be full of calls to DefaultClock::set(), Carbon::setTestNow(), or anything else - leading people to try and get creative to "remove the duplication" by having even more indirection on the tests
2. Test suite are likely to have flaky tests because of missing "resets" of the clock implementation and shared state - we're only humans, after all

But passing a clock interface to the aggregate root of my example feels odd - almost like a "dependency injection".

That's interesting... what makes it odd, considering that the current time (or a clock interface) is a dependency like TerminationReason (in the case of Subscription#cancel())?

An interesting thing to consider is PSR-20 which would allow you not to depend on a specific clock implementation.

from php-ddd.

considering that the current time (or a clock interface) is a dependency

Totally agree with your statement 👍

BTW, Eric Evans gave a really nice talk about modelling "time". I don't recall it being specifically about dealing with the injection of a clock service within an entity, but is related in some way. here it is: https://www.youtube.com/watch?v=T29WzvaPNc8.

from php-ddd.

That's interesting... what makes it odd, considering that the current time (or a clock interface) is a dependency like TerminationReason (in the case of Subscription#cancel())?

It feels like "injecting an extra service". Normally this is not required because other implementations e.g. UUID Factories are called statically:

Uuid::uuid4();

It feels more "hidden" like the Brick approach:

DefaultClock::set(new FixedClock(Instant::of(1630483200)));

But yes, hidden means "implicit" and maybe "explicite" should be preferred.

Passing a date directly feels similar. And it would require additional logic at least from / for the developer point-of-view. E.g.:

    public function cancel($cancelledAt, TerminationReason $reason, ?string $feedback): void
    {
        if ($cancelledAt < $this->subscribedAt) { //... }

        $this->expiresAt = ...;
    }

from php-ddd.

It feels like "injecting an extra service"

Sure, and what's the problem with that? Wouldn't you do the same when the Entity depends on a domain service?

UUID Factories are called statically

That's a design decision and there are consequences with it too. The key difference is that since we know that a UUID v4 is randomly generated we won't be performing assertions against it - we simply couldn't care less about the generated value.

from php-ddd.

My proposal is that you should always try to inject all such dependencies, no matter how (in)convenient this is. In most of the cases constructor injection works great but in some rare cases (eg. php attrubites or domain object code) parameter inject should do the trick. This way you are perfectly safe for the unit tests.

from php-ddd.