Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

How to test application service command handlers dealing with read models? #52

Open
webdevilopers opened this issue Nov 19, 2020 · 12 comments

Comments

@webdevilopers
Copy link
Owner

webdevilopers commented Nov 19, 2020

The following implementation uses a CQRS Login Query through the service bus.
The authentication currently is based on Symfony Security and the UserPasswordEncoderInterface.

How would you test this scenario?

  1. First of all, would you move the logic away from the handler e.g. to a "LoginService"?

  2. Would you even consider loading the WRITE model (event-sourced aggregate-root) though you don't want to change state?

Though you could add events e.g. "LoginFailed" to fill some audit log. But this actually is a query that should return a valid token.

  1. Would you e.g. mock the repository or type-hint the interface and test different read model scenarios. Keeping the logic inside the command handler?

Login Controller

final class LoginController
{
    private MessageBusInterface $queryBus;

    public function __construct(MessageBusInterface $queryBus)
    {
        $this->queryBus = $queryBus;
    }

    public function __invoke(Request $request): Response
    {
        $query = new LoginHost(json_decode($request->getContent(), true));

        $envelope = $this->queryBus->dispatch($query);
        $handledStamp = $envelope->last(HandledStamp::class);

        return new JsonResponse($handledStamp->getResult(), Response::HTTP_OK);
    }
}

Read Model

final class HostReadModel
{
    private HostId $hostId;

    private EmailAddress $emailAddress;

    private EncodedPassword $password;

    private bool $verified = false;

    private string $locale = 'de';

    public static function fromArray(array $data): HostReadModel
    {
        $self = new self();
        $self->hostId = HostId::fromString($data['hostId']);
        $self->emailAddress = EmailAddress::fromString($data['emailAddress']);
        $self->password = EncodedPassword::fromString($data['password']);
        $self->verified = $data['verified'];
        $self->locale = 'de';

        return $self;
    }

    public function isVerified(): bool
    {
        return $this->verified;
    }
}

Command handler

final class LoginHostHandler
{
    private HostDetailsFinder $hostDetailsFinder;

    private UserPasswordEncoderInterface $passwordEncoder;

    private JWTEncoderInterface $jwtEncoder;

    public function __construct(
        HostDetailsFinder $hostDetailsFinder,
        UserPasswordEncoderInterface $passwordEncoder,
        JWTEncoderInterface $jwtEncoder
    )
    {
        $this->hostDetailsFinder = $hostDetailsFinder;
        $this->passwordEncoder = $passwordEncoder;
        $this->jwtEncoder = $jwtEncoder;
    }

    public function __invoke(LoginHost $query): array
    {
        /** @var HostReadModel $hostDetails */
        $hostDetails = $this->hostDetailsFinder->ofEmailAddress($query->emailAddress()); // CQRS Read Model

        if (!$hostDetails->isVerified()) {
            throw new HostNotVerifiedException();
        }

        if (!$this->passwordEncoder->isPasswordValid($hostDetails, $query->password()->toString())) {
            throw new InvalidCredentialsException();
        }

        return ['token' => $this->jwtEncoder->encode($hostDetails->toArray())];
    }
}
@webdevilopers
Copy link
Owner Author

Our current basic test:

final class LoginControllerTest extends AbstractControllerTest
{
    /** @test */
    public function itCanBeAccessedAnonymously(): void
    {
        $commandHandlerMock = $this->getMockBuilder(LoginHostHandler::class)
            ->disableOriginalConstructor()
            ->getMock();

        $this->client->getContainer()->set(LoginHostHandler::class, $commandHandlerMock);

        $content = [
            'emailAddress' => '[email protected]',
            'password' => 'test1234'
        ];

        $this->client->request('POST', '/login', [], [], [], json_encode($content));

        $this->assertTrue($this->client->getResponse()->isSuccessful());
    }
}

@HellPat
Copy link

HellPat commented Nov 19, 2020

Seems that your example is not correct, HostReadModel is never beeing used?
If you want to test "service command handlers dealing with read models" mocking them seems somehow wrong?

IMO you can use your real LoginHostHandler and mock away every dependency of ``LoginHostHandler` which have side effects?

@webdevilopers
Copy link
Owner Author

Seems that your example is not correct, HostReadModel is never beeing used?

Thank you! The $hostDetails is the read model. I added a @var doc to it.

@webdevilopers
Copy link
Owner Author

IMO you can use your real LoginHostHandler and mock away every dependency of ``LoginHostHandler` which have side effects?

I must confess I did not understand this line. Could you please explain?

@HellPat
Copy link

HellPat commented Nov 19, 2020

The $hostDetails is the read model. I added a @var doc to it.

ah okay, thanks :-)

Your subject under test is your LoginHostHandler (as stated in the ticket title).
And in your example test you mock exactly this LoginHostHandler

$commandHandlerMock = $this->getMockBuilder(LoginHostHandler::class)
            ->disableOriginalConstructor()
            ->getMock();

Then you overwrite the LoginHostHandler with your mock:

$this->client->getContainer()->set(LoginHostHandler::class, $commandHandlerMock);

then you call your Application (via http I guess)

$this->client->request('POST', '/login', [], [], [], json_encode($content));

So the question is: why don't you just test the handler it self (as stated in the issue title)?

You could do this, while abstracting the readModel away:

function it_errors_on_unverified_host()
{
    $alwaysUnverified = new class implements HostDetailsFinder {
        public function isVerified() { return false; }
    }

    $subjectUnderTest = new LoginHostHandler(
        $alwaysUnverified,
        new UserPasswordEncoder
        new JWTEncoder
    );

    $this->expectException(HostNotVerifiedException::class);

    $subjectUnderTest(new LoginHost([
          'emailAddress' => '[email protected]',
          'password' => 'test1234'
    ]));
}

What did we do:

  1. we unit-tested LoginHostHandler in isolation, without framework
  2. we mocked away the readmodel, since it's not "subject under test"
  3. since readmodel is mocked you can test multiple scenarios

If you want to functional test your app you can mock the readmodel away the same way, but for a functional test I'd prefer to produce the data you need and test your application end2end.

Regarding your questions:

First of all, would you move the logic away from the handler e.g. to a "LoginService"?

no would't.
After all your Handler is already a single function service - reusable - totally fine.

Would you even consider loading the WRITE model (event-sourced aggregate-root) though you don't want to change state?

only if i have to read state from the aggregate root without eventual concistency.
Since you already have a ReadModel I'd use that.

Would you e.g. mock the repository or type-hint the interface and test different read model scenarios. Keeping the logic inside the command handler?

Im not 100% sure what you mean here, but when testing in isolation it's very easy to test multiple scenarios based on the ReadModel.

Hope I understood your questions :-)

@webdevilopers
Copy link
Owner Author

Sorry for the late reply @HellPat !

So the question is: why don't you just test the handler it self (as stated in the issue title)?

I must confess the featured test file was confusing. The controller actually was only used to check if routes are correctly protected by the authentication.
The handlers aren't really part of this test. And mocking was the easiest way to get through to the thrown http status.

@webdevilopers
Copy link
Owner Author

First of all, would you move the logic away from the handler e.g. to a "LoginService"?

no would't.
After all your Handler is already a single function service - reusable - totally fine.

That's good news.

    $alwaysUnverified = new class implements HostDetailsFinder {
        public function isVerified() { return false; }
    }

That is a very helpful part! I think that was the missing link.

I will give it a try! Thank you very much.

@HellPat
Copy link

HellPat commented Nov 25, 2020

You're welcome. Let me know if you have further questions an I'll share my opinion

@webdevilopers
Copy link
Owner Author

BTW @HellPat Are you on Twitter?

@HellPat
Copy link

HellPat commented Nov 26, 2020

Yep, this is how I found your Issue :-)

https://twitter.com/psren

@webdevilopers
Copy link
Owner Author

webdevilopers commented Dec 4, 2020

final class UniqueValueAddedTaxIdentifierNumberPolicy
{
    private HostReadModelRepository $repository;

    public function __construct(HostReadModelRepository $repository)
    {
        $this->repository = $repository;
    }

