swiftlang / swift-package-manager

The Package Manager for the Swift Programming Language
Apache License 2.0
9.75k stars 1.35k forks source link

`swift build --build-tests` doesn't allow disabling tests with `--skip` or `--filter` #7953

Open finagolfin opened 2 years ago

finagolfin commented 2 years ago

Description

This is particularly useful when cross-compiling the test runner for a package, as I do for Android:

swift build --build-tests --enable-test-discovery --destination ~/swift-android-sdk/android-aarch64.json

The idea here is to be able to cross-compile the test runner and skip some tests: ~an alternative would be to add a --skip-run flag to swift test, so it builds but won't run the tests.~ (that alternative won't work, see next comment). @grynspan informs me that the new swift-testing tests can be filtered/skipped easily by passing these flags to the test runner, so this issue only deals with XCTest tests.

Expected behavior

Be able to skip or filter certain tests when cross-compiling the tests

Actual behavior

> ~/swift-DEVELOPMENT-SNAPSHOT-2022-09-24-a-ubuntu20.04/usr/bin/swift build --build-tests --enable-test-discovery --filter Contended
error: Unknown option '--filter'

Steps to reproduce

  1. ~/swift-DEVELOPMENT-SNAPSHOT-2022-09-24-a-ubuntu20.04/usr/bin/swift build --build-tests --enable-test-discovery --filter Contended

Swift Package Manager version/commit hash

5.7 and 5.8

Swift & OS version (output of swift --version && uname -a)

Never worked in Swift on Ubuntu 20.04 x86_64

finagolfin commented 2 years ago

Looking into this further, I notice that the way those two flags are implemented now is to change the list of test names given to the test runner to execute at runtime, whereas what I'd like is either to disable the test runner from even having the option of running those tests or not compiling those tests altogether, ie changing how the test runner is built instead of how it is run.

neonichu commented 2 years ago

Don't think that's doable since filtering can work on an individual test method which would require generating a new subset source file. Allowing to filter on the basis of a test target (in the package manifest sense) at build time seems appropriate, though.

finagolfin commented 2 years ago

Don't think that's doable since filtering can work on an individual test method which would require generating a new subset source file.

Something like that would be needed to avoid compiling those tests altogether, but maybe not to avoid adding them to the list of available tests that swift test -l shows? I don't know if that list is generated by XCTest or SPM, and if the former, if we can exclude certain tests.

grynspan commented 3 months ago

Sorry for the late reply! Disabling a test is a runtime feature, not a compile-time feature. It would not make sense to pass --filter or --skip to swift build because they don't impact code generation.

finagolfin commented 3 months ago

@grynspan, I think you misunderstand this issue: I'm asking for precisely the ability to disable tests at compile-time.

There are at least two reasons you might want such a feature. One is that the source itself is broken in some way and you'd like a quick way to not build it for now, without having to comment out or delete the test entirely. That is not my problem, and @neonichu noted that would be hard with XCTest, though maybe not with swift-testing.

The issue I'm concerned with is that after cross-compiling the test runner binary, the current XCTest approach only has two options when running that test binary on the cross-compilation target: it either runs all tests when invoked or you have to give it a list of the specific tests you want to run.

The way the XCTest test runner binary works now even natively with --skip and --filter is that it creates a list of tests to run and passes it to the .xctest test runner:

> swift test --filter UsageGenerationTests --vv
-- snip unrelated output --
Build complete! (1.73s)
debug: /data/data/com.termux/files/home/swift-argument-parser/.build/aarch64-unknown-linux-android24/debug/swift-argument-parserPackageTests.xctest --dump-tests-json
debug: registering '/data/data/com.termux/files/home/swift-argument-parser/.build/aarch64-unknown-linux-android24/debug/swift-argument-parserPackageTests.xctest ArgumentParserUnitTests.UsageGenerationTests/testFlagSynopsis,ArgumentParserUnitTests.UsageGenerationTests/testNameSynopsis,ArgumentParserUnitTests.UsageGenerationTests/testNonwrappedValues,ArgumentParserUnitTests.UsageGenerationTests/testPositionalSynopsis,ArgumentParserUnitTests.UsageGenerationTests/testSynopsis,ArgumentParserUnitTests.UsageGenerationTests/testSynopsisWithCustomization,ArgumentParserUnitTests.UsageGenerationTests/testSynopsisWithDefaultValueAndTransform,ArgumentParserUnitTests.UsageGenerationTests/testSynopsisWithDefaults,ArgumentParserUnitTests.UsageGenerationTests/testSynopsisWithHidden,ArgumentParserUnitTests.UsageGenerationTests/testSynopsisWithMultipleCustomNames,ArgumentParserUnitTests.UsageGenerationTests/testSynopsisWithOptional,ArgumentParserUnitTests.UsageGenerationTests/testSynopsisWithPostTerminatorParsingStrategy,ArgumentParserUnitTests.UsageGenerationTests/testSynopsisWithRepeats,ArgumentParserUnitTests.UsageGenerationTests/testSynopsisWithSingleDashLongNameFirst,ArgumentParserUnitTests.UsageGenerationTests/testSynopsisWithTooManyOptions,ArgumentParserUnitTests.UsageGenerationTests/testSynopsisWithTransform' with terminator
debug: /data/data/com.termux/files/home/swift-argument-parser/.build/aarch64-unknown-linux-android24/debug/swift-argument-parserPackageTests.xctest ArgumentParserUnitTests.UsageGenerationTests/testFlagSynopsis,ArgumentParserUnitTests.UsageGenerationTests/testNameSynopsis,ArgumentParserUnitTests.UsageGenerationTests/testNonwrappedValues,ArgumentParserUnitTests.UsageGenerationTests/testPositionalSynopsis,ArgumentParserUnitTests.UsageGenerationTests/testSynopsis,ArgumentParserUnitTests.UsageGenerationTests/testSynopsisWithCustomization,ArgumentParserUnitTests.UsageGenerationTests/testSynopsisWithDefaultValueAndTransform,ArgumentParserUnitTests.UsageGenerationTests/testSynopsisWithDefaults,ArgumentParserUnitTests.UsageGenerationTests/testSynopsisWithHidden,ArgumentParserUnitTests.UsageGenerationTests/testSynopsisWithMultipleCustomNames,ArgumentParserUnitTests.UsageGenerationTests/testSynopsisWithOptional,ArgumentParserUnitTests.UsageGenerationTests/testSynopsisWithPostTerminatorParsingStrategy,ArgumentParserUnitTests.UsageGenerationTests/testSynopsisWithRepeats,ArgumentParserUnitTests.UsageGenerationTests/testSynopsisWithSingleDashLongNameFirst,ArgumentParserUnitTests.UsageGenerationTests/testSynopsisWithTooManyOptions,ArgumentParserUnitTests.UsageGenerationTests/testSynopsisWithTransform
Test Suite 'Selected tests' started at 2024-07-24 20:07:56.157
Test Suite 'UsageGenerationTests' started at 2024-07-24 20:07:56.164
Test Case 'UsageGenerationTests.testFlagSynopsis' started at 2024-07-24 20:07:56.164
Test Case 'UsageGenerationTests.testFlagSynopsis' passed (0.004 seconds)

That may be fine for native compilation and test runs, but for cross-compilation, it would be better if tests could be selected at compile-time to not be run.

As for whether it "would not make sense to pass --filter or --skip" for this, that is merely my suggested interface for this feature.

The real question is whether the feature of skipping tests at compile-time through command-line flags will be implemented, the particular flag names chosen is irrelevant.

If you're unsure about implementing that underlying feature, better to reopen this, as I think it is a worthwhile feature, given the limitations of the current test runner.

grynspan commented 3 months ago

I understand what you're asking for. 🙂 The feature you're requesting will not be implemented.

grynspan commented 3 months ago

If you have a test that doesn't work on some platform, the canonical way to disable it at compile time is with #if:

#if !os(Android)
func testThatDoesNotWorkOnAndroid() { ... }
#endif

