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

POC: Result-builder based DSL #1149

Closed

Conversation

amomchilov
Copy link
Contributor

@amomchilov amomchilov commented Apr 26, 2022

This pr is a really rough POC that uses Swift's new Result Builders.

I've found that setup code often needs to use fresh objects for each examples, which requires using beforeEach. But to access any of the values initialized in beforeEach, they need to be declared in the parent scope, and they must be optional, which is a nuance:

override func spec() {
    describe("The issue with beforeEach") {
        let aValueINeedForMyTest = thisIsOnlyCalledOnce()

        let iWantAFreshValueForEveryTest: THenItHasToBeOptional!

        beforeEach {
            iWantAFreshValueForEveryTest = thisIsWhereYouNeedToReinitializeIt()
        }
        
        it("uses the values") {
            use(aValueINeedForMyTest)
            use(iWantAFreshValueForEveryTest)
        }

        it("should get a fresh value") {
            use(iWantAFreshValueForEveryTest)
        }
    }
}

This new Result Builder based DSL lets you express the same thing using:

override class func spec() -> Spec {
    Spec {
        describe("The issue with beforeEach") {
            let aValueINeedForMyTest = thisIsCalledTwiceNow() // This is called twice now
            let iWantAFreshValueForEveryTest = lookMaNoOptionals()
            
            it("uses the values") {
                use(aValueINeedForMyTest)
                use(iWantAFreshValueForEveryTest)
            }

            it("should get a fresh value") {
                use(iWantAFreshValueForEveryTest)
            }
        }
    }
}

There's a trade-off here: now all code inside the spec acts as if it were in a beforeEach, meaning it's not currently possible to nest something inside the Spec and only have it called once. Arguably, such things should be lifted to stored properties above the spec() func, anyway.

What do you guys think?

Related: #908

Comment on lines +152 to +162
let group: ExampleGroup

if let x = specClass as? ResultBuilderQuickSpec.Type {
group = x.spec().rootGroup
} else {
group = ExampleGroup(
description: "root example group",
flags: [:],
isInternalRootExampleGroup: true
)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The new DSL builds its structure up internally, without needing global state like the World.

Still, the World is where the examples are ultimately queried from, so I just patched this method for now.

@younata
Copy link
Member

younata commented Jul 1, 2022

This would be helpful with the async stuff I'm doing. It might even make sense to to make async support require this and leave the existing API as-is.

I like doing this. I would also like to reduce some boilerplate/extra indentation if possible. I don't really have good ideas there, unfortunately.

@amomchilov
Copy link
Contributor Author

amomchilov commented Jul 4, 2022

I would also like to reduce some boilerplate/extra indentation if possible.

Me too!

The outter-most Spec { ... } can certainly be removed, I just couldn't figure out how at the time hahaha. But if you compare to SwiftUI, you'll notice that they don't need any kind of View { ... } top level element. All the lines (potentially multiple) are wrapped up into a view automatically. Just need to recreate that.

My biggest concern here is compile-time and run-time performance. I don't know how expensive the result builder mechanism is to desugar. Would need some measurement.

Follow-up thought:

We can remove all the class-related boiler plate, and just have top-level code like:

Spec {
    describe("foo") {
        it("foos") { ... }
    }
}

And have the framework automatically generate a class in the background, not unlike the way it uses it bodies to create methods at runtime. This will make the syntax much lighter, but it'll probably break the Xcode integrations like gutter icons.

@younata
Copy link
Member

younata commented Jul 7, 2022

This will make the syntax much lighter, but it'll probably break the Xcode integrations like gutter icons.

This is my main concern with removing too much of the boilerplate. While it would be awesome to remove the boilerplate associated with having to make QuickSpec subclasses w/ a spec subclass. I also know that Xcode's test discovery is fairly basic, and test gems (as they call it) show up only when it detects XCTestCase subclasses, and methods in those subclasses that start with test. Which Xcode does mostly via string matching.

The outter-most Spec { ... } can certainly be removed, I just couldn't figure out how at the time hahaha.

Well, you're further ahead than me! I haven't done any research on what we can do with ResultBuilders. I think this approach of removing that outer-most Spec {} is a good path forward.

@younata
Copy link
Member

younata commented Jul 31, 2022

I spent some time this weekend working with this. I can see eventually deprecating the current way of building tests in favor of using ResultBuilder. Especially once we remove the integration with World. We'd have to keep the current way of building tests in order to support Objective-C, but we could certainly move swift folks towards using SpecBuilder.

An incredibly disappointing issue that I ran into is that statements with variable assignment aren't picked up by buildExpression(_:). For example, the following code only assigns the variable once:

private var underlyingValue = 0
private func generateValue() -> Int {
    underlyingValue += 1
    print("Called `generateValue()`, underlyingValue is now \(underlyingValue)")
    return underlyingValue
}

// ...

Spec {
    describe_builder("testing functions being called more than once") {
        let value = generateValue() // this is only called once, when `Spec` is called

        it_builder("calls generate value each time") {
            expect(value).to(equal(1))
        }

        it_builder("calls generate value each time please") {
            expect(value).to(equal(2))
        }
    }
}

Which is incredibly annoying that the variable assignment is not picked up by buildExpression. If we just call generatedValue() without assigning it to a variable (i.e. _ = generateValue()), then the statement is picked up and added to beforeEach as you'd expected.

I get why this isn't working how we want it to (value needs to be captured in order to be given to the examples). But that's still incredibly disappointed considering that the main draw of a ResultBuilder-based way of building tests is that we want these kinds of stored variables to be automatically re-assigned each time the test runs.

I think this is still worth iterating on, especially if we can solve the issue with variable assignment statements.

@amomchilov
Copy link
Contributor Author

Hey Rachel, I saw your mention on #1177

Sorry, I kind of disappeared off the face of the earth. I think I want to get a bit more involved in this kind of open source stuff, so I'll try to at least respond in a timely manner from now on 😅

is that statements with variable assignment aren't picked up by buildExpression(_:)

That does kind of make sense because variable assignments aren't an expression. They're a statement. Although I can't find anything like a buildStatement().

I would love to further iterate on a POC for this. However, I would caution against committing to it as a chosen direction, because I think there's a sizable risk that we end up discovering that result builders are too slow to be reasonable for large specs.

SwiftUI does use result builders, and Apple does have every incentive to optimize the feature, but I fear that test specs on average are much larger, more complex structures than most UIs (which are encouraged to be refactored and broken down into small, modular sub-views).

There's only one way to find out :)

@amomchilov
Copy link
Contributor Author

Closing this, because I think Apple's official swift-testing package is the blessed way forward.

Apple has explored using Result Builders to implement a testing DSL, but it looks like Macros are a better fit here.

While developing swift-testing we also experimented with using Result Builders for test definition, but encountered several significant challenges which I described under Alternatives Considered 7 in our vision document draft. Indeed, one of those challenges was the burden we knew this approach would place on the compiler's type-checker, especially since test code can often be quite lengthy compared to other APIs which use result builders. There were other notable difficulties beyond that, too.

From @stmontgomery https://forums.swift.org/t/a-new-approach-to-testing-in-swift/67425/26

@amomchilov amomchilov closed this Sep 24, 2023
@younata
Copy link
Member

younata commented Sep 26, 2023

Closing this, because I think Apple's official swift-testing package is the blessed way forward.

Agreed. I do think that a form of Quick that's built on top of swift-testing has a future, especially once Quick 6 becomes a thing with required concurrency checks. Additionally, the existing syntax has a place just for legacy and obj-c compatibility.

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

Successfully merging this pull request may close these issues.

None yet

2 participants