8000 Upgrading a large codebase to Quick 6 is really problematic and `toEventually` is broken in Obj-C · Issue #1189 · Quick/Quick · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Upgrading a large codebase to Quick 6 is really problematic and toEventually is broken in Obj-C #1189

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.

Sign up for GitHub

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

Jump to bottom
Closed
1 task done
ViktorSimko opened this issue Dec 12, 2022 · 9 comments
Closed
1 task done

Upgrading a large codebase to Quick 6 is really problematic and toEventually is broken in Obj-C #1189

ViktorSimko opened this issue Dec 12, 2022 · 9 comments

Comments

@ViktorSimko
Copy link
  • I have read CONTRIBUTING and have done my best to follow them.

What did you do?

Tried upgrading to Quick 6.

What did you expect to happen?

Quick 6 has an incremental way of adopting concurrency and toEventually isn't broken in Objective-C.

What actually happened instead?

Quick 6 added support for concurrency in a way that currently makes incremental adoption impossible.

The codebase that has to be upgraded to Quick 6 has a huge number specs across hundreds of modules and in order to upgrade all the problems introduced by the breaking changes have to be fixed in all those modules at once. Also generally the repo has a large amount of open PRs which would also need to be checked if they contain broken code and be fixed at the same time too.

Details:

Now tests run off the main thread (and off the MainActor) by default

  • a lot of tests use Main actor-isolated properties from the iOS SDK, these tests now won't build
  • a lot of tests use main thread only APIs which crash when called off the main thread

To temporarily fix these each spec could be marked as @MainActor but that wouldn't solve the cases where its or beforeEachs and its variants are used in a local function, since inside those the closures passed won't be @MainActor:

@MainActor
final class SomeSpec: QuickSpec {
    override func spec() {
        describe("...") {
            context("...") {
                beforeEach { // this is @MainActor
                    ...
                }

                it { // this too
                    ...
                }

                func someFunc(...) {
                    beforeEach { // this isn't @MainActor
                        ...
                    }

                    it("...") { // this also isn't
                        ...
                    }
                }

                someFunc()
            }
        }
    }
}

Another possible temporary solution is to mark all the async closures in all the specs @MainActor but that would result in an immense amount of changes.

Now the closures passed to it, beforeEach and the variants of the latter are all async and there isn't a non-async alternative

  • because of this (of course in addition to the above mentioned things) only the latest version of Nimble can be used which provides the async versions of waitUntil, toEventually and its variants and all the current usages can only be fixed all at once

  • those are also used extensively and it's likely that numerous open PRs would also include new usages

toEventually is broken in Objective-C because it runs in an async context

Adding this testcase to FunctionalTests_ItSpec_ObjC shows that toEventually is broken:

it(@"works with toEventually", ^{
    __block id obj = @1;
    dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(0.2 * NSEC_PER_SEC)), dispatch_get_main_queue(), ^{
        obj = nil;
    });
    expect(obj).toEventually(beNil());
});

Even though World has syncIt for Objective-C, the closures passed there are still executed in an async context, which brakes toEventually as described here: Quick/Nimble#1007.

What causes it to break is that the Objective-C blocks aren't async, so the non-async version of toEventually is used, which doesn't work in an async context, in which ultimately those blocks are executed, because of how the concurrency interoperability works between Objective-C and Swift.

The Objective-C QuickSpec calls the synthesized runWithCompletionHandler on Example.
runWithCompletionHandler's implementation creates a detached Task in which it runs Example.run and we end up in an async context. (https://github.com/apple/swift-evolution/blob/main/proposals/0297-concurrency-objc.md)

I think to fix this a completely sync way of executing Examples still needs to exist besides the async, using which a better API that enables the incremental adoption of concurrency in Swift specs too could be implemented.

Environment

  • Quick: 6.1.0
  • Nimble: 11.2.1
  • Xcode Version: 14.1 (14B47b)
  • Swift Version: 5.7
  • CocoaPods: 1.11.3
@younata
Copy link
Member
younata commented Dec 21, 2022 •
edited
Loading

My apologies for breaking toEventually in objective-c.

The lack of objective-c support for toEventually is definitely problematic. I'll find time to address this (it still might be a while), which will likely involve fixing the poor upgrade experience.

This will likely end up looking like 2 separate XCTestCase subclasses for people to choose from when creating a test: QuickSpec will behave like Quick 5 and earlier, and QuickAsyncSpec will behave like Quick 6 and be only available in Swift.

Once this is in (plus some time to marinate and be tested out), I'll release it as Quick 7.

@acecilia
Copy link

Amazing, thanks so much! I was thinking about a similar solution, providing two subclasses so consumers can decide which one to choose

@ViktorSimko
Copy link
Author

That's great to hear, thanks a lot!

@younata younata mentioned this issue Jan 10, 2023
Merged
@neo-mo-poroushani
Copy link
neo-mo-poroushani commented May 17, 2023 •
edited
Loading

@younata when the Version 7.0.0 is planned to get the release tag?

@younata
Copy link
Member
younata commented May 17, 2023

@younata when is the Version 7.0.0 is planned to get the release tag?

I don't 8000 have a concrete date planned. I'm currently recovering from COVID, so I'm focusing more on that than on Quick.

The main task left to do before releases 7.0.0 is updating our documentation.

Other than that, I'd really like to provide some way to run all tests in an AsyncSpec on the main actor. As-is, if you install Quick via Swift Package Manager, there's no way to do that. The one solution I've thought for this (create an AsyncMainSpec class that does this) is one I'd rather not do. I'd really like at least one other solution to consider. However, I'm getting closer and closer to punting this to a future release.

@neo-mo-poroushani
Copy link

I hope you recover fast, and think of great solutions while resting! ❤️‍🩹

@younata
Copy link
Member
younata commented May 20, 2023

I hope you recover fast, and think of great solutions while resting! ❤️‍🩹

Thanks! Quick 7 is now released!

I'm still recovering, but I also was incredibly bored.

There's still not a solution to have all closures in an AsyncSpec run on the main actor that is compatible with Swift Package Manager. I'm ok with that being in a future release. It's more important to fix the disaster that was Quick 6's force-async-tests.

I'm going to close this issue as I consider this to be fixed. Upgrading from Quick 5 to Quick 7, while not a drop-in, is much simpler than upgrading to Quick 6. For the most part, upgrading is a find & replace. Along with (hopefully) minor changes for when you have test helpers as methods or properties on your QuickSpec subclass.

@younata younata closed this as completed May 20, 2023
@acecilia
Copy link

Thanks for your work @younata , this is huge. What a legend! 🔥

@neo-mo-poroushani
Copy link

Awesome! Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@younata @acecilia @ViktorSimko @neo-mo-poroushani
0