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

Check stamps and final message #5

Open
CharloMez opened this issue Apr 13, 2021 · 34 comments
Open

Check stamps and final message #5

CharloMez opened this issue Apr 13, 2021 · 34 comments

Comments

@CharloMez
Copy link

Is there a way to check stamps and the final message with this library ?
I explain myself.
I use messenger with rabbitmq, and between different microservices.
To perform that, I use a specific stamp to give the routing_key dynamically when i dispatch the message

    $envelope = new Envelope(
            $fooMessage,
            [
                new AmqpStamp("myexchange.{$bar}")
            ]
    );

And secondly I use symfony serializer to not pair the message with specific class.

    messenger:
        serializer:
            default_serializer: messenger.transport.symfony_serializer
            symfony_serializer:
                format: json
                context: { }

and the normalizer:

<?php

namespace App\Serializer\Normalizer;

use Symfony\Component\Messenger\Envelope;
use Symfony\Component\Serializer\Normalizer\NormalizerInterface;

class FooNormalizer implements NormalizerInterface
{
    /**
     * @param Envelope $object
     * @param string|null $format
     * @param array $context
     *
     * @return array
     */
    public function normalize($object, string $format = null, array $context = [])
    {
        /** @var FooMessage $fooMessage */
        $fooMessage = $object->getMessage();

        return [
            'body' => [
                'foo' => $fooMessage->bar(),
            ],
            'headers' => [
                'stamps' => $object->all()
            ]
        ];
    }

    public function supportsNormalization($data, string $format = null)
    {
        return $data instanceof Envelope
            && $data->getMessage() instanceof FooMessage
        ;
    }
}

I would like to perform fonctional testing on it, and for me it's very important to check that my stamp is good and that my final message too (I mean the final json which will really be in my queue).

I saw that you have the serializer in your transport class, is it possible to provide a function to get the serialized message ?
And I don't know how yet, but do you think it is possible to have a function to verify stamps instance and content ?

I don't have time to watch now, but if it is a good idea for you, i can look at it later and make a PR.

thanks !

@kbond
Copy link
Member

kbond commented Apr 13, 2021

but do you think it is possible to have a function to verify stamps instance and content

In your case, would the stamp be available on the envelope?

foreach ($this->messenger()->queue() as $envelope) {
    $envelope->all(); // is the listed stamp here?
}

@CharloMez
Copy link
Author

Just check it, yes this works fine for stamps my bad i didn't see

        $stamps = [];
        foreach ($this->transport('foo_transport')->queue() as $envelope) {
            $stamps = $envelope->all();
        }

        $this->assertArrayHasKey(AmqpStamp::class, $stamps);
        $this->assertSame('myexchange.bar_type', $stamps[AmqpStamp::class][0]->getRoutingKey());

But it could be more friendly i think.
It would be really great to provide some assert and accessors like for messages:

public function assertStampsContains | notContains(string $stampClass)

public function stamps(string $Class)

And for the serialized message I thought about a property serializedMessages in your TestTransport.php

and in your EnvelopeCollection.php a

public function getSerializedMessage(?string $class = null)

which get the serialized message which represent the $class message

What do you think about it ?

@kbond
Copy link
Member

kbond commented Apr 13, 2021

But it could be more friendly i think.

Absolutely! I'm thinking about how the API for this should work though. I'm trying to think where the assertStamps(Not)Contains() assertions should live.

public function getSerializedMessage(?string $class = null)

Still trying to understand this one - you want this method to return the serialized string for the message?

@kbond
Copy link
Member

kbond commented Apr 13, 2021

Currently, it's difficult to get the exact envelope/message you want if there are multiple messages sent with the same message class. Perhaps #3 could help but instead of returning Envelope it returns a wrapped TestEnvelope with the stamp assertions on it?

@CharloMez
Copy link
Author

public function getSerializedMessage(?string $class = null)

Still trying to understand this one - you want this method to return the serialized string for the message?

Yes thats what I want, but serialized by the right serializer, the one I configure in messenger

    messenger:
        serializer:
            default_serializer: messenger.transport.symfony_serializer
            symfony_serializer:
                format: json
                context: { }

It's possible to configure a specific serializer per transport like this:

external_messages:
    dsn: '%env(MESSENGER_TRANSPORT_DSN)%'
    serializer: App\Messenger\ExternalJsonMessageSerializer

Like explain here https://symfonycasts.com/screencast/messenger/transport-serializer

So it's not a complete functionnal test since we can't check the output of these serialisation

@CharloMez
Copy link
Author

Absolutely! I'm thinking about how the API for this should work though. I'm trying to think where the assertStamps(Not)Contains() assertions should live.