It is also possible to set up compiler conditionals for this purpose:

// In Package.swift:
.define("RUN_FANCY_TESTS", .when(platforms: [.android])),

// In test files:
#if RUN_FANCY_TESTS
final class FancyTests: XCTestCase {
  ...
}
#endif

If the test compiles but fails at runtime, you can also use the XCTSkip API family to skip the test.

finagolfin commented 3 months ago

The feature you're requesting will not be implemented.

Meaning Apple devs won't implement it or because you think nobody should? If the former, this should be reopened; if the latter, you haven't given any explanation.

finagolfin commented 3 months ago

If you have a test that doesn't work on some platform, the canonical way to disable it at compile time is with #if:

Yes, I'm aware of all that. The advantage of the --skip and --filter flags is to be able to do that quickly on the command-line. That currently only works for native test runs though, hence asking for this feature to help with cross-compilation test runs.

grynspan commented 3 months ago

What precisely do you envision these flags doing at compile time? Presumably you intend for them to affect exactly what part of a test target is compiled, effectively commenting out any test functions that are not included?

finagolfin commented 3 months ago

What precisely do you envision these flags doing at compile time?

Simple, XCTest is clearly generating a list of tests internally, which SwiftPM queries with --dump-tests-json above to determine what tests to run. I would like at compile-time to be able to filter/skip tests from being added to that test list in the first place, ie the test runner won't even list them in .build/aarch64-unknown-linux-android24/debug/swift-argument-parserPackageTests.xctest -l and thus not run them at all in the cross-compiled binary.

grynspan commented 3 months ago

In that scenario, the code will still need to compile, but it would affect the output of swift test list (and --dump-tests-json.) Note that on Apple platforms, XCTest behaves differently and there is no pregenerated list of tests, so this feature would not work there.

Why is it not sufficient to write e.g.:

#if os(Android)
throw XCTSkip("Not supported on Android.")
#endif

Or something similar? What is the immediate benefit of introducing a new feature here if a developer can already effectively accomplish the same task in several different ways?

finagolfin commented 3 months ago

In that scenario, the code will still need to compile, but it would affect the output of swift test list (and --dump-tests-json.)

Yes, I'm just trying to change the list of tests run, however it's implemented.

Note that on Apple platforms, XCTest behaves differently and there is no pregenerated list of tests, so this feature would not work there.

OK, I don't use Apple platforms, but given how most Swift code is cross-compiled, this might be useful there too, even if it would need to be implemented differently.

What is the immediate benefit of introducing a new feature here if a developer can already effectively accomplish the same task in several different ways?

Why have --skip/--filter CLI flags at all then? I completely agree that it is possible to do this much more laboriously now: the advantage of this new feature would be to quickly and easily change the subset of cross-compiled tests run on the fly by using the CLI, just as you can natively now.

grynspan commented 3 months ago

OK, I don't use Apple platforms, but given how most Swift code is cross-compiled, this might be useful there too, even if it would need to be implemented differently.

I would wager a significant percentage of Swift developers do use Apple platforms. 😁 Generally speaking, developers expect that any changes to swift-corelibs-xctest functionality are going to be compatible with Apple's XCTest implementation. This sort of change would not be straightforward to implement in Apple's XCTest, so please keep that in mind.

Why have --skip/--filter CLI flags at all then?

I'm not questioning the utility of filtering tests at runtime. I'm questing the utility of filtering them at compile time in another way. Adding --skip/--filter to swift build means that, if you call swift build with different options, you'd end up rebuilding the test target each time. This is expensive compared to building the tests, then just running them with different configurations as needed.

finagolfin commented 3 months ago

Generally speaking, developers expect that any changes to swift-corelibs-xctest functionality are going to be compatible with Apple's XCTest implementation. This sort of change would not be straightforward to implement in Apple's XCTest, so please keep that in mind.

OK, is this feature available now when running Swift packages' tests on the iOS simulator, ie being able to change the subset of tests run from the CLI? If not, don't you think it would be useful?

I'm questing the utility of filtering them at compile time in another way. Adding --skip/--filter to swift build means that, if you call swift build with different options, you'd end up rebuilding the test target each time. This is expensive compared to building the tests, then just running them with different configurations as needed.

These are implementation details that can likely be avoided.

Note the goal of this feature is still the same as the current --skip/--filter flags: change the list of tests that are run, but because these tests are cross-compiled, it has to be done differently. Another way to implement this feature would be to give the final .xctest test runner binary a richer command-line interface, eg being able to pass it --skip/--filter flags, rather than only a full list of tests now.

finagolfin commented 3 months ago

Ping @grynspan, this should be reopened, as you yourself admit these flags are useful when natively compiling and cross-compilation is the dominant use of Swift, where they don't work. Whether implementation will be easy is irrelevant, someone else from the community may always implement this if Apple doesn't have the resources.

grynspan commented 3 months ago

Another way to implement this feature would be to give the final .xctest test runner binary a richer command-line interface, eg being able to pass it --skip/--filter flags, rather than only a full list of tests now.

This is a significantly different problem/solution than we were originally discussing here, and one that is more feasible to implement.

The good news is that Swift Testing already supports this functionality. On non-Darwin platforms, directly executing the .xctest executable and passing --testing-library swift-testing will call the Swift Testing library's entry point function which performs its own argument parsing and can interpret --skip, --filter, and various other arguments. This is, in fact, what swift test does. (On Darwin platforms, a helper executable is now included with Swift Package Manager that loads the .xctest bundle and then triggers the same entry point function.)

The less good news is that for similar functionality to be implemented in swift-corelibs-xctest, we need to also implement it in the XCTest framework included in Xcode. Xcode is Apple's proprietary software and not part of the Swift open source project, while swift-corelibs-xctest is an open-source reimplementation of XCTest that seeks to provide a subset of the functionality available from XCTest.

If this solution would meet your needs, we can transfer this issue to Swift Testing (once the repo moves to swiftlang in a few days) and reopen it. I'd also ask that you file feedback with Apple asking for the same feature in XCTest and share the FB number here.

finagolfin commented 3 months ago

This is a significantly different problem/solution than we were originally discussing here

It isn't, the original description always stated the problem to be solved: "The idea here is to be able to cross-compile the test runner and skip some tests." The rest was just discussing how that might be implemented.

The good news is that Swift Testing already supports this functionality. On non-Darwin platforms, directly executing the .xctest executable and passing --testing-library swift-testing will call the Swift Testing library's entry point function which performs its own argument parsing and can interpret --skip, --filter, and various other arguments.

Great, I did not know that was an option: it will probably suffice for this issue.

The less good news is that for similar functionality to be implemented in swift-corelibs-xctest

I'm just invoking SwiftPM, which library is used underneath isn't important to me.

I have not been following the transition to swift-testing, I will start doing so now that you say it has a feature like this.

If this solution would meet your needs, we can transfer this issue to Swift Testing (once the repo moves to swiftlang in a few days) and reopen it. I'd also ask that you file feedback with Apple asking for the same feature in XCTest and share the FB number here.

That's okay, I'm just interested in using this feature in SwiftPM. I will try it out and report back, this issue can be kept closed in the interim.

As for XCTest having this feature too, that doesn't really matter to me. What matters to me is that some combination of flags passed to SwiftPM in my CI, then to the cross-compiled test binary, allows me to quickly enable and disable what tests are run without having to patch the source. Precisely which testing library executes this underneath is irrelevant to me. 😄

finagolfin commented 3 months ago

Alright, looked into this by creating a test project with one XCTest test and one using the new Testing library. It looks like SwiftPM executes the resulting test runner .xctest executable twice: once to run XCTest tests using that library and again a second time with --testing-library swift-testing to run those tests.

When using --skip/--filter, the XCTest tests all have to be listed out for the runner, while the Testing tests use the new --skip/--filter flags in the test runner that you mention.

Since those new runner flags can only be used for Testing tests, they are not useful for the vast majority of packages that only use XCTest.

for similar functionality to be implemented in swift-corelibs-xctest, we need to also implement it in the XCTest framework included in Xcode.

