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

[SwiftBindings] Improve testing framework #2871

Open
wants to merge 16 commits into
base: feature/swift-bindings
Choose a base branch
from

Conversation

kotlarmilos
Copy link
Member

Description

This PR introduces the following test improvements:

  • Unit tests: Tests for components of the projection tooling.
  • Integration tests: Tests that compile Swift code and invoke the projection tooling to generate bindings on macOS.
  • Framework tests (TBD): Tests that consume bindings as a NuGet package, build tests/apps, and run on target platforms.

This change improves debugging by referencing generated bindings within the integration tests. Additionally, it removes need for reflection in tests. On non-macOS platforms, only the tooling is built, and tests are skipped.

The objective is to set up the pipeline for StoreKit2 and SwiftUI, and begin tracking bindings coverage using the projection tooling, which will be introduced in a follow-up PR.

@kotlarmilos kotlarmilos added the area-SwiftBindings Swift bindings for .NET label Dec 11, 2024
@kotlarmilos kotlarmilos self-assigned this Dec 11, 2024
Copy link

@stephen-hawley stephen-hawley left a comment

Choose a reason for hiding this comment

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

I'd like to see this branch get migrated to use TypeMetadata instead of void * and be ready to use the upcoming metadata access method in TypeMetadata. Like what I'm seeing.

@@ -182,11 +182,12 @@ public static void GenerateBindings(Queue<string> paths, string outputDirectory,
}

Choose a reason for hiding this comment

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

You might want:

var libraryDirectory = Path.Combine(Directory.GetParent(outputDirectory)!.FullName, "Library");
Directory.CreateDirectory(libraryDirectory);
...
var destFilePath = Path.Combine(libraryDirectory, fileName);

Factoring it out eliminates redundant string allocation/concatenation.

Also, if you do this:

var srcDirInfo = new DirectoryInfo(Path.Combine(AppDomain.CurrentDomain.BaseDirectory, "Library"));
foreach (var fileInfo in srcDirInfo.GetFiles()) { // returns FileInfo
    file.CopyTo(Path.Combine(libraryDirectory, file.Name));
}

It's a little tighter.

@@ -20,7 +20,7 @@ public unsafe struct ChaChaPoly
/// </summary>
public sealed unsafe class Nonce : IDisposable, ISwiftObject
{
private static nuint PayloadSize = (nuint)((Runtime.ValueWitnessTable*)Test.Runtime.GetValueWitnessTable(Metadata))->Size;
private static nuint PayloadSize = (nuint)((Runtime.ValueWitnessTable*)Runtime.GetValueWitnessTable(Metadata))->Size;

Choose a reason for hiding this comment

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

Heads up that this is going to change to use TypeMetadata.Size

Copy link
Member Author

Choose a reason for hiding this comment

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

Replaced with Metadata.ValueWitnessTable->Size.

@@ -38,7 +38,7 @@ public Nonce(Data data)
_payload = NativeMemory.Alloc(PayloadSize);
SwiftIndirectResult swiftIndirectResult = new SwiftIndirectResult(_payload);

void* metadata = Test.Runtime.GetMetadata(data);
void* metadata = Runtime.GetMetadata(data);

Choose a reason for hiding this comment

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

The signature should (eventually) be TypeMetadata instead of void*

Copy link
Member Author

Choose a reason for hiding this comment

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

Replaced with TypeMetadata. We will automate generation of bindings when more support is available.


namespace BindingsGeneration.Tests
{
public class SmokeTests : IClassFixture<SmokeTests.TestFixture>
Copy link
Member

@jkurdek jkurdek Dec 12, 2024

Choose a reason for hiding this comment

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

I would see it the other way around. Lets consider some example lets say simple frozen structs. I think the test setup should look like this:

  1. We have a set of "integration" unit tests - basically small pieces of swift code which we project into c# and verify that they work. So for frozen structs we can have e.g.. "CallsInstanceMethodOnFrozenStruct" or "ConstructsNewNonFrozenStruct" etc.
  2. Then for bunch of features (lets say all structs code) we have some stress test where we try to generate all the possible interactions of those in order to verify we have not missed some scenarios. Those should not be bound to "FrozenStructs" category, but rather operate on broader scope to test more interactions
  3. Framework tests - where we apply the projections to real examples

So to sum up. I think that we should split up the tests which you called "SmokeTests" into multiple feature streams and look at them as part of DoD (basically treat them like unit tests), and then consider stress tests as thing testing a large group of features.

Copy link
Member Author

@kotlarmilos kotlarmilos Dec 12, 2024

Choose a reason for hiding this comment

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

We have a set of "integration" unit tests - basically small pieces of swift code which we project into c# and verify that they work. So for frozen structs we can have e.g.. "CallsInstanceMethodOnFrozenStruct" or "ConstructsNewNonFrozenStruct" etc.
Then for bunch of features (lets say all structs code) we have some stress test where we try to generate all the possible interactions of those in order to verify we have not missed some scenarios. Those should not be bound to "FrozenStructs" category, but rather operate on broader scope to test more interactions

We can group "integration" tests as needed, but they all require a Swift source code, which is why I categorized them as "integration" tests. These tests can target different levels of granularity, but they all involve running the tooling to generate bindings. I agree that stress tests should be broader and will evolve as more support is introduced.

Unit tests should focus on projection tooling components, such as the demangler/ABI parser. Ideally, they shoudn't depend on Swift code (mock inputs where necessary). Framework tests are straightforward.

Do you want to organize the "integration" tests differently ("functional" tests for simple interactions and "stress" tests for a language feature)? This would include:

  • Unit tests for projection tooling components
  • Integration -> Functional tests for simple interactions
  • Integration -> Stress tests for entire language features (such as struct, enum, etc)
  • Framework -> E2E (CryptoKit, StoreKit, etc)

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I wasnt precise. I agree that things living in "UnitTests" should not depend of Swift Code. But this means that UnitTests will most likely not test projections in any way, except for some Swift.Runtime types)

So talking strictly about stuff which lives in IntegrationTests space. I see it like this

integration tests
├── functional tests
│   ├── StructTests
│   ├── FuncTests
│   └── etc
├── stress tests
│   └── StructsStressTests \\ or even broader
└── framework tests

where functional tests will contain (mostly) hand-written tests which should cover (ideally all) use cases. Whereas stress test can be source generated and serve as a safety net to catch hard to spot bugs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I wasnt precise. I agree that things living in "UnitTests" should not depend of Swift Code. But this means that UnitTests will most likely not test projections in any way, except for some Swift.Runtime types)

Correct, it will not test projections but the projection tooling components.

So talking strictly about stuff which lives in IntegrationTests space. I see it like this

Sounds good -- Let's reflect it in the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-SwiftBindings Swift bindings for .NET
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants