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

Add a way to handle transport specific interfaces #83

Open
aegypius opened this issue May 15, 2024 · 7 comments · May be fixed by #84
Open

Add a way to handle transport specific interfaces #83

aegypius opened this issue May 15, 2024 · 7 comments · May be fixed by #84

Comments

@aegypius
Copy link
Contributor

One of my tests fails when using this package if i use a test:// configuration.

To be more precise, I am writing a prometheus collector that iterates against transports and expose the number of message to be processed. For that purpose, I use a similar code as :

$stats = new \ArrayObject([]);

// $transports is autowired with  #[AutowireIterator(tag: 'messenger.receiver', indexAttribute: 'name')]
foreach ($transports as $name => $transport) {
    if (!($transport instanceof \Symfony\Component\Messenger\Transport\Receiver\MessageCountAwareInterface)) {
        continue;
    }

    try {
        [$shortName] = \array_slice(\explode('.', $name), -1);
        $stats->append(new Transport(
            name: $shortName,
            count: $transport->getMessageCount(),
        ));
    } catch (\Symfony\Component\Messenger\Exception\TransportException) {
        continue;
    }
}

If I don't use test:// scheme, the native transport (which hold a MessageCountAwareInterface interface) is properly listed but when I start using this package to tests queues the transport is no longer listed and my integration test fails.

How would you solve this issue ? Do we need to add some sorte of option to the test transport to had these kind of interface ?

@nikophil
Copy link
Member

I'm wondering if we should just make TestTransport implement the interface MessageCountAwareInterface? WDYT?

@aegypius
Copy link
Contributor Author

I'm wondering if we should just make TestTransport implement the interface MessageCountAwareInterface? WDYT?

It would definitely solve this test but would fail if the test does the opposite

@nikophil
Copy link
Member

seems like our hands are tied...

@nikophil
Copy link
Member

seen directly with @aegypius: a solution would be to decorate with an anonymous class:

new class extends TestTransport implements MessageCountAwareInterface {
   use MessageCountAwareTrait;
}

but this would cause problem with interface composition (ListableReceiverInterface, QueueReceiverInterface, ...).

Let's use a pragmatic solution: since our TestTransport is listable and countable, let's make it implement these interfaces. And we'll think about another complex solution when/if someone complains about the fact it cannot be tested as a "not listable or countable"

any thoughts @kbond ?

@aegypius
Copy link
Contributor Author

And we'll think about another complex solution when/if someone complains about the fact it cannot be tested as a "not listable or countable"

I promise I will not 🤣 !

@kbond
Copy link
Member

kbond commented May 15, 2024

Let's use a pragmatic solution: since our TestTransport is listable and countable, let's make it implement these interfaces.

I agree!

It would definitely solve this test but would fail if the test does the opposite

@aegypius, you're fine with not being able to test the opposite (or using another solution for this test)?

@aegypius
Copy link
Contributor Author

@kbond yes I am fine with this solution

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

Successfully merging a pull request may close this issue.

3 participants