Why? There are certainly differences already between the way SwiftPM operates on macOS versus other platforms.

we can transfer this issue to Swift Testing (once the repo moves to swiftlang in a few days) and reopen it.

I don't see why, you've pointed out that this feature already exists for Testing tests.

I'd also ask that you file feedback with Apple asking for the same feature in XCTest and share the FB number here.

Not my concern, as I don't use Apple platforms.

@grynspan, I take your point that this has been implemented in swift-testing, but that is no help to me since the vast majority of Swift tests are XCTest tests. Please reopen, and I will specify in the OP that this has been implemented in swift-testing but not XCTest.

Obviously this is a useful feature, or swift-testing would not have implemented it. There are probably at least 3-4 ways it could be implemented for XCTest driven by SwiftPM also, this issue is asking for that and discussing ways it could be done.

Apple's internal rules and requirements shouldn't matter for discussion and implementation of this feature. If Apple doesn't want this for XCTest, it could be implemented by others outside Apple and only available on non-Darwin platforms with XCTest, just as there are already Apple-specific features in SwiftPM.

finagolfin commented 3 months ago

Ping @grynspan, this should be reopened: it's great that swift-testing supports this, but XCTest and thus most Swift packages do not. I will update the OP to point out this is currently only needed for XCTest once it's reopened.

finagolfin commented 2 months ago

Ping @neonichu, please reopen: the rationale for closing this makes no sense, and he has stopped responding.

grynspan commented 2 months ago

As explained in the readme for swift-corelibs-xctest:

This version of XCTest implements the majority of unit testing APIs included in XCTest from Xcode 7 and later. Its goal is to enable your project's tests to run on all the platforms Swift supports without having to rewrite them.

In other words, features not present in XCTest.framework (part of Xcode) are not generally considered candidates for inclusion in swift-corelibs-xctest. As I wrote previously, the next step here is to file a feature request with Apple. If you are unable to do so, I can file one for you.

Separately, swift-corelibs-xctest has been migrated to the swiftlang organization on GitHub, so we can transfer this issue now. I'll do that momentarily, so don't be surprised if you see this issue disappear from swift-package-manager.

finagolfin commented 2 months ago

In other words, features not present in XCTest.framework (part of Xcode) are not generally considered candidates for inclusion in swift-corelibs-xctest.

It is not clear that modifying XCTest in any way will be required. I happened to be looking at the generated files that SwiftPM uses to build a package's test runner for a different issue last week, and I noticed that it creates Swift files with a generated list of auto-discovered XCTest tests to run in .build/aarch64-unknown-linux-android24/debug/swift-nioPackageDiscoveredTests.derived/. Since SwiftPM generates that list in Swift files, it should be pretty easy to implement this test filtering feature in SwiftPM.

we can transfer this issue now. I'll do that momentarily, so don't be surprised if you see this issue disappear from swift-package-manager.

I received no notification that this was transfered, even though you wrote this comment here, so I thought this issue was deleted. Only after I talked to @bnbarham on the forum and he pointed me at this link did I see this issue again.

Please move this back to SwiftPM and reopen. Nobody is asking you to implement or otherwise get involved with this matter. All I was looking for was info on how this was internally implemented by SwiftPM, which perhaps you and the other devs in this thread were unaware of, and ideas and feedback from the community on how to move forward.

If you are worried that people will misunderstand and think that this feature has not been implemented in the swift-testing repo already and bother you for it, I already told you almost a month ago that I'll be happy to modify the OP to specify that I want this for XCTest tests only in SwiftPM, because it is already available in swift-testing tests.

finagolfin commented 2 months ago

Ping @bnbarham, do you mind reopening and moving this issue back to SwiftPM? Not getting much response from Jon here, and the stated reason for closing this makes no sense to me.

grynspan commented 2 months ago

You probably want @dschaefer2 now, actually.

I'm not ignoring you. I have asked @briancroom to join the discussion.

dschaefer2 commented 2 months ago

Since SwiftPM generates that list in Swift files, it should be pretty easy to implement this test filtering feature in SwiftPM.

