Introduce a new SPM-based test harness #307

colinmarc · 2025-01-26T23:09:37Z

This is currently pretty hacky, and also doesn't actually compile on my machine because Swift 6 introduced a bunch of concurrency checks.

colinmarc · 2025-01-26T23:11:32Z

test-swift-rust-integration.sh

+
+# Create a swift file with the generated swift code and an import for the
+# generated C code. This is necessary because SPM projects don't support
+# mixed-source - the C code has to be a separate SPM target.


This is something that should be addressed upstream before this goes any farther. Basically, unlike, xcode, SPM doesn't allow mixed-language modules - you can't put .swift and .h files in the same target. Right now, the generation code assumes that all the swift and C code will coexist in the same namespace.

Ah. Can you open an issue about this and link to it from here?

Right now, the generation code assumes that all the swift and C code will coexist in the same namespace.

In the issue can you list out some of the ways that we make this assumption so that it's more clear what might need to change?

#315. I think taking the approach I outlined there would address a lot of the issues here, for example the hardcoded output path in build.rs.

This reverts commit c45a38c.

colinmarc · 2025-01-27T13:53:48Z

With #312 and #310, this now runs to completion! 🎉

[33/33] Linking IntegrationTestsPackageTests.xctest
Build complete! (4.24s)
Test Suite 'All tests' started at 2025-01-27 14:53:36.569
Test Suite 'debug.xctest' started at 2025-01-27 14:53:36.570
Test Suite 'PrimitiveTests' started at 2025-01-27 14:53:36.570
Test Case 'PrimitiveTests.testRustCallsSwiftPrimitives' started at 2025-01-27 14:53:36.570
Test Case 'PrimitiveTests.testRustCallsSwiftPrimitives' passed (0.0 seconds)
Test Case 'PrimitiveTests.testSwiftCallsRustPrimitives' started at 2025-01-27 14:53:36.570
Test Case 'PrimitiveTests.testSwiftCallsRustPrimitives' passed (0.1 seconds)
Test Suite 'PrimitiveTests' passed at 2025-01-27 14:53:36.670
	 Executed 2 tests, with 0 failures (0 unexpected) in 0.101 (0.101) seconds
Test Suite 'debug.xctest' passed at 2025-01-27 14:53:36.670
	 Executed 2 tests, with 0 failures (0 unexpected) in 0.101 (0.101) seconds
Test Suite 'All tests' passed at 2025-01-27 14:53:36.670
	 Executed 2 tests, with 0 failures (0 unexpected) in 0.101 (0.101) seconds
◇ Test run started.
↳ Testing Library Version: 6.0.3 (18c42c19cac3faf)
✔ Test run with 0 tests passed after 0.001 seconds.

Using the Swift Package Manager instead of xcode allows for developing on Linux and Windows, and could potentially make life easier. Fixes chinedufn#306

colinmarc · 2025-01-27T19:39:10Z

integration-tests/Tests/IntegrationTests/PrimitiveTests.swift

@@ -0,0 +1,28 @@
+import RustLib


This is just a reformat of the existing test, plus this import RustLib was added. If you'd like me to take a stab at porting it to the new Swift Testing framework, let me know.

Here's a guide for doing that: https://developer.apple.com/documentation/testing/migratingfromxctest

I get the impression that Apple favors Swift testing over XCodeTest.

So, yeah, good idea, let's start by using Swift Testing from the beginning.

chinedufn · 2025-01-30T01:34:09Z

Thanks for working on all of this stuff. I haven't looked yet.
My current plan is to to review this and your other issues either Thurs or Fri.

chinedufn

Awesome work here.
Excited for it to be possible for Linux users to contribute to the project.

Left some mostly minor feedback and questions.

chinedufn · 2025-01-31T10:50:04Z

integration-tests/Tests/IntegrationTests/PrimitiveTests.swift

@@ -0,0 +1,28 @@
+import RustLib


I get the impression that Apple favors Swift testing over XCodeTest.

