orchestral / testbench

Laravel Testing Helper for Packages Development
https://packages.tools/testbench
MIT License
2.11k stars 136 forks source link

Automatically calling setup/teardown on traits can lead to double-initialization/breakages #360

Closed mfn closed 1 year ago

mfn commented 1 year ago

Description:

In https://github.com/rebing/graphql-laravel the tests started to fail with the 8.1.0 release due to https://github.com/orchestral/testbench-core/commit/b824c5eeee2f52212bb54d1e373998273a6b5c5c

The project uses the convention to have a trait SqlAssertionTrait [1] with a method setupSqlAssertionTrait [2] which was initialized manually via a setupTraits [3] methods

[1] https://github.com/rebing/graphql-laravel/blob/1345744decc28c5a3833a2010531e1ee5722a3b7/tests/Support/Traits/SqlAssertionTrait.php#L18 [2] https://github.com/rebing/graphql-laravel/blob/1345744decc28c5a3833a2010531e1ee5722a3b7/tests/Support/Traits/SqlAssertionTrait.php#L27 [3] https://github.com/rebing/graphql-laravel/blob/8b156d5b98dee7e3fb86c62f42253a1d647de288/tests/TestCaseDatabase.php#L22-L31

See also https://github.com/rebing/graphql-laravel/issues/1017#issuecomment-1494815972 for a more in-depth analysis.

Since I've a 8.x branch in that project which still supports Laravel 6 (already on its way out), I still have to support orchestra/testbench:^4.

The fix on my side is easy, to stay backwards compatible I renamed the method in my project.

In my case, the breaking change was that the trait was initialized twice and thus malfunctioned (it registered a listener, which then was registered twice, breaking assertions in a couple of tests).

I don't need a fix for my project, renaming is an okay fix, just wanted to bring it to your attention. Feel free to close this issue!

crynobone commented 1 year ago

The method was introduced in Laravel Framework and we add it for compatibility. I'll add a note in the changelog later today.