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

QuickConfiguration could inherit @MainActor attribute from spec class. #1182

Closed
1 task done
matheusalano opened this issue Nov 8, 2022 · 4 comments
Closed
1 task done

Comments

@matheusalano
Copy link

matheusalano commented Nov 8, 2022

  • I have read CONTRIBUTING and have done my best to follow them.

What did you do?

When you have a QuickSpec with the @MainActor and a QuickConfiguration with (for example) an afterEach, the closure of this afterEach won't use the main actor.

The example below is using NSLog to show which process Id each closure is being run from. This is to easily tell from the console whether they’re both running on the same thread or not.

@MainActor
final class Actor_Spec: QuickSpec {
    override func spec() {
        describe("mainActor") {
            it("should be true") {
                NSLog("Log from spec class")
                expect(Thread.isMainThread) == true
            }
        }
    }
}

final class ThreadTestConfiguration: QuickConfiguration {
    override class func configure(_ configuration: QCKConfiguration) {
        configuration.afterEach {
            NSLog("Log from configuration class")
        }
    }
}

What did you expect to happen?

The process ID from the spec and configuration classes should match.

What actually happened instead?

The process ID from the spec and configuration classes don't match.
Adding configuration.afterEach { @MainActor in makes the process IDs match.

Environment

List the software versions you're using:

  • Quick: v6.0.0
  • Nimble: v11.1.0
  • Xcode Version: 14.1
  • Swift Version: 5.7.1

Please also mention which package manager you used and its version. Delete the
other package managers in this list:

  • Carthage: 0.38.0
@younata
Copy link
Member

younata commented Nov 11, 2022

Huh. I 100% missed that line in SE-0316 prior to this.

I don't think we should do this. I think that tagging the configuration closure with @MainActor in is sufficient enough. We don't expect there to be more than a few QuickConfiguration subclasses, so adding the @MainActor in syntax isn't that much of a lift. Additionally, not all the contents of every configuration.X will need to run on the main actor, and I feel that it's bad practice to unnecessarily force everything to run on the main actor.

I'm going to leave this open for now. I'd like to see more evidence/arguments for why you think this is desirable. But I don't think this is a convincing-enough reason to do this.

@matheusalano
Copy link
Author

Yeah, I honestly don't know if this suggestion would be a bad practice or if it's even possible.
but the "context" behind it is that we have a couple of QuickConfiguration subclasses and their afterEach may involve UI elements.

Let's say I have a spec that is testing a UIView. Because this test is interacting with a UI element, I have to add the @MainActor to the QuickSpec.

The problem is that the QuickConfiguration subclass won't run on the main actor, and Xcode will complain that a UI element that was in the main thread is now being run on another thread.

We could add the @MainActor to the QuickConfiguration subclass, but we didn't want to force that configuration to always use the main actor when it really depends on the test "using" it.

I came up with a """workaround""", but I'm not proud of it.

final class MyConfiguration: QuickConfiguration {
    override class func configure(_ configuration: QCKConfiguration) {
        let isMainThread = Thread.isMainThread
        configuration.afterEach { _ in
            if isMainThread {
                await MainActor.run { /* do something */ }
            } else {
                /* do something */
            }
        }
    }
}

@airlacodes
Copy link

Any updates on this? 👀

@younata
Copy link
Member

younata commented Jun 30, 2023

This isn't an issue as of Quick 7. QuickConfiguration only lets you specify synchronous setup/teardown methods, which will all run on the main thread.

@younata younata closed this as completed Jun 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants