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

Reset TestTransport on ensureKernelShutdown #90

Merged

Conversation

HypeMC
Copy link
Contributor

@HypeMC HypeMC commented Sep 12, 2024

An alternative approach to #82, requires symfony/symfony#58240

Copy link
Member

@nikophil nikophil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @HypeMC

thanks for this PR :)

requires symfony/symfony#58240

why don't we implement ResetInterface? So we won't need Symfony 7.2 to benefit from this feature.

Would you mind adding a test which confirms that everything is working? We should test this in both a test which uses InteractsWithMessenger trait, and a test which doesn't, I guess

src/InteractsWithMessenger.php Show resolved Hide resolved
@HypeMC
Copy link
Contributor Author

HypeMC commented Sep 13, 2024

requires symfony/symfony#58240

why don't we implement ResetInterface? So we won't need Symfony 7.2 to benefit from this feature.

@nikophil Hi, the reason is your comment about introducing a BC break. With this approach, the existing method's behavior remains unchanged.

Also, symfony/symfony#58240 is a bugfix, so it won't require 7.2.

Would you mind adding a test which confirms that everything is working? We should test this in both a test which uses InteractsWithMessenger trait, and a test which doesn't, I guess

Sure, no problem, but I don't think I can actually cover the original scenario as it depends on the execution order and a specific test being run first:

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.

@nikophil
Copy link
Member

nikophil commented Sep 13, 2024

the reason is #82 (comment). With this approach, the existing method's behavior remains unchanged.

oups yes, you're right 🤭 let's do your way then!

Also, symfony/symfony#58240 is a bugfix, so it won't require 7.2.

perfect

depends on the execution order and a specific test being run first

yeah... we're doing strange things to test behavior between tests in this library 😅 ... basically, not using --order-by nor any executionOrder in the xml will play the tests in the natural order (alphabetically). I know this is not ideal, but it is the only way to test such behaviors. No problem if you don't want to bother with this, I might do it at some point. Maybe I'll upgrade first to PHPUnit 10, in order to leverage #[DependsExternal] attribute...

@HypeMC
Copy link
Contributor Author

HypeMC commented Sep 13, 2024

depends on the execution order and a specific test being run first

yeah... we're doing strange things to test behavior between tests in this library 😅 ... basically, not using --order-by nor any executionOrder in the xml will play the tests in the natural order (alphabetically). I know this is not ideal, but it is the only way to test such behaviors. No problem if you don't want to bother with this, I might do it at some point. Maybe I'll upgrade first to PHPUnit 10, in order to leverage #[DependsExternal] attribute...

@nikophil Actually, I just noticed the testsuite name="zenstruck/messenger-test transports are reset correctly" which is exactly what I need, so I think the test is possible.

@HypeMC HypeMC force-pushed the reset-testtransport-on-ensurekernelshutdown branch from 1e85bc1 to ab2123d Compare September 13, 2024 16:19
@HypeMC
Copy link
Contributor Author

HypeMC commented Sep 13, 2024

@nikophil Funny thing, the test was kind of already there, it just didn't work when all tests were run cause it requires TestTransport::$enableMessagesCollection to be set to its initial value of true for the bug to manifest. The tests are now failing cause they require symfony/symfony#58240. See ab2123d

@nikophil
Copy link
Member

Don't hesitate to ping me once the fix is released in SF, and I'll merge and release this PR

It seems there is a bunch of issues and PR that can be closed as well. Thanks!

@HypeMC
Copy link
Contributor Author

HypeMC commented Sep 21, 2024

@nikophil The fix has been released by Symfony, however, I don't think it's possible to get the tests for 6.3 and 7.0 to pass since those versions are no longer supported. I'm not sure what to do about this, the fix is valid for all supported versions.

@nikophil
Copy link
Member

hey @HypeMC

I just discovered that some tests are failing, maybe because of a new version of Symfony. I'll fix them asap, before merging your PR

@nikophil
Copy link
Member

@HypeMC would you mind to rebase your PR?

@HypeMC HypeMC force-pushed the reset-testtransport-on-ensurekernelshutdown branch from 818f91d to 688b12c Compare September 26, 2024 17:08
@HypeMC
Copy link
Contributor Author

HypeMC commented Sep 26, 2024

@HypeMC would you mind to rebase your PR?

@nikophil Done

@nikophil nikophil merged commit d9d711b into zenstruck:1.x Sep 26, 2024
14 checks passed
@HypeMC HypeMC deleted the reset-testtransport-on-ensurekernelshutdown branch September 26, 2024 17:29
@nikophil
Copy link
Member

released in 1.10.0

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.

2 participants