    public function isSatisfiedBy(HostId $hostId, ValueAddedTaxIdentificationNumber $vatId): bool
    {
        try {
            $host = $this->repository->ofVatId($vatId);
        } catch (HostNotFoundException $exception) {
            return true;
        }

        if ($host->hostId()->sameValueAs($hostId)) {
            return true;
        }

        return false;
    }
}
interface HostReadModelRepository
{
    public function ofHostId(HostId $hostId): HostReadModel;

    public function ofEmailAddress(EmailAddress $emailAddress): HostReadModel;

    public function ofVerificationToken(string $token): HostReadModel;

    public function ofVatId(ValueAddedTaxIdentificationNumber $vatId): HostReadModel;
}
final class UniqueValueAddedTaxIdentifierNumberPolicyTest extends TestCase
{
    /** @test */
    public function itIsSatisfiedWhenVatIdIsUsedBySameHost(): void
    {
        $hostReadModelRepository = new class implements HostReadModelRepository {
            public function ofHostId(HostId $hostId): HostReadModel
            {}

            public function ofEmailAddress(EmailAddress $emailAddress): HostReadModel
            {}

            public function ofVerificationToken(string $token): HostReadModel
            {}

            public function ofVatId(ValueAddedTaxIdentificationNumber $vatId): HostReadModel
            {
                return HostReadModel::fromArray([
                    'hostId' => 'b38f676e-221d-4807-97c9-8f549b13425e',
                    'emailAddress' => '[email protected]',
                    'verified' => true,
                    'profileCompleted' => true,
                    'activePlaces' => false,
                    'activePlan' => false,
                ]);
            }
        };

        $policy = new UniqueValueAddedTaxIdentifierNumberPolicy($hostReadModelRepository);

        $this->assertTrue($policy->isSatisfiedBy(
            HostId::fromString('b38f676e-221d-4807-97c9-8f549b13425e'),
            ValueAddedTaxIdentificationNumber::fromString('DE 123456789')
        ));
    }
}

Hi @HellPat , here is a different unit test that also involves repositories.
Following your example I recognized that anonymous classes do not have to implement the body or return value of the interface.

    public function ofHostId(HostId $hostId): HostReadModel;

Can be written...

new class implements HostReadModelRepository {
            public function ofHostId(HostId $hostId): HostReadModel
            {}

...without any return value at all.

Though PHPStorm warns me about it:

Screenshot from 2020-12-04 15-56-06

Should I change this and / or are there any more ways to shorten the code for the test?

Thanks in advance.

@HellPat
Copy link

HellPat commented Dec 7, 2020

Yeah,
If you have an Interface you MUST fulfill it. The more methods you have, the more complex it gets.

In general I'd not mock the repository.
The repository is interacting with your database, and if you test your Repository you should test it against the database.

e.g.

  • perform some actions (commands)
  • look if these actions lead to the desired result (read: the repository returns the right things)

If you want to test UniqueValueAddedTaxIdentifierNumberPolicy you could pass the Readmodel directly.

e.g.

final class UniqueValueAddedTaxIdentifierNumberPolicy
{
    public function isSatisfiedBy(HostId $hostId, HostReadModel $host): bool
    {
        return $host->hostId()->sameValueAs($hostId);
    }
}

You can catch HostNotFoundException at the place you use the Repository (where UniqueValueAddedTaxIdentifierNumberPolicy is beeing used, or in a global ExceptionHandler)

Your looks like

final class UniqueValueAddedTaxIdentifierNumberPolicyTest extends TestCase
{
    /** @test */
    public function itIsSatisfiedWhenVatIdIsUsedBySameHost(): void
    {
        $hostReadModel = HostReadModel::fromArray([
               'hostId' => 'b38f676e-221d-4807-97c9-8f549b13425e',
                'emailAddress' => '[email protected]',
                'verified' => true,
                'profileCompleted' => true,
                'activePlaces' => false,
                'activePlan' => false,
            ]);

        $policy = new UniqueValueAddedTaxIdentifierNumberPolicy();

        $this->assertTrue($policy->isSatisfiedBy(
            HostId::fromString('b38f676e-221d-4807-97c9-8f549b13425e'),
            $hostReadModel
        ));
    }
}

but to be honest, you should already have a test for sameValueAs so the class UniqueValueAddedTaxIdentifierNumberPolicy is not necessary at all.

I'm not entirely sure what you want to test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants