-
-
Notifications
You must be signed in to change notification settings - Fork 914
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
Add aroundEach #820
Add aroundEach #820
Conversation
phase = .beforesExecuting | ||
for before in befores { | ||
before(exampleMetadata) | ||
wrappers.prepend { exampleMetadata, runExample in |
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.
Unused Closure Parameter Violation: Unused parameter "exampleMetadata" in a closure should be replaced with _. (unused_closure_parameter)
} | ||
|
||
internal func appendBefore(_ closure: @escaping BeforeExampleClosure) { | ||
befores.append { (_: ExampleMetadata) in closure() } | ||
wrappers.append { exampleMetadata, runExample in |
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.
Unused Closure Parameter Violation: Unused parameter "exampleMetadata" in a closure should be replaced with _. (unused_closure_parameter)
Sources/Quick/Example.swift
Outdated
let allWrappers = group!.wrappers + world.exampleHooks.wrappers | ||
let wrappedExample = allWrappers.reduce(runExample) { | ||
closure, wrapper in | ||
{ wrapper(exampleMetadata, closure) } |
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.
Opening Brace Spacing Violation: Opening braces should be preceded by a single space and on the same line as the declaration. (opening_brace)
Sources/Quick/Example.swift
Outdated
after(exampleMetadata) | ||
let allWrappers = group!.wrappers + world.exampleHooks.wrappers | ||
let wrappedExample = allWrappers.reduce(runExample) { | ||
closure, wrapper in |
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.
Closure Parameter Position Violation: Closure parameters should be on the same line as opening brace. (closure_parameter_position)
Sources/Quick/Example.swift
Outdated
@@ -79,21 +79,23 @@ final public class Example: _ExampleBase { | |||
world.currentExampleMetadata = nil | |||
} | |||
|
|||
world.exampleHooks.executeBefores(exampleMetadata) | |||
|
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.
Vertical Whitespace Violation: Limit vertical whitespace to a single empty line. Currently 2. (vertical_whitespace)
Generated by 🚫 Danger |
I’ve been using this successfully in Siesta’s specs for some time now. It works well. Project maintainers, questions for you:
If the answer to 1 is yes, I'll work on tidying up and fixing the assorted CI issues. |
Would one of the maintainers answer the author's question, please? I could really use this myself. In my case, I'd like to run some of my tests on a thread other than the main thread. I also really like and would use the approach to testing for leaks! |
@tmandry While the project maintainers attend to other things (I can sympathize!), you can use my fork of Quick with this patch (the I don't keep the fork up-to-date with new Quick commits all that aggressively, but it is an active dependency of Siesta, and I'll keep the fork working as long as I'm depending on it. |
I'm sorry for the late response! I'll answer as an active maintainer.
I'm open to this feature, that looks lovely 🤩
I think the existing test suite would be good enough.
I think so 😎 |
I hope this PR gets merged. It seems that it is verified at least one and half year since siesta started using it. |
I’ve been putting in some open source hours of late, so I’ll bet I could find time to add ObjC support, and also make sure it works in an ObjC-free SwiftPM environment. It would indeed be nice to get it working. |
Added an ObjC API and fixed the SwiftLint errors. I also decided that adding some regression tests for this would indeed be worthwhile. Writing those tests raised a sticky question: execution order. Suppose we have the following code: context("foo") {
beforeEach { before0() }
afterEach { after0() }
aroundEach { run in
around0Prefix()
run()
around0Suffix()
}
beforeEach { before1() }
afterEach { after1() }
aroundEach { run in
around1Prefix()
run()
around1Suffix()
}
beforeEach { before2() }
afterEach { after2() }
it("says hi") {
print("hi")
}
} What order should the various before / after / around callbacks run in? Quick’s documentation gives us fairly free reign on that question:
The most logical order for them to run in would be nesting in and then nesting out:
However, Quick throws a wrench into this by executing
We could switch to order 0 above (which is actually a bit simpler to implement), but it would change Quick’s behavior for any existing code that uses multiple FWIW, I did a quick survey of approaches in other frameworks. JUnit reorders its before/after blocks in a way that it deterministic but intentionally nonsensical, lest tests rely on order. RSpec does something very close to the nested approach, except that it forces all the Preserving Quick’s behavior and keeping order 1 seems like the safest option, but I thought I’d bring it up. |
Replicated non-check for proper context from beforeEach and afterEach
Added docs. Fixed a Linux + Swift 4.2 issue, and I think this is ready to merge, pending resolution of the ordering question above. |
About orderingI read RSpec's documentation and assume that
in that case
is what I expect.
1900cc6?w=1 is a quick sketch of this behavior. |
Note from my comment above that RSpec actually does the exact opposite of what the docs say!
|
Let's see rspec/rspec-core#2729, the following description is actually wrong:
and the |
If we're willing to revisit execution order, I'd strongly recommend preserving source order like this:
This means:
That last point is nice because it doesn’t reorder setup/cleanup operations if, for example, somebody needs to add cleanup to a It’s also an argument in favor of consistency, I think, that the order above is the natural behavior of this PR if you remove the additional work I did to preserve Quick’s current order. So if we are willing to break the existing ordering, this is the one I would advocate. |
I’ve updated the PR to match the latest from the main branch. I’d love to get this PR merged. Given any further thought to #989? You do make a really good point in that PR that the more logical ordering I proposed above would be consistent with |
P.S. I pushed the changes to switch to logical afterEach ordering in a separate commit off this PR, in case you want to see how it impacts the code and tests: pcantrell@c0de5a5 |
I’d really, really like to get this merged. Here is a proposal to resolve the ordering question:
That would allow this feature to be merged without resolving #989, and could also serve to pave an incremental adoption path for #989. What do you think? I’m happy to code this up if the approach meets with the maintainers’ approval. |
Hey there! 👋🏼 I'm a new maintainer for this project and I'm trying to get the next release out ASAP and also clean up old issues and old PRs. I'm closing all PRs older than 1-2 years, but that doesn't mean these changes are necessarily being rejected. If you are still interested in contributing these changes, please submit a new PR branching from the top of We appreciate your work here and acknowledge the time and effort you put in to contribute! You're awesome. 💯 However, we are all volunteers here with limited capacity working for free. Unfortunately, that means we must close out stale issues and PRs to move the project forward in a reasonable way. We apologize for the inconvenience and appreciate your understanding! 😎 ✌🏼 |
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)?
This is a proposed feature. It’s not yet ready to merge; I’m submitting the PR to see if Quick’s maintainers are amenable to the feature before completing the tests and documentation.
This PR adds an
aroundEach
decoration, which accepts the individual example as a callback:See the similar features in Rspec and JUnit.
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
andafterEach
. The original motivation for addingaroundEach
is Siesta checking for leaks in specs. 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:I’d like to pull out both the
autoreleasepool
block andcheckObjectsNoLongerRetained()
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 withbeforeEach
/afterEach
, but witharoundEach
it is: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
andafterEach
, it’s not always desirable. Grouping tightly coupled before and after behavior in a singlearoundEach
can help ensure proper nesting of operations.Questions
First, is this a feature Quick’s maintainers would consider merging?
Second, does this introduce any regressions? I rewrote
beforeEach
andafterEach
in terms ofafterEach
. This is necessary to ensure proper mutual ordering between the three; Quick’s existing strategy of keeping separate before and after lists won’t work when “around” needs to be ordered properly with respect to both kinds.Doing this actually cleaned up the code a bit, but I'm a little concerned about accidental breakage. The existing specs all pass, but I would want to double check that this doesn’t subtly change the ordering of spec decorators in some subtle circumstance. There’s also the possibility that it lowers the effective limit on how many before/after blocks there can be. I'd be surprised if anybody has enough of them to overflow the stack, but who knows? People do wild things!
Finally, does it need Obj-C support?
Merge checklist