So, yeah, good idea, let's start by using Swift Testing from the beginning.

chinedufn · 2025-01-31T10:51:23Z

test-swift-rust-integration.sh

-
-# Integration tests between Swift and Rust
-
-set -e


Why was this set -e removed?

It was there to ensure that we return with a non-zero exit code if any commands in this file.

I just moved it up to the shebang out of habit. Happy to restore it if you prefer this idiom.

Oh I didn't notice/know about that. Ok moving it up is fine.

chinedufn · 2025-01-31T10:56:18Z

test-swift-rust-integration.sh

+# Run the swift package tests. Note that we have to instruct swift where to look
+# for the rust static lib.


Mention where this instruction lives.

Oh, it was in this line of code, and then I moved it inside Package.swift.

Got it. What I meant was "Make the comment mention where the instruction lives so that the reader knows where to jump to next if they're trying to step through how this all works".

chinedufn · 2025-01-31T10:58:59Z

test-swift-rust-integration.sh

+
+# Create a swift file with the generated swift code and an import for the
+# generated C code. This is necessary because SPM projects don't support
+# mixed-source - the C code has to be a separate SPM target.


Ah. Can you open an issue about this and link to it from here?

Right now, the generation code assumes that all the swift and C code will coexist in the same namespace.

In the issue can you list out some of the ways that we make this assumption so that it's more clear what might need to change?

chinedufn · 2025-01-31T11:01:42Z

integration-tests/Tests/IntegrationTests/PrimitiveTests.swift

+
+@testable import SharedLib
+
+/// Tests for generic types such as `type SomeType<u32>`


Awesome. Let's add a README.md file to this integration-tests dir.

It should include:

instructions on how to run this integration-tests test suite

a note that old tests are being gradually ported here, such that it becomes clearer to new contributors that their new tests should go in this dir and not in the old Xcode integration test dir

anything else that you think would be useful for maintainers to know

chinedufn · 2025-01-31T11:25:24Z

crates/swift-bridge-ir/src/codegen/codegen_tests/result_codegen_tests.rs

@@ -4,7 +4,7 @@ use super::{CodegenTest, ExpectedCHeader, ExpectedRustTokens, ExpectedSwiftCode}
 use proc_macro2::TokenStream;
 use quote::quote;

-/// Test code generation for Rust function that accepts a Result<T, E>


This test module doesn't accept and return a Result, it only accepts a Result.

This is part of the revert in #310, which I rebased in so that it would compile.

chinedufn · 2025-01-31T11:25:30Z

crates/swift-bridge-ir/src/codegen/codegen_tests/result_codegen_tests.rs

@@ -64,72 +64,6 @@ void __swift_bridge__$some_function(struct __private__ResultPtrAndPtr arg);
    }
 }

-/// Test code generation for Rust function that returns a Result<T, E>


Why was this removed?

This is part of #310.

chinedufn · 2025-01-31T11:28:21Z

Cargo.toml

-  "SwiftRustIntegrationTestRunner/integration-test-create-swift-package",
-  "SwiftRustIntegrationTestRunner/swift-package-rust-library-fixture",


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

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.

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.

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).

chinedufn · 2025-01-31T11:28:57Z

SwiftRustIntegrationTestRunner/Headers/BridgingHeader.h

@@ -1,14 +0,0 @@
-//


Let's restore this SwiftRustIntegrationTestRunner directory, and add a README.md that explains that we are gradually migrating tests over to the integration-tests dir.

chinedufn · 2025-01-31T11:33:41Z

integration-tests/Tests/IntegrationTests/PrimitiveTests.swift

@@ -0,0 +1,28 @@
+import RustLib


Let's add a GitHub actions job that runs these integration-tests.

chinedufn · 2025-01-31T11:36:53Z

Please update the README to include more information so that:

when I rebase these commits I can use the PR body as the commit message
contributors can more quickly look at a PR and understand 60%+ of what it is doing / why it exists / the motivations
when writing release notes we can more quickly find the important details to communicate

