-
-
Notifications
You must be signed in to change notification settings - Fork 906
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
Reconsider afterEach execution order #989
Comments
Hey @ikesyo 👋🏼 😄 In the interest of time and getting v5 out sooner, I'm going to bump this to v6. |
FWIW, I maintain a branch with a working implementation of this idea here: pcantrell/Quick@around-each...pcantrell:logical-aftereach-order See especially this diff in the specs, which does a good job of explaining the effects of the change. That branch could become a PR if we decide we want to implement this. |
jessesquires
pushed a commit
that referenced
this issue
Apr 14, 2022
This is a reopening of #820, brought up to date with the latest main. Here is an updated summary; full historical discussion is in the original PR. I’d sure love to get this merged before it hits the 4-year mark! --- This PR adds an `aroundEach` decoration, which accepts the individual example as a callback: ```swift aroundEach { example in ... example() ... } ``` See the similar features in [Rspec](https://relishapp.com/rspec/rspec-core/v/3-0/docs/hooks/around-hooks) and [JUnit](https://junit.org/junit4/javadoc/4.12/org/junit/rules/RuleChain.html#around(org.junit.rules.TestRule)). ## Motivation There are a few test setup/cleanup scenarios the existing `beforeEach`/`afterEach` don’t handle, or don’t handle well. Some test decorations can’t split into a separate `beforeEach` and `afterEach`. The original motivation for adding `aroundEach` is [Siesta checking for leaks in specs](https://github.com/bustoutsolutions/siesta/blob/2b7bff709e0938a593de1f08415e905b4eece3ab/Tests/Functional/ResourceSpecBase.swift#L115-L145). To do this, it needs to wrap each spec in its own autorelease pool, then verify that objects are no longer retained _after_ the pool is drained: ```swift it("does this") { autoreleasepool { doThis() } checkObjectsNoLongerRetained() } it("does that") { autoreleasepool { doThat() } checkObjectsNoLongerRetained() } ``` I’d like to pull out both the `autoreleasepool` block _and_ `checkObjectsNoLongerRetained()` so they’re not repeated in each spec. (The two are tightly coupled, and it makes sense to factor them out together.) This is not possible with `beforeEach`/`afterEach`, but with `aroundEach` it is: ```swift aroundEach { example in autoreleasepool { example() } checkObjectsNoLongerRetained() } it("does this") { doThis() } it("does that") { doThat() } ``` Other methods that take a closure/block present a similar problem: XCTest’s `measure`, for example. Even in cases where it is possible to split the decoration into `beforeEach` and `afterEach`, it’s not always desirable. Grouping tightly coupled before and after behavior in a single `aroundEach` can help ensure proper nesting of operations. ## Implementation notes This PR works to preserve Quick’s somewhat illogical before/after hook execution order. (See #989.) This PR reimplements _before_ and _after_ hooks using the new feature. Internally, there is now only _around_. This appears to work; all existing specs still pass! ## Merge checklist - [x] Tests - [x] Documentation - [x] Objective-C support - [x] Ensure new implementation of before/after does not break existing public behavior - [x] Appease SwiftLint - [x] Is this a new feature (Requires minor version bump)?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
ref: #820 (comment)
On the other hand, XCTestCase's
addTeardownBlock
says:I'm considering that making the behavior to match with XCTest would be desirable/easy to understand.
The text was updated successfully, but these errors were encountered: