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

Introduce a new SPM-based test harness #307

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Introduce a new SPM-based test harness #307

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
db73038
Revert "Support Rust returning `-> Result<_, String>` (#296)"
colinmarc Jan 27, 2025
5c3d672
Add Sendable to all async return types in tests
colinmarc Jan 27, 2025
5aaf654
Introduce a new SPM-based test harness
colinmarc Jan 26, 2025
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
2 changes: 0 additions & 2 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,6 @@ members = [
"crates/swift-bridge-macro",

"crates/swift-integration-tests",
"SwiftRustIntegrationTestRunner/integration-test-create-swift-package",
"SwiftRustIntegrationTestRunner/swift-package-rust-library-fixture",
Comment on lines -36 to -37
Copy link
Owner

Choose a reason for hiding this comment

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

Why was this removed?

We should keep it around until:

  • all tests have been ported over
  • we're confident that we won't need an integration tests that can only run in Xcode
    • i.e. if/when we're sure that swift testing is capable of testing 100% of the things that we might care to test, both now and in the future

Copy link
Author

Choose a reason for hiding this comment

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

It's a bit awkward to have both the old and new integration test suite in the same PR, so my intention was to keep this as a draft until all current tests are migrated over (verified by running the tests and counting the assertions, or whatever). The reason is this:

  • The swift code depends on the swift-integration-tests crate
  • That crate depends on the swift code in SwiftRustIntegrationTestRunner (Result.swift, etc)
  • Therefore, that code has to be duplicated in both harnesses, or there has two be two different identical rust crates.

If you favor a more incremental approach, I would propose the following:

  • Leave the existing harness untouched
  • Create a blank "project" as the new test harness, with no rust code and no swift code. (Or maybe just the primitive tests as a starter.)
  • Incrementally add tests to it, adding both the needed rust code and needed swift code, in a series of PRs.

Copy link
Owner

Choose a reason for hiding this comment

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

Therefore, that code has to be duplicated in both harnesses, or there has two be two different identical rust crates.

I'm not quite following why we'd need to duplicate anything vs. restructuring things such that we can move Rust+Swift code over from the old suite to the new suite, one batch at time.
i.e., Old Swift + Old Rust (starts with everything) -> New Swift + New Rust (starts with nothing)

Maybe that's what your incremental approach proposal is saying. I'm not quite sure if you're suggesting that there are downsides to the incremental approach.

Regardless, I'm cool with whichever option you prefer.
I'm just happy to see this work progressing and won't bother trying to weigh the pros and cons, so anything that best helps you land it is more than fine by me.

Copy link
Author

Choose a reason for hiding this comment

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

It's probably a bit slower, but it's also cleaner. Once #310 and #312 land, I will rebase and push a version that does that (and then mark the PR as not a draft).

Copy link
Owner

Choose a reason for hiding this comment

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

I've replied to #310 and #312 with recommendations for how to land them.

Hoping to unblock this Introduce a new SPM-based test harness PR so that we can merge it and have a foundation that we can start to build upon (by gradually porting tests over).


"examples/async-functions",
"examples/codegen-visualizer",
Expand Down
14 changes: 0 additions & 14 deletions SwiftRustIntegrationTestRunner/Headers/BridgingHeader.h
View file
Open in desktop

This file was deleted.

718 changes: 0 additions & 718 deletions SwiftRustIntegrationTestRunner/SwiftRustIntegrationTestRunner.xcodeproj/project.pbxproj
View file
Open in desktop

This file was deleted.

7 changes: 0 additions & 7 deletions ...ner/SwiftRustIntegrationTestRunner.xcodeproj/project.xcworkspace/contents.xcworkspacedata
View file
Open in desktop

This file was deleted.

8 changes: 0 additions & 8 deletions ...IntegrationTestRunner.xcodeproj/project.xcworkspace/xcshareddata/IDEWorkspaceChecks.plist
View file
Open in desktop

This file was deleted.

88 changes: 0 additions & 88 deletions ...rationTestRunner.xcodeproj/xcshareddata/xcschemes/SwiftRustIntegrationTestRunner.xcscheme
View file
Open in desktop

This file was deleted.

11 changes: 0 additions & 11 deletions ...tRunner/SwiftRustIntegrationTestRunner/Assets.xcassets/AccentColor.colorset/Contents.json
View file
Open in desktop

This file was deleted.

58 changes: 0 additions & 58 deletions ...estRunner/SwiftRustIntegrationTestRunner/Assets.xcassets/AppIcon.appiconset/Contents.json
View file
Open in desktop

This file was deleted.

6 changes: 0 additions & 6 deletions SwiftRustIntegrationTestRunner/SwiftRustIntegrationTestRunner/Assets.xcassets/Contents.json
View file
Open in desktop

This file was deleted.

21 changes: 0 additions & 21 deletions SwiftRustIntegrationTestRunner/SwiftRustIntegrationTestRunner/ContentView.swift
View file
Open in desktop

This file was deleted.

6 changes: 0 additions & 6 deletions ...nner/SwiftRustIntegrationTestRunner/Preview Content/Preview Assets.xcassets/Contents.json
View file
Open in desktop

This file was deleted.

10 changes: 0 additions & 10 deletions ...tionTestRunner/SwiftRustIntegrationTestRunner/SwiftRustIntegrationTestRunner.entitlements
View file
Open in desktop

This file was deleted.

17 changes: 0 additions & 17 deletions ...egrationTestRunner/SwiftRustIntegrationTestRunner/SwiftRustIntegrationTestRunnerApp.swift
View file
Open in desktop

This file was deleted.

17 changes: 0 additions & 17 deletions ...tIntegrationTestRunner/SwiftRustIntegrationTestRunnerSwiftRustIntegrationTestRunner.swift
View file
Open in desktop

This file was deleted.

21 changes: 0 additions & 21 deletions ...ustIntegrationTestRunner/SwiftRustIntegrationTestRunnerTests/ArgumentAttributesTest.swift
View file
Open in desktop

This file was deleted.

Loading