zenstruck / messenger-test

Assertions and helpers for testing your symfony/messenger queues.
MIT License
218 stars 15 forks source link

feat: document resetting all collected messages in setUp #77

Open benito103e opened 4 months ago

benito103e commented 4 months ago

TestTransport use static properties, that are not resetted between tests event when Symfony Kernel is rebooted.

nikophil commented 4 months ago

Hi @benito103e

these properties are reset between tests (or at least, they should be :sweat_smile:)

This behavior is tested here: https://github.com/zenstruck/messenger-test/blob/1.x/tests/TransportsAreResetCorrectly/UsingTraitInteractsWithMessengerTest.php

Do your test suite actually shutdown the kernel between each other? (maybe KernelTestCase::tearDown() is overridden, and parent::tearDown() not called?) If it is the case, this could be a bug. Could you provide a reproducer? But that's strange because I'm using a lot this library and it's been a long time I didn't encountered it :thinking:

benito103e commented 4 months ago

Hi @nikophil,

You're right, these properties are reset after a test that implements InteractsWithMessenger but not after a test that doesn't. So if we have this scenario :

  1. Test1 add messages on transport but doesn't care about messenger assertions so doesn't implement InteractsWithMessenger
  2. messages remains in transports
  3. Test2 implements InteractsWithMessenger and should reset collected messages before tests.
benito103e commented 4 months ago

Maybe we should add a #[Before] attribute to InteractsWithMessenger::_resetMessengerTransports

nikophil commented 4 months ago

ok you're right, that's a bug :sweat_smile:

Maybe a #[BeforeClass] would do the job?

nikophil commented 4 months ago

I know we had complex problems around this in the past.

@kbond any clue why we don't call these methods before each test?

        TestTransport::resetAll();
        TestBus::resetAll();
benito103e commented 4 months ago

Ahah I can easily push the "before" modification but it's going to be a bit tricky for the associated automatic test...

nikophil commented 4 months ago

I think the problem was around blocking/intercepting.

maybe the easiest thing to do would be to initialize as well $queue, $dispatched, $acknowledged and $rejected in TestTransport::initialize()?

benito103e commented 4 months ago

@nikophil I agree, this would be equivalent to calling _resetMessengerTransports inside _initializeTestTransports

kbond commented 4 months ago

I think what would be ideal is having the test transport behave exactly like the InMemoryTransport (messages reset during kernel reboots) unless TestTransport::initialize() is called.

Maybe not exactly like in memory - we'd probably want to take into account the default transport config. (ie test://?intercept=false would process the messages immediately still)

benito103e commented 4 months ago

@kbond InMemoryTransport uses object properties. While TestTransport uses static class properties. So InMemoryTransport is automatically reset on each kernel reboot like all services, objects used by kernel. But class static properties require a specific call to be cleared.

kbond commented 4 months ago

Yep, my thought was to mimic the InMemoryTransport behaviour when using in a test that doesn't have InteractsWithMessenger trait. (reset the static properties on kernel reboot)

nikophil commented 3 months ago

Hey! I've thought a little bit more to this problem

What if we make TestTransport implement ResetInterface? this is what is done in InMemoryTransport

At first I was thinking it would call reset() between each messages, like it is done in "real life", but because we never add ResetServicesListener to the dispatcher (it is added "on the go" by messenger:consume command), the reset() method is not called between messages. So the we'd only reset the transports at the end of a test, in KernelTestCas::tearDown(), disregarding to the usage of InteractsWithMessenger trait.

WDYT?