Skip to content

swift test: Fix incorrect wasmkit arguments #9114

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

Jump to bottom
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

swift test: Fix incorrect wasmkit arguments #9114

wants to merge 2 commits into from
+5 −2

Conversation

MaxDesiatov
Copy link
Contributor

wasmkit expects executable arguments to be passed after -- delimiter. Additionally, we need to bridge current directory to the host with the additional --dir . option.

Fixed #9113

@MaxDesiatov
swift test: Fix incorrect wasmkit arguments
be54abc 
`wasmkit` expects executable arguments to be passed after `--` delimiter. Additionally, we need to bridge current directory to the host with the additional `--dir .` option.

Fixes #9113
@MaxDesiatov MaxDesiatov added bug cross-compilation swift test Changes impacting `swift test` tool labels Sep 9, 2025
@MaxDesiatov MaxDesiatov added the swift sdk Changes impacting `swift sdk` tool label Sep 9, 2025
@MaxDesiatov MaxDesiatov requested a review from bnbarham September 9, 2025 18:24
@MaxDesiatov
Copy link
Contributor Author

@swift-ci test

@MaxDesiatov MaxDesiatov requested a review from grynspan September 9, 2025 19:42
grynspan
grynspan reviewed Sep 9, 2025
View reviewed changes
Sources/Commands/SwiftTestCommand.swift
@@ -966,6 +966,9 @@ final class TestRunner {
args.append(runnerPath.pathString)
args.append(contentsOf: runner.extraCLIOptions)
args.append(testPath.relative(to: localFileSystem.currentWorkingDirectory!).pathString)
if runner.path?.components.last == "wasmkit" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have a more robust way to check for WasmKit?

Copy link
Contributor Author

@MaxDesiatov MaxDesiatov Sep 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think so. Yes, it feels somewhat icky to check for a path component, but test runners from toolsets and Swift SDKs provide no other identification. Given that's all we have, why wouldn't we consider this simple and robust enough?

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test

@MaxDesiatov MaxDesiatov added the needs tests This change needs test coverage label Sep 9, 2025
@MaxDesiatov
Copy link
Contributor Author

MaxDesiatov commented Sep 9, 2025 •
edited
Loading

No new tests were added, as impacted class TestRunner doesn't seem to have any existing test harness.

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

bkhouri
bkhouri requested changes Sep 9, 2025
View reviewed changes
Copy link
Contributor

@bkhouri bkhouri left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Although the PR has low LOC changes, could I ask that you add a test that will ensure we don't regress the behaviour added by this PR?

@MaxDesiatov
Copy link
Contributor Author

MaxDesiatov commented Sep 9, 2025 •
edited
Loading

Although the PR has low LOC changes, could I ask that you add a test that will ensure we don't regress the behaviour added by this PR?

I've addressed that in the comment above, currently no new tests can be added, because none of the existing class TestRunner code has a test harness set up. The Swift SDK arguments change is covered in updated test in 6023552

@MaxDesiatov MaxDesiatov requested a review from bkhouri September 9, 2025 20:05
@MaxDesiatov MaxDesiatov removed the needs tests This change needs test coverage label Sep 9, 2025
@bkhouri
Copy link
Contributor

bkhouri commented Sep 9, 2025

Although the PR has low LOC changes, could I ask that you add a test that will ensure we don't regress the behaviour added by this PR?

I've addressed that in the comment above, currently no new tests can be added, because none of the existing class TestRunner code has a test harness set up. The Swift SDK arguments change is covered in updated test in 6023552

Can we add a test similar to the changes in #9085, more specifically where we inspect the command line arguments passed to the test runner?
Can we create a change that will allow us to verify the TestRunner class?

@MaxDesiatov
Copy link
Contributor Author

MaxDesiatov commented Sep 9, 2025 •
edited
Loading

Can we add a test similar to the changes in #9085, more specifically where we inspect the command line arguments passed to the test runner?

This requires no new tests, as mentioned above this was addressed for the existing test in 6023552 commit in this same PR.

Can we create a change that will allow us to verify the TestRunner class?

No, this was discussed in of the older PRs that added relevant modifications to TestRunner. Currently TestRunner is not written in a testable way and would require a major refactoring, which is out of scope for this PR that addresses this critical bug.

kkebo
kkebo reviewed Sep 9, 2025
View reviewed changes
Sources/Commands/SwiftTestCommand.swift
@@ -966,6 +966,9 @@ final class TestRunner {
args.append(runnerPath.pathString)
args.append(contentsOf: runner.extraCLIOptions)
args.append(testPath.relative(to: localFileSystem.currentWorkingDirectory!).pathString)
if runner.path?.components.last == "wasmkit" {
args.append("--")
Copy link
Contributor

@kkebo kkebo Sep 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

wasmkit expects executable arguments to be passed after -- delimiter.

That has been fixed by swiftwasm/WasmKit#175, and the fix has been shipped as 0.1.4. So if you're going to merge swiftlang/swift#80820, you don't need to add the -- delimiter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a great point, thanks!

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test macos self hosted

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test linux

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnbarham bnbarham bnbarham approved these changes

@grynspan grynspan grynspan approved these changes

@dschaefer2 dschaefer2 dschaefer2 approved these changes

@kateinoigakukun kateinoigakukun Awaiting requested review from kateinoigakukun

@jakepetroules jakepetroules Awaiting requested review from jakepetroules jakepetroules is a code owner

@shawnhyam shawnhyam Awaiting requested review from shawnhyam shawnhyam is a code owner

@bripeticca bripeticca Awaiting requested review from bripeticca bripeticca is a code owner

@plemarquand plemarquand Awaiting requested review from plemarquand plemarquand is a code owner

@owenv owenv Awaiting requested review from owenv owenv is a code owner

@cmcgee1024 cmcgee1024 Awaiting requested review from cmcgee1024 cmcgee1024 is a code owner

@daveyc123 daveyc123 Awaiting requested review from daveyc123 daveyc123 is a code owner

@bkhouri bkhouri Awaiting requested review from bkhouri bkhouri is a code owner

+1 more reviewer

@kkebo kkebo kkebo left review comments

Reviewers whose approvals may not affect merge requirements

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
bug cross-compilation swift sdk Changes impacting `swift sdk` tool swift test Changes impacting `swift test` tool WebAssembly
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

swift test is not supported with Swift SDK for Wasm in Swift 6.2 development snapshots
6 participants
@MaxDesiatov @bkhouri @kkebo @bnbarham @grynspan @dschaefer2