[PITCH] Enable Test Discovery by Default

Is the next step a PR into SwiftPM or a full blown SE proposal or something in between?

I don't think we need an evolution proposal for this. It's not changing behavior for packages that already have well-defined tests (LinuxMain.swift and runtime discovery still get respected when possible), so this seems like a workflow improvement & not a fundamental piece of configuration.

1 Like

I'm going to try and kick this off this week - one question that I didn't ask originally is should this change be global and immediate or should it be predicated by the tools version?

So if the tools version is 5.3 (or whichever version this would ship in), look for XCTMain.swift or use test discovery by default. If the tools version is still 5.2 then keep with LinuxMain.swift and no automatic test discovery. Or should it just use automatic test discovery etc no matter the tools version

3 Likes

Great, thank you!

I don't think this needs to be gated by the tools version, since we said this:

So existing packages would opt-in by deleting that file.

One idea here is that when LinuxMain gets renamed, the new Main file is used on all OSs by default if present. If anyone wants to have the new Main file and get the default Xcodey behavior on Apple platforms, they can call XCTestSuite.default.run(), right?

1 Like

By default on macOS swift test builds an .xctest bundle and runs that through xctest, so is your idea that SPM's behavior would change to build a standard executable instead of an MH_BUNDLE if a Main file is present?

This would be quite nice actually, because it would remove an awkward limitation that only exists on macOS right now: since SPM is tightly coupled to XCTest's notion of a test bundle, it doesn't easily permit the development of other testing frameworks or methodologies. It's great to streamline the XCTest use case since it's the common one, but from a general build system/package manager point of view, it would be nice if swift test could also work in a mode that was just "build the target, run it, and the process exit code defines success/failure."

Admittedly, that sounds a lot like swift run, but that's already the behavior on Linux AFACIT (sans test discovery), and keeping the sources in the Tests vs. Sources tree would still be an important distinction.

1 Like

@NeoNacho given the proposal was to also rename the file, should I check for both LinuxMain.swift and XCTMain.swift?

If there's not a strong reason to have the filename be closely associated with XCTest, I think it would be better to leave that out, as it would leave the door open to having swift test support non-XCTest-based testing patterns in the future without having to change it again or leaving an odd historical artifact in place.

Is there anything preventing us from just calling this file main.swift in the Tests source root? SPM could still ignore it on macOS, just as LinuxMain.swift already is and the proposed new-name would be, and then the platform differences could be resolved in follow-up work.

If main.swift isn't possible for some reason, I'd lean toward something less XCTest-specific like TestMain.swift.

4 Likes

Yah, I think it makes sense to check for both old and new name.

I hope the proposal could also address this issue: "Problem with --enable-test-discovery and the XCTestObservationCenter". I have a similar problem when running tests for SwiftWasm apps in browsers, we need to communicate test results back to the user, but the default test reporter does not provide parseable output. Installing a custom JSON test reporter as an observer helps, but unfortunately it's not possible with --enable-test-discovery.

I've recently got a new XCTMain override merged to XCTest, which allows passing custom observers to replace the default test reporter, maybe something could be built on top of that?

2 Likes

What's the status of this?

I recently needed to disable a couple of tests on Windows, and found it to be quite a hassle due to needing to update the generated manifest. I don't even want to use those generated manifests - discovery works perfectly well for me, but IIRC I can't even do a normal swift build on Linux unless I have a LinuxMain.swift in my test target.

I wish I could just get rid of the manifests and forget they ever existed. It's a shame that Swift makes this so hard.

Test discovery is on by default from 5.4 onwards if you delete LinuxMain.swift: https://github.com/apple/swift-package-manager/pull/3053

2 Likes

You can do swift build --enable-test-discovery on Linux to get your app building without a LinuxMain.swift

@Karl is right. The pre‐5.4 implementation of --enable-test-discovery had a bug in that it neglected to disable an early sanity check that verified some LinuxMain.swift file existed in the file system. So at the moment you still need to have such a file, even though most of us just leave it empty or with a reminder to ourselves like #error("You fool, you forgot the --enable-test-discovery flag again."). All the other test manifest files can be safely deleted.

Can you expand on this bug? Because I haven't seen it and since 5.4 just sets the flag by default, if there's a bug then it will still be present!

AFAIK swift build --enable-test-discovery will build the library/executable without needing a LinuxMain.swift or any test manifests. swift test --enable-test-discovery will run the tests using test discovery to find them

On closer inspection, your suggestion does appear to successfully dodge the bug. But --enable-test-discovery was never supposed to be necessary with swift build. swift build does not even build the tests at all without the extra --build-tests flag.

I am reasonably certain that the overhaul in 5.4 does not suffer from any of this.

There is an issue now with Swift packages that have a LinuxMain that says to manually invoke this flag, ie for the pre-5.4 behavior, so I still have to invoke this flag with the 5.4 snapshot builds or it errors out. Is there some modification we can make to that file with the error message so that passing this flag won't be needed for 5.4, but it still errors for 5.3 and before?

@Finagolfin the implementation allows for a LinuxMain.swift to be specified so that existing test manifests still works and workaround some of the bugs in test discovery. If you have a LinuxMain.swift then it will always default to that, so if that has a fatal error in it, just remove it

Take a look at the LinuxMain I linked: it became standard practice in several Swift package repos to error if this flag wasn't passed, which worked well up to 5.3. The problem is that that's no longer needed with 5.4, but if you are still dealing with users on 5.3, you may want to keep that error for them. I'm asking if there's some way we can do that.

2 Likes

I don't think there is a way to preserve any backwards compatible behaviour like that. @NeoNacho or @tomerd might be able to say otherwise

correct, if LinuxMain.swift exists, SwiftPM will continue to invoke it. this is for backwards compatibility for libraries that have been relying on it and as an escape hatch for cases where automatic test discovery does not work well.

libraries that follow the pattern @Finagolfin describes should delete LinuxMain.swfit if they can, or continue to lean on --enable-test-discovery until such time.