Swift 5.10 Concurrency and XCTest

How baked are some of Xcode 15.3's concurrency warnings? One notable issue is that XCTestCase can no longer be annotated @MainActor:

@MainActor
class MyTests: XCTestCase {  ⚠️

:warning: Main actor-isolated class 'MyTests' has different actor isolation from nonisolated superclass 'XCTestCase'; this is an error in Swift 6

While it seems like the migration path is to move the actor annotation to the individual test case, that breaks quickly if you need to use a method like fulfillment(of:):

@MainActor
func testSomething() async {
  let something = expectation(description: "something")
  await fulfillment(of: [something])  ⚠️
}

:warning: Passing argument of non-sendable type 'XCTestCase' outside of main actor-isolated context may introduce data races

And some XCTest APIs require @MainActor, like UI tests, which touch XCUIApplication, so these are just incompatible with fulfillment(of:) at the moment if you want to avoid introducing warnings, though we can workaround the problem with a synchronously call to the old API, which seems like a hack:

_ = { wait(for: [something]) }()

So I guess my question is, what is the correct migration path here? Just wait for the XCTest framework to address these issues?

34 Likes

We also experience these warnings and would like to know how to address them properly.

4 Likes

Xcode 15.3 shipped without doing anything about this, so I guess we just have to turn Sendability checking off for test suites? I haven't been able to figure out any way to use fulfillment(of:) that doesn't produce warnings and it's so many warnings that it makes any other sendability warnings impossible to spot anyway.

1 Like

A pattern that has sometimes worked for me is to remove the MainActor isolation entirely.

func mainActorTest() async {
	let exp = expectation(description: "happened")

	Task {
		// access MainActor-isolated stuff here, including using the expectation
	}

	await fulfillment(of: [exp], timeout: 1.0)
}

This has limited utility, because if you cannot make use of local state across calls to fulfillment. You may be able to use test instance variables to do it. But, I've found that incredibly annoying and not particularly scalable.

I just fixed a whole bunch of those in my project.
In this case I simply moved the @MainActor annotation from the entire XCTestCase subclass definition to each individual method (that required it). Apparently that also works when you override setUpWithError(), etc. so while it was a little bothersome to change so many test functions it now runs without issues (note: I am not yet using -strict-concurrency=complete, but will look at this in the near future, I hope).

What's mostly affected so far seem to be the tests for UIViewController subclasses, as we're still using a lot of UIKit, but to me it looks like this is the way to properly go: Leave the test class un-annotated, and annotate the test methods instead.

Btw, I don't entirely get what the problem with

is? That in itself is not really an issue with @MainActor, but rather just a consequence of the test method not being async. Just using wait(for: [something]) is sufficient in this case, whether the (synchronous) test method runs on the main actor or not. And while wait is "old" it's not deprecated and I doubt it's going anywhere anytime soon. It's simply the synchronous counterpart to fulfillment(of:).

1 Like

Sorry, my example was missing the async modifier. Edited now.

Hm, okay, I was considering you might have left that out, but then that code does not produce any error, I believe? At least when I tried it out it does not?
Admittedly I could have also noticed that the error message you gave does not at all match what you'd get when simply not making the function async... that's my bad. That would be the much more obvious 'async' call in a function that does not support concurrency...

Your example obviously leaves out the part where the expectation is fulfilled, so I quickly spun up this:

    @MainActor
    func test_bla() async {
        let something = expectation(description: "bla")
        Task {
            try? await Task.sleep(for: .seconds(1.0))
            something.fulfill()
        }
        await fulfillment(of: [something])
    }

That compiles and runs without any problems in my freshly updated Xcode, even with -strict-concurrency=complete. Perhaps that was a last little bug that had been fixed in the final release?

The only other reason I could see is in how you use whatever instance of the type to test you have to fulfill the expectation... I find myself often using an [unowned self] capture list in some closures I use to do so, as I tend to have an underTest property in my test's instance that I use to, well, test...

If the warning in this case is still there, can you maybe give a more concrete example that I could try to reproduce it with?
As said I was able to simply make all my functions @MainActor where I had previously used test cases annotated that way, but none of those were actually async...

Thanks for starting a discussion about this. This warning

Is pointing out a real potential for data races and it's not in the camp of "some more compiler analysis could prove this code safe". I do think there are ways that the compiler could lift the restriction at the cost of some different restrictions, e.g. we could make is so that you can add isolation to a subclass of a non-Sendable, non-isolated superclass, but the subclass cannot be made Sendable and overrides still must be nonisolated. Other folks have brought up the idea of somehow making isolated subclasses retroactively make everything from the superclass isolated to the main actor, but that would come with other restrictions too like no upcasting, and no access to nonisolated async methods which wouldn't solve the problem here with the fulfillment method.

The reason why this is unsafe is because XCTestCase is not Sendable since it is a reference type with mutable state. For non-Sendable types, you get a guarantee that only one isolation domain has access to the value at any given time. XCTestCase.fulfillment is a nonisolated async method, which means it's evaluated off of the main actor, so calling it from the main actor will pass self over an isolation boundary. While your test case is suspended waiting for the result of fulfillment, other work can happen on the main actor that accesses self, e.g. if you've done any concurrent work in your test method. My understanding is that XCTest will create fresh instances of your test case type for parallel tests, so I think the only time you can get concurrent access to self is if you've done any concurrent work yourself in your tests.

If your test isn't actually doing anything concurrently, you can use nonisolated(unsafe) to suppress the warning:

@MainActor
func testSomething() async {
  let something = expectation(description: "something")

  // This is okay because this test isn't doing any concurrent work, and any tests
  // that run in parallel will have a completely separate instance of the test case type.
  nonisolated(unsafe) let `self` = self
  await self.fulfillment(of: [something])
}
13 Likes

I'm not sure what you mean, but my example includes the await fulfillment(of: […]).

I'm in the latest Xcode 15.3 with concurrency checking set to complete and I get this on the await fulfillment(of:) line:

:warning: Passing argument of non-sendable type 'XCTestCase' outside of main actor-isolated context may introduce data races

Referring to the implicit self of self.fulfillment.

That's a good trick, thanks! And thanks for the detailed explanation of the things :smile:

4 Likes

Do you know if we should we be filing Feedbacks for APIs which aren't playing nicely with Swift Concurrency at this stage?

For example, in SwiftUI:

import StoreKit

/* @MainActor */ // occurs with or without annotation 
struct MyView: View {
  // Cannot form key path to main actor-isolated property 'requestReview'; this is an error in Swift 6
  @Environment(\.requestReview) private var requestReview

  var body: some View {
    Text("Cannot form key path to main actor-isolated property")
  }
}

Or os:

/* @preconcurrency doesn't help */ import os

enum Log {
  // Static property 'shared' is not concurrency-safe because it is not either conforming to 'Sendable' or isolated to a global actor; this is an error in Swift 6
  static let shared = Logger(subsystem: "logging", category: "warnings")
}

Or Foundation:

import Foundation

public final class UnitWarning: Dimension {
  
  public override class func baseUnit() -> UnitWarning { .warnings }
  
  // Static property 'warnings' is not concurrency-safe because it is not either conforming to 'Sendable' or isolated to a global actor; this is an error in Swift 6
  public static let warnings = UnitWarning(
    symbol: "WARN",
    converter: UnitConverterLinear(coefficient: 1)
  )
}

(This can be 'fixed' by turning it into a computed property but that would be pretty terrible for performance.)

1 Like

Not all of your examples are SDK issues, but yes, you should absolutely file feedback reports if you believe an API is mis-annotated.

I think using a main-actor isolated key-path in a property wrapper initializer should be fine as long as your enclosing view is @MainActor, per SE-0411: Isolated default values. That makes this just a compiler bug.

It's a compiler bug that the @preconcurrency import doesn't suppress the warning.

Dimension is has an explicit, unavailable Sendable conformance, which means it's definitely not a thread-safe type, so this is not an issue for Foundation to fix. If you want to use a non-Sendable type as a global/static variable in your code, you will need to isolated it to a global actor for safety to be proven statically, or wrap it in some kind of Sendable container type that protects access to it. If you don't want static data-race safety, you can mark it as nonisolated(unsafe) and take care at each use site to make sure you are synchronizing access to it, e.g. by using a lock or a dispatch queue.

2 Likes

OK, thank you @hborla ! I'll file those compiler bugs tomorrow,

The example of Dimension above is pretty much exactly how the documentation suggests a Dimension subclass should be created. The whole point is the conversion between the various statically declared units. Unless I'm missing something, It doesn't seem like its design fits with the new concurrency model. (Without getting Sendable conformance.)

Thanks for raising this issue! We're tracking it internally now.

3 Likes

Regarding this one, @eskimo answered this on the developer forums: Is OSLog Logger Sendable? | Apple Developer Forums

tldr - Logger is thread safe and is just not yet annotated as Sendable.

2 Likes

Thanks, but I think what @hborla is saying is that it's a compiler bug that @preconcurrency doesn't suppress the warning in the meantime.

1 Like

Yep, I'm not disputing that :slight_smile:

1 Like

Another workaround to the problem with await fulfillment(of: [exp], timeout: 1.0) appears to be:

await XCTWaiter(delegate: self).fulfillment(of: [exp], timeout: 1)

XCTWaiter conforms to Sendable.

1 Like