Maybe creating a StampCollection with these function and in TestTransport.php a function

    public function stamp(string $queueName): StampCollection

I feel we loose the queue name in your lib, but a function like this would be great.
Like that in test we could do

$this->transport('my_transport')->stamp('queue_name')->assertContains(AmqpStamp::class);

Would be the mirror of this conf:

        transports:
            my_transport:
                dsn: '%env(MESSENGER_DEFAULT_DSN)%'
                options:
                    ...
                    queues:
                        queue_name:
                            binding_keys:
                                ....

@kbond
Copy link
Member

kbond commented Apr 13, 2021

The "queue_name" is an option that not all transports have, isn't it? The AmqpStamp is specific to the AmpqTransport so it just wouldn't be there when using the test transport.

@kbond
Copy link
Member

kbond commented Apr 13, 2021

For the stamp assertions, I was thinking these would only be applicable for "user-defined" stamps (ie DelayStamp).

@CharloMez
Copy link
Author

The "queue_name" is an option that not all transports have, isn't it? The AmqpStamp is specific to the AmpqTransport so it just wouldn't be there when using the test transport.

hmm yeah right, amqb have specific conf, cf. doc:

    messenger:
        transports:
            async_priority_high:
                dsn: '%env(MESSENGER_TRANSPORT_DSN)%'
                options:
                    # queue_name is specific to the doctrine transport
                    queue_name: high

                    # for AMQP send to a separate exchange then queue
                    #exchange:
                    #    name: high
                    #queues:
                    #    messages_high: ~
                    # or redis try "group"
            async_priority_low:
                dsn: '%env(MESSENGER_TRANSPORT_DSN)%'
                options:
                    queue_name: low

Complicate to handle everything

@kbond
Copy link
Member

kbond commented Apr 13, 2021

Just check it, yes this works fine for stamps my bad i didn't see

I'm not seeing how this code snipped works? How is the AmpqStamp being added when using this lib's TestTransport?

@CharloMez
Copy link
Author

For the stamp assertions, I was thinking these would only be applicable for "user-defined" stamps (ie DelayStamp).

Mostly yes, but I think not only. Some library/bundles can apply specific stamps based on annotations in the application (I mainly think about api-platform, but I don't have example). In that case, you want to check tier library/bundles stamps to be sure you configure in the right way

@kbond
Copy link
Member

kbond commented Apr 13, 2021

I'm not seeing how this code snipped works?

Forget this - I see now you're adding it (it's user defined). I thought it was something the AmpqTransport added....

@kbond
Copy link
Member

kbond commented Apr 13, 2021

So to summarize my understanding here, I'm seeing two separate features:

  1. Easy access to and assertions on envelope stamps. Could an api like this work?
    $this->messenger('my_transport')->queue()->first(MyMessage::class)->assertHasStamp(AmpqStamp::class);
  2. Access the raw serialized string for a message. Could an api like this work?
    $serializedString = $this->messenger('my_transport')->queue()->first(MyMessage::class)->serialized();
    I'm still trying to find the value of this one. Are you going to be making assertions on the raw string?

@kbond
Copy link
Member

kbond commented Apr 13, 2021

Some library/bundles can apply specific stamps based on annotations in the application

What I meant is we can't make assertions on the stamps different "transports" add.

@CharloMez
Copy link
Author

So to summarize my understanding here, I'm seeing two separate features:

  1. Easy access to and assertions on envelope stamps. Could an api like this work?

    $this->messenger('my_transport')->queue()->first(MyMessage::class)->assertHasStamp(AmpqStamp::class);
    
  2. Access the raw serialized string for a message. Could an api like this work?

    $serializedString = $this->messenger('my_transport')->queue()->first(MyMessage::class)->serialized();
    

    I'm still trying to find the value of this one. Are you going to be making assertions on the raw string?

Yes that would be great, but with your other comment I understand that if we configure specific serializer per transport we can't have them threw your transport.

@kbond
Copy link
Member

kbond commented Apr 13, 2021

Can you not do?

# config/packages/test/messenger.yaml

framework:
    messenger:
        transports:
            async:
                dsn: test://
                serializer: App\Messenger\ExternalJsonMessageSerializer

The TestTransportFactory should create the TestTransport with your configured serializer. Admittedly, this advanced messenger stuff is a little out of my depth.

@CharloMez
Copy link
Author

I'm just thinking, but I feel that if we avoid all transport specific conf, it makes tests around it not totally functional.
I mean that, for me a functional test should only mock when it go external.

I'm trying to think about how to do.
Do you think it is possible to make a CompilerPass which get all configured transport, and proxify all of them with your transport threw decoration ?

You could override just the dsn to work in memory or something like that.
You understand what I mean ? sorry for my english :)

@kbond
Copy link
Member

kbond commented Apr 13, 2021

The purpose of this library as I see it, is a hybrid of the in-memory/sync transports. It's use is to assert your app is sending the correct message to your async buses with an option to process them (if applicable). I think transport specific stuff is out of this scope.

@kbond
Copy link
Member

kbond commented Apr 13, 2021

I can definitely see the use case for asserting stamps you define are added to your messages but as for wiring up your production transports (even the serializer imo) should be left for unit tests/E2E tests.

@kbond
Copy link
Member

kbond commented Apr 13, 2021

Question, is your ExternalJsonMessageSerializer equipped to "unserialize" the message or is this done in a completely separate app? Basically, is this causing a problem?

@CharloMez
Copy link
Author

In that case for me, with your lib we can perform integration tests but not really functional test.
But I understand your point of view.

@kbond
Copy link
Member

kbond commented Apr 13, 2021

In that case for me, with your lib we can perform integration tests but not really functional test.

That's a good way to put it.

@CharloMez
Copy link
Author

Question, is your ExternalJsonMessageSerializer equipped to "unserialize" the message or is this done in a completely separate app? Basically, is this causing a problem?

Yes the unserialize is in other app, but this line is not a problem it doesn't make error (probably because I use the symfony serializer system).

@CharloMez
Copy link
Author

Even with custom serializer it can't makes error, thanks to the interface

@CharloMez
Copy link
Author

CharloMez commented Apr 13, 2021

But it could hide a real error if the serializer in your class is not the same as the one configured in the real transport

@kbond
Copy link
Member

kbond commented Apr 13, 2021

So, I'm thinking the stamp assertions are useful and well within the scope of this library. Would that be helpful for you?

@CharloMez
Copy link
Author

I'm still trying to find the value of this one. Are you going to be making assertions on the raw string?

To answer to this question, yes that's what i planned to do. Like in behat when you check the output content like this:

And the JSON node "data.my_node" should be equal to "my data"

Actually I used to do that for rabbit message with behat (in previous project with swarrot), with my team we created Swarrot Context that allowed us to do things like

And I should have 1 message in "my_queue"
Then I process 1 message
And the JSON node "data.my_node" should be equal to "my data"

But now we I use messenger and phpunit for functional tests, and I try to do the same kind of assertion

@CharloMez
Copy link
Author

So, I'm thinking the stamp assertions are useful and well within the scope of this library. Would that be helpful for you?

Yes that would still be helpful, and the serializedMessage too because your lib use the same serializer as my project

@kbond
Copy link
Member

kbond commented Apr 13, 2021

Yes that would still be helpful

Ok, I can work on this feature - it's related to #3.

and the serializedMessage too because your lib use the same serializer as my project

This is because you're setting your custom serializer on the test transport like here? I'm still a little unclear on how to implement this feature - I'll tackle the stamp assertions first and then attempt this with some additional feedback from you.

@CharloMez
Copy link
Author

No I configured It globally on messenger like in my first message, so I think your transport got it too.

@kbond
Copy link
Member

kbond commented Apr 13, 2021

Got it

@kbond
Copy link
Member

kbond commented Apr 13, 2021

Forgive me, I'm still trying to understand the serialized string feature. If your serializer is configured globally, TestTransport::send() encodes/decodes, doesn't that tell you it's working correctly? In this context I don't understand the need to get the serialized string.

What I do understand is a transport that encodes a message for an external consumer with a proprietary format and the current app can't decode. I can see you'd want to test the encoded string to see it's in the correct format. Even here though, it makes more sense to me as a unit test on your custom serializer (or perhaps a real E2E test...)

@CharloMez
Copy link
Author

The serialization process need to be tested, there is important logic inside.
Yes it is mandatory to test all serialization process by unit tests, but it is not enough. The serialization process can be a full chain of different class (normalizers and decoders) that can call themselves the serializer again, and all of that can be driven by the data.
Of courses a large amount of E2E tests can cover it, but I can say same for unit tests "we don't need it because we have E2E". And having E2E tests with async micro services working with rabbit is really really not easy.
For me functional test is an important part of the overall test strategy.

kbond added a commit to kbond/messenger-test that referenced this issue Apr 14, 2021
@kbond
Copy link
Member

kbond commented Apr 14, 2021

@CharloMez, #6 adds the stamp assertions (here's the documentation for this). WDYT?

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

No branches or pull requests

2 participants