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

feat: document resetting all collected messages in setUp #77

Open
wants to merge 1 commit into
base: 1.x
Choose a base branch
from

Conversation

benito103e
Copy link
Contributor

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

TestTransport use static properties, that are not resetted between tests event when Symfony Kernel is rebooted.
@nikophil
Copy link
Member

nikophil commented Mar 22, 2024

Hi @benito103e

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

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 🤔

@benito103e
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@nikophil
Copy link
Member

ok you're right, that's a bug 😅

Maybe a #[BeforeClass] would do the job?

@nikophil
Copy link
Member

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
Copy link
Contributor Author

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

@nikophil
Copy link
Member

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
Copy link
Contributor Author

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

@kbond
Copy link
Member

kbond commented Mar 22, 2024

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
Copy link
Contributor Author

benito103e commented Mar 26, 2024

@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
Copy link
Member

kbond commented Mar 26, 2024

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
Copy link
Member

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?

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

Successfully merging this pull request may close these issues.

None yet

3 participants