[Pitch] Add a `.taskLocal` trait to Swift Testing

Evolution Pitches
1

Hi all,@stephencelis and I have a pitch for a new feature to add to Swift Testing: TaskLocal test trait.

All the details are in the link above, but in a nutshell, rather than defining a custom conformance to TestTrait, SuiteTrait and TestScoping (along with a static convenience function) when all you want to do is bind a @TaskLocal, you can now use the generic .taskLocal trait:

@Suite(.taskLocal(MyLocals.$isEnabled, true))
struct MySuite {
  @Test func basics() {
    // All tests run with MyLocals.isEnabled == true
    #expect(MyLocals.isEnabled  == true)
  }
}

Let us know if you have any feedback!

22 Likes
2

I did review the proposal pr, just dropping by to say this loooks like good idea and the spelling is nice as well :+1:

5 Likes
3

Actually, working with @kukushechkin and @FranzBusch on some new APIs for distributed tracing -- specifically to make tracers bindable for a test scope.

We're aiming to offer a way to opt-into a task-local binding for a tracer as well as metrics factories. Looking at this proposal, it seems we already have everything in place, right?

We'd have to mirror the pattern here and offer an extension to Trait in the -testkit modules of those libraries... Something like

extension Trait {
  public static func taskLocalTracer() -> Self // maybe with "Test" in the name...
  where Self == TaskLocalTracerTrait<Value>
}

Allowing for

import TracingTestKit // TBD, we currently just have InMemoryTracing

@Suite(.taskLocalTracer())
struct MySuite {
  @Test func myTest() { withSpan("...") } // each test gets individual trace
  @Test func myTest2() { withSpan("...") } // each test gets individual trace
}

where we'd provide the wrapping...

Anyway, so in the end it seems we don't even need this specific proposal to land, but I expect to follow the same pattern.

Looking really good, thanks for the pitch again.

4 Likes
5

Yeah, technically Swift Testing today is capable of doing everything you want thanks to TestScoping, but this proposal can make things a little nicer.

And ServiceContext is a good example of how this proposal can do that. Right now swift-service-context vends a task local that others will want to bind in tests. Currently there are two ways to do that:

  • Anyone that uses ServiceContext defines their own test trait for binding the task local in their tests.
  • Or, swift-service-context can create a whole new ServiceContextModuleTestSupport library that creates this trait a single time, and then anyone that wants to bind the task local in their tests will depend on this library.

Neither option is great. The first leads to a lot of duplication across different projects, and the second is extra code and documentation to maintain for the library author (also test support library docs can't link to the main library in DocC), and they have to educate their users about how to use the test support library. And every library that has a @TaskLocal will need to do this, and the docs for these traits are basically all identical.

So, if this proposal is accepted, anybody using swift-service-context can just do this:

@Suite(.taskLocal(ServiceContext.$current, …))
struct MySuite {
  …
}

And the same goes for any task locals DistributedTracing introduces.

2 Likes
6

To share a bit more about what Konrad alluded to here. I think we can do more than just wire up a task local for logging/metrics/tracing with a custom trait that does both wiring up the task local but also integrate with features like test attachments. I am imagining something like a logging test trait that sets up a specific log handler which will add attachments with the captured log for failing tests. Similar for the other observability signals.

2 Likes
7

I'm curious why naming this API .withValue(true, for: $isEnabled) was set aside. It mirrors TaskLocal.withValue(_:operation:), which is how task locals bind values.

Bike-shedding aside, this looks great to me. Huge +1!

8

I personally think .withValue(_:for:) is a little strange because it flips the order of arguments:

.withValue(true, for: $isEnabled)
// vs
$isEnabled.withValue(true)

And I like .taskLocal because it is more descriptive.

But I don't have a strong opinion. It is mentioned in the alternatives considered (along with some other alternatives), and more than happy to update if there is a strong consensus.

5 Likes
9

Yeah, I'd agree with Brandon here -- the "for:" makes it a bit awkward. (I think we even considered this API when we introduced task locals but didn't like it for that reason hah)

