-
-
Notifications
You must be signed in to change notification settings - Fork 355
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
Fix static hooks #357
Fix static hooks #357
Conversation
in other words, they can only run once per file
this happens due to the global `*All()` hooks (before & after) being defined as static. We should be a good citizen and we need to clean up our mess for the next person in the test instance constructor
CC @nunomaduro @owenvoke @lukeraymonddowning Requesting a review for this bug fix when you find some time 🙂 |
src/Concerns/Testable.php
Outdated
@@ -73,6 +73,7 @@ public function __construct(Closure $test, string $description, array $data) | |||
{ | |||
$this->__test = $test; | |||
$this->__description = $description; | |||
self::$beforeAll = self::$afterAll = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is ripe coming from the guy who promotes higher order expectations inside higher order tests but should we split this over two lines for clarity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably, yeah. It is kind of easy to miss, if not careful. Two lines it is!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated!
@jordanbrauer just for my own understanding, if we add another test to the AfterEachHook test down the line, we'll need to update the BeforeEachHook test offset, yes? |
@lukeraymonddowning , not quite, and thank goodness haha. The offset would only need to be updated if another test file was added to the For a better visual of why offset is
This kind of sucks though still, because if we ever have a feature to randomize the test order (or if we currently do), it would fail for false positive. However, that being said, shouldn't be a problem since there are snapshot tests that indirectly care about the order. Here I added some dummy tests that just expect true, but no changes to offset. |
Issue
The issue is essentially, exactly, as described by @fnzr in the related ticket (great job and good find!).
More specifically it is a result of the static nature of these hooks, which is why we do not see this behaviour on the
beforeEach
andafterEach
variants. This would have been a huge oversight on my part when initially implementing this feature in #282 , and could pose a potential performance issue for some folks, should they be doing any heavy work or network activity within.Solution
Funnily enough, it's a one-liner ... 😑 ... 🤣 . Nearly all changes are test related.
Source
static ?Closure $beforeAll
andstatic ?Closure $afterAll
at constructor-time for each test case, within theTestable
concern trait;Tests
Due to how these hooks work (e.g., can be defined in multiple files, variable amount of tests using them, etc.), it's a little tricky to ensure they are only run once per file. The solution to this is outlined below.
calls
property to theglobal $globalHook
object;BeforeAllTest.php
andAfterAllTest.php
header comments for full details