I left notes about this here -> #310 (comment)

chinedufn · 2025-01-31T11:38:27Z

I left my review as if this PR was ready and wasn't a draft, but just a heads up that I recognize that this was submitted as a draft so if I may have commented on stuff that you were already going to change.

Figured that since the PR was small it was easier to make sure we were both on the same page vs. making assumptions about what was finished and what wasn't.

colinmarc commented Jan 26, 2025

View reviewed changes

colinmarc mentioned this pull request Jan 26, 2025

SPM setup for rust library #280

Open

colinmarc added 2 commits January 27, 2025 14:18

Revert "Support Rust returning -> Result<_, String> (chinedufn#296)"

db73038

This reverts commit c45a38c.

Add Sendable to all async return types in tests

5c3d672

colinmarc force-pushed the spm-test-harness branch from 681285a to 3e62a31 Compare January 27, 2025 13:52

colinmarc force-pushed the spm-test-harness branch 3 times, most recently from 0edc00b to 8b77575 Compare January 27, 2025 19:36

Introduce a new SPM-based test harness

5aaf654

Using the Swift Package Manager instead of xcode allows for developing on Linux and Windows, and could potentially make life easier. Fixes chinedufn#306

colinmarc force-pushed the spm-test-harness branch from 8b77575 to 5aaf654 Compare January 27, 2025 19:37

colinmarc commented Jan 27, 2025

View reviewed changes

chinedufn reviewed Jan 31, 2025

View reviewed changes

chinedufn mentioned this pull request Jan 31, 2025

Suggestion: use server-side swift to run integration tests #306

Open

chinedufn linked an issue Jan 31, 2025 that may be closed by this pull request

Suggestion: use server-side swift to run integration tests #306

Open

This was referenced Feb 2, 2025

Creating SPM packages from swift-bridge crates is error-prone #315

Open

Remove Error and Sendable implementations for RustStringRef #309

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce a new SPM-based test harness #307

Introduce a new SPM-based test harness #307

colinmarc commented Jan 26, 2025

colinmarc Jan 26, 2025

chinedufn Jan 31, 2025

colinmarc Feb 2, 2025

colinmarc commented Jan 27, 2025

colinmarc Jan 27, 2025 •

edited

Loading

chinedufn Jan 31, 2025

chinedufn commented Jan 30, 2025

chinedufn left a comment

chinedufn Jan 31, 2025

chinedufn Jan 31, 2025

colinmarc Feb 2, 2025

chinedufn Feb 2, 2025

chinedufn Jan 31, 2025

colinmarc Feb 2, 2025

chinedufn Feb 2, 2025

chinedufn Jan 31, 2025

chinedufn Jan 31, 2025

chinedufn Jan 31, 2025

colinmarc Feb 2, 2025

chinedufn Jan 31, 2025

colinmarc Feb 2, 2025

chinedufn Jan 31, 2025

colinmarc Feb 2, 2025

chinedufn Feb 2, 2025

colinmarc Feb 2, 2025

chinedufn Jan 31, 2025

chinedufn Jan 31, 2025

colinmarc Feb 2, 2025

chinedufn commented Jan 31, 2025

chinedufn commented Jan 31, 2025

		# Run the swift package tests. Note that we have to instruct swift where to look
		# for the rust static lib.


		@testable import SharedLib

		/// Tests for generic types such as `type SomeType<u32>`

		"SwiftRustIntegrationTestRunner/integration-test-create-swift-package",
		"SwiftRustIntegrationTestRunner/swift-package-rust-library-fixture",

Introduce a new SPM-based test harness #307

Are you sure you want to change the base?

Introduce a new SPM-based test harness #307

Conversation

colinmarc commented Jan 26, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colinmarc commented Jan 27, 2025

colinmarc Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chinedufn commented Jan 30, 2025

chinedufn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chinedufn commented Jan 31, 2025

chinedufn commented Jan 31, 2025

colinmarc Jan 27, 2025 •

edited

Loading