[PITCH] Enable Test Discovery by Default

jrose · 2020-07-15T02:24:37.214Z

rballard:

The difference between macOS and Linux behavior is unfortunate and we should probably revisit it, but that could be done separately & after from this proposal.

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?

allevato · 2020-07-15T03:52:46.572Z

jrose:

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?

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.

0xTim · 2020-07-15T09:38:32.557Z

NeoNacho:

Great, thank you!

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

0xTim:

Test discovery will not be used if SwiftPM detects a LinuxMain.swift and the tests are run with swift test on Linux

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

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

allevato · 2020-07-15T14:36:18.840Z

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.

NeoNacho · 2020-07-15T16:52:36.807Z

0xTim:

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

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

Max_Desiatov · 2020-07-28T17:57:42.121Z

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?

Karl · 2021-03-27T13:23:28.094Z

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.

owenv · 2021-03-27T14:38:40.160Z

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

0xTim · 2021-03-27T16:50:29.032Z

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

SDGGiesbrecht · 2021-03-27T23:10:04.334Z

@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.

0xTim · 2021-03-28T17:31:33.227Z

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

SDGGiesbrecht · 2021-03-28T20:16:15.634Z

0xTim:

AFAIK swift build --enable-test-discovery will build the library/executable without needing a LinuxMain.swift or any test manifests.

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.

Finagolfin · 2021-03-29T09:16:26.204Z

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?

0xTim · 2021-03-29T09:29:51.348Z

@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

Finagolfin · 2021-03-29T09:35:55.409Z

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.

0xTim · 2021-03-29T09:44:28.772Z

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

tomerd · 2021-03-29T22:28:18.971Z

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.

Karl · 2021-03-30T10:43:02.411Z

I wonder if we could add an annotation to the top of LinuxMain.swift - something like:

// swift-package-is-discovery-warning

And when SPM sees that, it’ll know that LinuxMain is just a dummy file for back-compat purposes, and that it should still use test discovery rather than compiling LinuxMain.

lukasa · 2021-04-07T16:55:05.645Z

Can we at least silence the warning? grpc-swift gets this nasty combination of outcomes:

root@368b93eb7b6d:/code# swift test --enable-test-discovery
warning: '--enable-test-discovery' option is deprecated; tests are automatically discovered on all platforms

root@368b93eb7b6d:/code# swift test
[4/4] Build complete!
grpc_swiftPackageTests/LinuxMain.swift:16: Fatal error: Use --enable-test-discovery to run tests on Linux

If SwiftPM is going to insist to invoke the LinuxMain.swift, then it shouldn't get mad if --enable-test-discovery is passed to circumvent the behaviour. In that mode of operation the flag isn't deprecated: it has a clear, proactive behaviour that users genuinely want.

NeoNacho · 2021-04-07T17:09:58.901Z

Why are you keeping a LinuxMain.swift if it isn't supposed to be used? To tell users on older SwiftPM versions about the test discovery flag?