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.

Sign up for GitHub

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

Reenable testUsage() as testInvalidUsage() #8014

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+36 −37
Open

Reenable testUsage() as testInvalidUsage() #8014

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a9ceaf7
Reenable testUsage() as testInvalidUsage()
plemarquand Oct 1, 2024
ed15f87
Merge branch 'main' into reenable-testusage
plemarquand Oct 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 36 additions & 37 deletions Tests/CommandsTests/PackageCommandTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,9 @@ final class PackageCommandTests: CommandsTestCase {
XCTAssertMatch(stdout, .contains("USAGE: swift package"))
}

func testUsage() async throws {
throw XCTSkip("rdar://131126477")
Copy link
Contributor

@MaxDesiatov MaxDesiatov Oct 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Do we have this radar resolved then? I find it confusing that any option that starts with -h leads to this unexpected behavior. We should at least raise this on the SAP repo for them to resolve this first, especially as this is a regression from Swift Argument Parser 1.2.2.

Copy link
Contributor Author

@plemarquand plemarquand Oct 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think this is works as intended, but maybe @natecook1000 can chime in. If any argument resolves to a help flag it takes precedence and shows the help string.

Swift Argument Parser respects combined flags, so -halp is equivalent to -h -a -l -p. Short flags can be combined, and short flags must also be only one character, so -halp isn’t a valid as a single flag. Only short flags can be combined, so —help-foo won’t match —help.

Copy link
Contributor

Choose a reason for hiding this comment

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

cc @rauhul

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm surprised that's the default, given that both swiftc and clang stick to single dash options, and thus clang -fblocks is clearly not equivalent to clang -f -b -l -o -c -k -s.

Copy link
Member

Choose a reason for hiding this comment

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

swiftc and clang also use options with -long-name so I wouldn't take them as amazing prior art. arg parser follows the pattern of ls -asl and git clean -Xdf where short names are combined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MaxDesiatov @rauhul Any lingering concerns with this PR/behaviour?

Copy link
Member

Choose a reason for hiding this comment

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

I'm happy with it

Copy link
Contributor

Choose a reason for hiding this comment

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

Even if swift build -halp is parsed the same as swift build -h -a -l -p and -h is a valid flag, I'd still expect an error message to be printed about invalid -a -l -p flags.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @MaxDesiatov .

I also think swift-argument-parser test need to be augmented to cover this case, if they currently don't!

Copy link
Contributor

@bkhouri bkhouri Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

After looking at the current swift package manager behaviour, I think I'm find with it.

Running swift build -halp and swift build -alhp both yield the help usage. That's ok.

Running swift build -alp yields an error.

❯ swift build -alp
error: Unknown option '-alp'. Did you mean '--vv'?
Usage: swift build <options>
  See 'build -help' for more information.

The usage help should probably indicate See 'build --help for more informationorSee 'build -h' for more information. it currently has a single -beforehelp`.

func testInvalidUsage() async throws {
do {
_ = try await execute(["-halp"])
_ = try await execute(["--xxx"])
Copy link
Contributor

Choose a reason for hiding this comment

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

issue: Can we add a test, if they don't currently exist, that specified the behaviour when executing swift build -halp, swift build -alhp and swift build --help

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think these tests should probably go in swift-argument-parser, since it handles the help arguments

Copy link
Contributor

Choose a reason for hiding this comment

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

Similar tests should definitely be included in swift-argument-parser. However, from swift-package-manager perspective, it's an implementation details.

This is a large (ie: end-to-end) test, where the code under test is not visible. So from the tests's perspective, we don't know we are using swift-argument-parser, or maybe we will use a different argument parser in the future, which breaks the behaviour. As such, tests should be included in swift-package-manager that verify the expected behaviour as a software developer would use the command line. We should include large (ie: end-to-end) tests that cover the expected behaviour, or maybe at least describe the existing behaviour.

XCTFail("expecting `execute` to fail")
} catch SwiftPMError.executionFailure(_, _, let stderr) {
XCTAssertMatch(stderr, .contains("Usage: swift package"))
Expand All @@ -69,27 +68,27 @@ final class PackageCommandTests: CommandsTestCase {
let stdout = try await execute(["--version"]).stdout
XCTAssertMatch(stdout, .contains("Swift Package Manager"))
}

func testCompletionTool() async throws {
let stdout = try await execute(["completion-tool", "--help"]).stdout
XCTAssertMatch(stdout, .contains("OVERVIEW: Completion command (for shell completions)"))
}

func testInitOverview() async throws {
let stdout = try await execute(["init", "--help"]).stdout
XCTAssertMatch(stdout, .contains("OVERVIEW: Initialize a new package"))
}
func testInitUsage() async throws {
let stdout = try await execute(["init", "--help"]).stdout
XCTAssertMatch(stdout, .contains("USAGE: swift package init [--type <type>] "))
XCTAssertMatch(stdout, .contains(" [--name <name>]"))
}
func testInitOptionsHelp() async throws {
let stdout = try await execute(["init", "--help"]).stdout
XCTAssertMatch(stdout, .contains("OPTIONS:"))
}
func testInitOverview() async throws {
let stdout = try await execute(["init", "--help"]).stdout
XCTAssertMatch(stdout, .contains("OVERVIEW: Initialize a new package"))
}

func testInitUsage() async throws {
let stdout = try await execute(["init", "--help"]).stdout
XCTAssertMatch(stdout, .contains("USAGE: swift package init [--type <type>] "))
XCTAssertMatch(stdout, .contains(" [--name <name>]"))
}

func testInitOptionsHelp() async throws {
let stdout = try await execute(["init", "--help"]).stdout
XCTAssertMatch(stdout, .contains("OPTIONS:"))
}

func testPlugin() async throws {
await XCTAssertThrowsCommandExecutionError(try await execute(["plugin"])) { error in
Expand Down Expand Up @@ -2301,13 +2300,13 @@ final class PackageCommandTests: CommandsTestCase {
}

try await runPlugin(flags: [], diagnostics: ["print", "progress", "remark"]) { stdout, stderr in
XCTAssertMatch(stdout, isOnlyPrint)
XCTAssertMatch(stdout, isOnlyPrint)
XCTAssertMatch(stderr, containsProgress)
}

try await runPlugin(flags: [], diagnostics: ["print", "progress", "remark", "warning"]) { stdout, stderr in
XCTAssertMatch(stdout, isOnlyPrint)
XCTAssertMatch(stderr, containsProgress)
XCTAssertMatch(stdout, isOnlyPrint)
XCTAssertMatch(stderr, containsProgress)
XCTAssertMatch(stderr, containsWarning)
}

Expand All @@ -2330,24 +2329,24 @@ final class PackageCommandTests: CommandsTestCase {
}

try await runPlugin(flags: ["-q"], diagnostics: ["print", "progress"]) { stdout, stderr in
XCTAssertMatch(stdout, isOnlyPrint)
XCTAssertMatch(stdout, isOnlyPrint)
XCTAssertMatch(stderr, containsProgress)
}

try await runPlugin(flags: ["-q"], diagnostics: ["print", "progress", "remark"]) { stdout, stderr in
XCTAssertMatch(stdout, isOnlyPrint)
XCTAssertMatch(stdout, isOnlyPrint)
XCTAssertMatch(stderr, containsProgress)
}

try await runPlugin(flags: ["-q"], diagnostics: ["print", "progress", "remark", "warning"]) { stdout, stderr in
XCTAssertMatch(stdout, isOnlyPrint)
XCTAssertMatch(stdout, isOnlyPrint)
XCTAssertMatch(stderr, containsProgress)
}

try await runPluginWithError(flags: ["-q"], diagnostics: ["print", "progress", "remark", "warning", "error"]) { stdout, stderr in
XCTAssertMatch(stdout, isOnlyPrint)
XCTAssertMatch(stderr, containsProgress)
XCTAssertNoMatch(stderr, containsRemark)
XCTAssertMatch(stdout, isOnlyPrint)
XCTAssertMatch(stderr, containsProgress)
XCTAssertNoMatch(stderr, containsRemark)
XCTAssertNoMatch(stderr, containsWarning)
XCTAssertMatch(stderr, containsError)
}
Expand All @@ -2363,27 +2362,27 @@ final class PackageCommandTests: CommandsTestCase {
}

try await runPlugin(flags: ["-v"], diagnostics: ["print", "progress"]) { stdout, stderr in
XCTAssertMatch(stdout, isOnlyPrint)
XCTAssertMatch(stderr, containsProgress)
XCTAssertMatch(stdout, isOnlyPrint)
XCTAssertMatch(stderr, containsProgress)
}

try await runPlugin(flags: ["-v"], diagnostics: ["print", "progress", "remark"]) { stdout, stderr in
XCTAssertMatch(stdout, isOnlyPrint)
XCTAssertMatch(stderr, containsProgress)
XCTAssertMatch(stdout, isOnlyPrint)
XCTAssertMatch(stderr, containsProgress)
XCTAssertMatch(stderr, containsRemark)
}

try await runPlugin(flags: ["-v"], diagnostics: ["print", "progress", "remark", "warning"]) { stdout, stderr in
XCTAssertMatch(stdout, isOnlyPrint)
XCTAssertMatch(stderr, containsProgress)
XCTAssertMatch(stdout, isOnlyPrint)
XCTAssertMatch(stderr, containsProgress)
XCTAssertMatch(stderr, containsRemark)
XCTAssertMatch(stderr, containsWarning)
}

try await runPluginWithError(flags: ["-v"], diagnostics: ["print", "progress", "remark", "warning", "error"]) { stdout, stderr in
XCTAssertMatch(stdout, isOnlyPrint)
XCTAssertMatch(stderr, containsProgress)
XCTAssertMatch(stderr, containsRemark)
XCTAssertMatch(stdout, isOnlyPrint)
XCTAssertMatch(stderr, containsProgress)
XCTAssertMatch(stderr, containsRemark)
XCTAssertMatch(stderr, containsWarning)
XCTAssertMatch(stderr, containsError)
}
Expand Down