So it'd have to be .withValue($blippy, value) which isn't that descriptive... I'm more than happy to spell out the name "taskLocal" in this API as it's nice and clear that "task local voodoo is going non in these tests!" which is good for folks who maybe have never seen a task local before hmm... Since the with... { ... } isn't explicit in source I think this is useful enough.

2 Likes
10

In Factory there are two TaskLocals used (one for the container, one for the singleton scope), so the ContainerTrait is implemented with two withValues embedded.

1.) Do I understand correctly that with this change we could just replace the .container trait with the below?

@Suite(
  .taskLocal(Container.$shared, Container.shared),
  .taskLocal(Singleton.$shared, Singleton.shared.clone()),
)

2.) Factory also supports an optional transform closure where the user can apply some changes on the passed in value:

@Test(.container {
  $0.fooBarBaz.register { Baz() }
  $0.fooBarBazCached.register { Baz() }
  $0.fooBarBazSingleton.register { Baz() }
})

I can imagine this being done with immediately invoked closure expressions, but that may not be the most elegant looking solution for that. Would you have any suggestion for this use case? swift-dependencies also have a really similar helper closure if I remember correctly.

11

Hi @grabz, 1) yes this should work just fine with the @TaskLocals shipped in Factory. However, even with this test trait, there may still be a use for libraries defining their own domain-specific test traits. In the case of .container, it is binding two task locals, and so the library author may want to keep that behavior in a single trait.

And for 2), technically transformations of task locals is already supported since you can reference the current value directly:

@TaskLocal var count = 0
// ...
@Test(.taskLocal($count, count + 1))
func test() {}

And if you need a multi-line context you can use an immediately evaluated closure:

@TaskLocal var count = 0
// ...
@Test(.taskLocal($count, {
  count + 1
}()))
func test() {}

It's not as nice as a dedicated tool for transforming task locals, but TaskLocal.withValue doesn't support such an operation, and we would have to think of a new name if we want this operation. Something like .transformTaskLocal(_:_:)?

And while technically our swift-dependencies library uses a task local under the hood, we don't really expose that detail (it's underscored). And so even with this test trait we will continue shipping our domain-specific test trait. A better example of how we would use this trait is what we did in one of our other libraries (that diff uses another variation of naming mentioned in the alternatives, but ultimately we don't think it's the best name).

2 Likes
12

I really like this proposal. Agree it won’t replace all task local traits but it definitely makes setting simple task locals in tests super easy.

13

Strong +1 to the proposal. As it mentions, I called this out as a future direction in ST-0007 so that's perhaps unsurprising.

The only reason I didn't pursue it after ST-0007 was because I was under the impression that the problem described in Macro symbol visibility to other macros affected all macro usages. But it turns out that only affects macros declared in the same module as the reference, so that addresses what was my main concern. I'd still like to see progress on resolving that issue, but the scope of the problem seems small enough that this ought to proceed in the meantime.

I did have one suggestion, which I added as a comment on the implementation PR as well: I think the value the task local is bound to ought to be lazily-evaluated. A couple reasons for that:

  • If the test is skipped (e.g. it has a .disabled trait) the value expression will never be evaluated. That can be useful if the evaluation has observable side effects you only want to occur if the test actually runs, especially if that work is expensive.
  • The value obtained lazily during provideScope() can be released and discarded once the test finishes, which again can be important if the value occupies significant memory or otherwise needs to be deallocated once it's no longer needed.

The PR comment goes into a bit more detail and suggests using @autoclosure, but I wanted to surface this topic here for other reviewers' consideration too.

3 Likes
14

@mbrandonw thanks a lot for your response! I participated in the implementation of .container, hence the questions. .transformTaskLocal(_:_:) sounds interesting. Anyway +1 from me

1 Like
15

Yep we agree on this point and have updated the proposal and implementation accordingly.

2 Likes