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

Add aroundEach #1132

Merged
merged 15 commits into from
Apr 14, 2022
Merged

Add aroundEach #1132

merged 15 commits into from
Apr 14, 2022

Conversation

pcantrell
Copy link
Contributor

@pcantrell pcantrell commented 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:

aroundEach { example in
    ...
    example()
    ...
}

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 and afterEach. The original motivation for adding aroundEach 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:

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:

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

  • Tests
  • Documentation
  • Objective-C support
  • Ensure new implementation of before/after does not break existing public behavior
  • Appease SwiftLint
  • Is this a new feature (Requires minor version bump)?

@pcantrell
Copy link
Contributor Author

FYI, the one CI failure is Danger giving an error that appears to be unrelated to these changes:

Octokit::Forbidden: POST https://api.github.com/repos/Quick/Quick/issues/1132/comments: 403 - Resource not accessible by integration // See: https://docs.github.com/rest/reference/issues#create-an-issue-comment

@jessesquires jessesquires added this to the v5.0.0 milestone Apr 14, 2022
@jessesquires
Copy link
Member

Thanks so much @pcantrell ! 💯

Does this also address #989 or no?

Copy link
Member

@jessesquires jessesquires left a comment

Choose a reason for hiding this comment

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

Looks like just a few linter errors. Let's address those, then we can merge.

@jessesquires
Copy link
Member

FYI, the one CI failure is Danger giving an error that appears to be unrelated to these changes:

Yes. Danger cannot run on forks because GitHub can't be bothered to implement a CI system that works.

@pcantrell
Copy link
Contributor Author

Does this also address #989 or no?

No, #989 is a separate discussion about a breaking change to the execution order of afterEach callbacks. Quick’s current (undocumented) behavior is already a little bit illogical, and the addition of aroundEach only exacerbates the problem.

This PR preserves Quick’s existing afterEach behavior. #989 proposes a simpler execution order that is better in principle, but could conceivably break specs that rely on the relative execution order of afterEach callbacks.

Detailed discussion here: #820 (comment)

GitHub can't be bothered to implement a CI system that works.

Does any CI system work, really? 🧐

@jessesquires
Copy link
Member

No, #989 is a separate discussion about a breaking change to the execution order of afterEach callbacks.

got it 👍🏼

Does any CI system work, really?

lol

@jessesquires
Copy link
Member

@pcantrell there are conflicts from #1133 -- very sorry about that! 😊

@jessesquires
Copy link
Member

After this merges I'll cut the 5.0 release! 😄

@pcantrell
Copy link
Contributor Author

Should be gtg, assuming remaining tests pass on CI.

Sorry about the big merge commits. There have been enough intervening changes that it doesn’t rebase cleanly anymore!

@jessesquires
Copy link
Member

Sorry about the big merge commits. There have been enough intervening changes that it doesn’t rebase cleanly anymore!

No worries! I'll squash and merge this one.

@jessesquires jessesquires merged commit 13a4ac9 into Quick:main Apr 14, 2022
@pcantrell pcantrell deleted the around-each branch April 14, 2022 19:59
@jessesquires
Copy link
Member

@pcantrell invited you to the org as a maintainer! 😄

@pcantrell
Copy link
Contributor Author

Yay slash oh no!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants