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

Misleading CLI help for swift test --parallel #8174

Open
1 task done
MahdiBM opened this issue Dec 16, 2024 · 13 comments
Open
1 task done

Misleading CLI help for swift test --parallel #8174

MahdiBM opened this issue Dec 16, 2024 · 13 comments

Comments

@MahdiBM
Copy link

MahdiBM commented Dec 16, 2024

Is it reproducible with SwiftPM command-line tools: swift build, swift test, swift package etc?

  • Confirmed reproduction steps with SwiftPM CLI. The description text must include reproduction steps with either of command-line SwiftPM commands, swift build, swift test, swift package etc.

Description

This is the "help" for the --parallel flag:

swift test --help | grep "\-parallel"                                               
  --parallel/--no-parallel
                          Run the tests in parallel. (default: --no-parallel)

Expected behavior

Don't say default: --no-parallel, instead say something like default: --no-parallel for XCTest, --parallel for swift-testing.

Actual behavior

mentions default: --no-parallel which is incorrect for swift-testing.

Steps to reproduce

swift test --help | grep "\-parallel"     

Swift Package Manager version/commit hash

Swift Package Manager - Swift 6.0.3

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

swift-driver version: 1.115.1 Apple Swift version 6.0.3 (swiftlang-6.0.3.1.8 clang-1600.0.30.1)
Target: arm64-apple-macosx15.0
Darwin 24.2.0 Darwin Kernel Version 24.2.0: Sun Nov  3 20:52:07 PST 2024; root:xnu-11215.60.405~54/RELEASE_ARM64_T6000 arm64
@MahdiBM MahdiBM added the bug label Dec 16, 2024
@finagolfin
Copy link
Member

@grynspan, would you fix this? I too found it jarring that the defaults are different for both. Ideally, --parallel would be the default for both, as that's what I pass in most of the time, but if you decide to keep the default --no-parallel for consistency, Testing should do the same.

Let us know what you think.

@grynspan
Copy link
Contributor

We should make sure the CLI documentation is accurate. We do not plan to change the implemented behaviour at this time as making XCTest parallel by default will break a lot of tests that were written before the feature was added.

@finagolfin
Copy link
Member

We do not plan to change the implemented behaviour at this time

Meaning you will not change Testing to serial by default either, to match the XCTest default? I think the first step here is getting both to do the same by default.

@grynspan
Copy link
Contributor

Swift Testing is intentionally parallel by default. We do not intend to change that. I would suggest you reach out to @briancroom if you need more information.

@MahdiBM
Copy link
Author

MahdiBM commented Jan 20, 2025

I think both libraries' default are already set in stone.
I don't think we can do much to change the behaviors in a beneficial way that is also worth the trouble, without breaking users.

I'd say we should just somehow fix what the CLI says and move on.

@finagolfin
Copy link
Member

OK, pinging @briancroom, we are in a weird situation here where SwiftPM is by default running XCTest tests serially and Testing tests in parallel. Can we set the default of both to be the same?

Pinging @bnbarham and @dschaefer2 on the SwiftPM side too.

@dschaefer2
Copy link
Member

Can we set the default of both to be the same?

I think it's pretty clear from the people involved that the answer is no.

I'm with @MahdiBM, we can probably fix the wording.

@finagolfin
Copy link
Member

I think it's pretty clear from the people involved that the answer is no.

Who would that be and why not? I've only heard from Jon so far that "Swift Testing is intentionally parallel by default." That has no bearing on whether the default can be changed.

I understand that there are good reasons not to change a legacy library like XCTest, but since Testing is brand new, I don't see why it shouldn't be made consistent by default.

@grynspan
Copy link
Contributor

Brian's been notified about this thread, but he has the day off as it's a holiday in the United States. Please give him time to respond.

(Also that's not my name. 🙃)

@finagolfin
Copy link
Member

Please give him time to respond.

I've only pinged him once and am not expecting an answer right away. I was simply explaining to Doug why the previous response didn't really answer my question.

Also that's not my name

Your github profile says your name is Jonathan and at least in the US, it is common to shorten that as I did. If you don't like that shortening for some reason, just let me know what you prefer.

@jakepetroules
Copy link
Contributor

Let's keep this issue focused on the original topic of improving the help documentation to be clearer.

Whether the current behavior should be changed is a different debate, but as previously noted, the DRIs for SwiftTesting have already made clear that they would like the default to remain "parallel".

I'd also like to add that XCTest and SwiftTesting are only two possible testing frameworks. Consider whether SwiftPM added support for other testing systems in future, some of which might only run serially, some of which might only run parallel, and/or some of which might have specific reasons for default to one or the other. So I don't think it necessarily adds anything to suggest that all testing systems supported by SwiftPM should have the same parallelism defaults.

@finagolfin
Copy link
Member

Let's keep this issue focused on the original topic of improving the help documentation to be clearer.

We can't without knowing if that inconsistency was a mistake in the first place.

as previously noted, the DRIs for SwiftTesting have already made clear that they would like the default to remain "parallel".

Then surely they can explain why that decision was made? Nobody in this thread has so far.

I don't think it necessarily adds anything to suggest that all testing systems supported by SwiftPM should have the same parallelism defaults.

Leaving aside hypotheticals that don't exist, my understanding is that both these two supported testing libraries support both parallel and serial modes. As such, it is inconsistent for SwiftPM to set different defaults for each. If it's so important to keep XCTest backwards-compatible with previous serial-only tests, I see no reason not to favor consistency by making Testing also run serially by default.

It's fine to suggest that consistency is sometimes trumped by other concerns, but so far no argument has been made for why Testing can't be made consistent by default.

@jakepetroules
Copy link
Contributor

@grynspan or @briancroom can add their own takes if they like, but mine is that parallelism by default allows test execution to benefit from faster wall time performance without needing to pass special flags (or know they exist), and discourages accidental reliance on serial execution. I believe that's a greater user benefit than the consistency of all supported test execution engines having the same default w.r.t. parallel execution.

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

No branches or pull requests

5 participants