Despite it looking easy, we can't see how we'd implement this. That's kinda why it's sitting here. Any advice on how we could would be helpful. PRs even more so.

finagolfin commented 2 months ago

we can't see how we'd implement this. That's kinda why it's sitting here. Any advice on how we could would be helpful.

All the more reason to reopen and move it back. I don't claim to know how this works for XCTest exactly yet, and I suspect nobody else in this thread fully does either. The whole point of leaving this open is so we can get more feedback and info from other devs who want this feature.

I am suggesting that based on the fact that SwiftPM's automated test discovery generates lists of XCTest tests in Swift files before building the XCTest test runner with them, we could get SwiftPM to modify those generated lists using these flags. Obviously that won't work for Swift packages that don't use automated test discovery, instead listing out all their XCTest tests to run manually, but that's fine by me, as those are a minority now in my experience.

Maybe there's some other problem that blocks this idea: I haven't looked at the relevant SwiftPM source yet. But closing this issue and prematurely saying it can't be done ensures we won't get a proper discussion about that.

dschaefer2 commented 2 months ago

I'm happy to reopen, but it's not going to make any progress unless a PR lands.

finagolfin commented 2 months ago

@dschaefer2, not sure what "it's not going to make any progress" refers to: nobody is asking you or any other Apple dev to implement this new feature. And it can make progress just by discussion, ie people discussing how it might be done until someone finds a way.

Please move this back to the SwiftPM repo where I opened it, as it's not clear this will require any changes to XCTest, certainly not if my last idea is feasible, and that's where other devs will look for this SwiftPM feature. Even if it eventually requires some XCTest changes, this is primarily a user-facing SwiftPM feature.

grynspan commented 2 months ago

So you're aware, the implementation of XCTest on Darwin (where it's an Apple-proprietary framework that ships inside Xcode) is very different from the implementation used on Linux/Windows/Android/etc. where it's an open-source partial reimplementation of the aforementioned Xcode framework.

Test discovery is also very different between platforms: on Darwin, it's done by querying the Objective-C runtime. When using the open-source repository, it's done at compile-time using the mechanism you've identified.

Swift Package Manager has no ability to affect the code generated discovery codepath on Darwin, which means a solution implemented in Swift Package Manager is incomplete. Although you've indicated you are primarily concerned with Android, the Swift project as a whole does consider Darwin an important platform family. Please do keep that in mind as you work on your PR. :)

dschaefer2 commented 2 months ago

not sure what "it's not going to make any progress" refers to: nobody is asking you or any other Apple dev to implement this new feature.

Of course. I look forward to community contributions here.

finagolfin commented 2 months ago

Test discovery is also very different between platforms: on Darwin, it's done by querying the Objective-C runtime. When using the open-source repository, it's done at compile-time using the mechanism you've identified. Swift Package Manager has no ability to affect the code generated discovery codepath on Darwin, which means a solution implemented in Swift Package Manager is incomplete.

Understood, but maybe this can be a feature only made available when cross-compiling to non-Darwin platforms with an SDK that contains the OSS swift-corelibs-xctest? I don't use Darwin platforms or Windows, so I can't be expected to develop these features for those different platforms, particularly since you say each uses different platform-specific mechanisms to discover tests.

Perhaps this can be made available as a cross-compilation feature to Unix platforms only initially, then expanded later to other platforms by those who use and want this feature there too.

grynspan commented 2 months ago

I don't use Darwin platforms or Windows, so I can't be expected to develop these features for those different platforms, particularly since you say each uses different platform-specific mechanisms to discover tests.

That's why I had asked you to file feedback with Apple: so we can get the request into Apple's systems and perhaps start working on a solution there as well. 🙂 I could write a feedback report myself, but from this thread I get the sense I wouldn't capture the problem as well as you would.

(Windows uses the same test discovery mechanism as Linux.)

finagolfin commented 2 months ago

@grynspan, alright, let me see if I can get this working when cross-compiling tests for non-Darwin platforms at some point, then we can see what you all want to do about Darwin.