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

Adapt monotouch-test to work with NativeAOT #17774

Closed
Tracked by #80905 ...
ivanpovazan opened this issue Mar 13, 2023 · 10 comments · Fixed by xamarin/Touch.Unit#116
Closed
Tracked by #80905 ...

Adapt monotouch-test to work with NativeAOT #17774

ivanpovazan opened this issue Mar 13, 2023 · 10 comments · Fixed by xamarin/Touch.Unit#116
Labels
tests Anything related to tests
Milestone

Comments

@ivanpovazan
Copy link
Contributor

Description

During the startup, monotouch-test app collects the assemblies which represent test suites and calls NUnitTestAssemblyRunner to bundle each of them in a TestSuite object.
This happens in TouchRunner::GetViewController through LoadSync method.

The following code path is followed:

https://github.com/spouliot/Touch.Unit/blob/564433f35c8ab6b7bb0709f83e1b81a89c406956/NUnitLite/TouchRunner/TouchRunner.cs#L168-L170

https://github.com/spouliot/Touch.Unit/blob/564433f35c8ab6b7bb0709f83e1b81a89c406956/NUnitLite/TouchRunner/TouchRunner.cs#L608C4

https://github.com/nunit/nunit/blob/af8ca8bc779072b4fa5432e8620f06ca796d13a3/src/NUnitFramework/framework/Api/NUnitTestAssemblyRunner.cs#L170

https://github.com/nunit/nunit/blob/af8ca8bc779072b4fa5432e8620f06ca796d13a3/src/NUnitFramework/framework/Api/DefaultTestAssemblyBuilder.cs#L81

https://github.com/nunit/nunit/blob/af8ca8bc779072b4fa5432e8620f06ca796d13a3/src/NUnitFramework/framework/Internal/AssemblyHelper.cs#L48-L56

Finally, the final piece ends up calling CodeBase property on Assembly type which is not supported with NativeAOT:
https://github.com/dotnet/runtime/blob/588dcd7a29f109daae2d8c999322acd8e853a8c6/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/General/ThunkedApis.cs#L70-L78

Proposal

/cc: @rolfbjarne

@rolfbjarne
Copy link
Member

@rolfbjarne rolfbjarne added this to the .NET 8 milestone Mar 13, 2023
@rolfbjarne rolfbjarne added the tests Anything related to tests label Mar 13, 2023
@ivanpovazan
Copy link
Contributor Author

ivanpovazan commented Mar 14, 2023

I tried this:

return AddSuite ((TestSuite) runner.Load (assembly.Location, CreateSettings (settings)));

However, unfortunately, it also leads to a crash/unsupported features.

In general, notions of Location/CodeBase do not seem to make sense with NativeAOT as the assembly is not preserved in its original format.

PS I would be happy to contribute to solving this.

@rolfbjarne
Copy link
Member

Next attempt: try calling this method using reflection (since it's private):

https://github.com/nunit/nunit/blob/af8ca8bc779072b4fa5432e8620f06ca796d13a3/src/NUnitFramework/framework/Api/DefaultTestAssemblyBuilder.cs#L117

NUnit's code flow is rather annoying, because the only reason it seems to want location/codebase is to figure out the assembly name, and then it also reloads the assembly when it's already been given the assembly. Calling that method might skip over that logic, and go straight to the actual important bits. If that doesn't work, maybe we'll have to implement a custom ITestAssemblyBuilder.

PS I would be happy to contribute to solving this.

I guess one way would be to just try to make NUnit work with NativeAOT, and get it fixed in NUnit instead...

@ivanpovazan
Copy link
Contributor Author

ivanpovazan commented Mar 14, 2023

According to your suggestion of using a different overload, would it make sense to do this instead of using reflection?

public bool Load (Assembly assembly, IDictionary<string, object> settings = null)
{
        var runner = new NUnitTestAssemblyRunner (builder);
        runners.Add (runner);
+#if NATIVEAOT
+       return AddSuite ((TestSuite) runner.Load (assembly.GetName().Name, CreateSettings (settings)));
+#else
        return AddSuite ((TestSuite) runner.Load (assembly, CreateSettings (settings)));
+#endif
}

I agree it is hacky, but seems to work as NUnit will recreate the FullName based on already available name...
If you think this is not acceptable, I can try to use reflection and/or look into how to contribute to NUnit framework instead.

@rolfbjarne
Copy link
Member

According to your suggestion of using a different overload, would it make sense to do this instead of using reflection?

public bool Load (Assembly assembly, IDictionary<string, object> settings = null)
{
        var runner = new NUnitTestAssemblyRunner (builder);
        runners.Add (runner);
+#if NATIVEAOT
+       return AddSuite ((TestSuite) runner.Load (assembly.GetName().Name, CreateSettings (settings)));
+#else
        return AddSuite ((TestSuite) runner.Load (assembly, CreateSettings (settings)));
+#endif
}

I agree it is hacky, but seems to work as NUnit will recreate the FullName based on already available name... If you think this is not acceptable, I can try to use reflection and/or look into how to contribute to NUnit framework instead.

Won't that run into the same issue as you describe here: #17774 (comment)?

In any case, I'm fine with this solution if it works (even better if the NATIVEAOT codepath works when !NATIVEAOT, then we can just use that always).

@ivanpovazan
Copy link
Contributor Author

ivanpovazan commented Mar 15, 2023

Won't that run into the same issue as you describe here: #17774 (comment)?

The difference is that with the approach above:

return AddSuite ((TestSuite) runner.Load (assembly.Location, CreateSettings (settings)));

Location returns an empty string causing FullName generation to fail when NUnit tries to build the assembly name.
While on the other hand, we can trick NUnit by providing a simple name of the assembly.

@rolfbjarne
Copy link
Member

Won't that run into the same issue as you describe here: #17774 (comment)?

The difference is that with the approach above, Location returns an empty string causing FullName generation to fail when NUnit tries to build the assembly name.

Ah ok - well, if that works, then great :)

@ivanpovazan
Copy link
Contributor Author

In any case, I'm fine with this solution if it works (even better if the NATIVEAOT codepath works when !NATIVEAOT, then we can just use that always).

Regarding this, I was wondering whether some test (or the infrastructure around it) cares about assembly name in the "Assembly.dll" format.

@rolfbjarne
Copy link
Member

In any case, I'm fine with this solution if it works (even better if the NATIVEAOT codepath works when !NATIVEAOT, then we can just use that always).

Regarding this, I was wondering whether some test (or the infrastructure around it) cares about assembly name in the "Assembly.dll" format.

There might be, but we can change those tests if need be.

rolfbjarne pushed a commit to xamarin/Touch.Unit that referenced this issue Mar 16, 2023
…name (#116)

This PR adds a workaround to support NativeAOT runtime with Xamarin.

### Introduction

The limitation comes from the unsupported or limited support for the following properties on `Assembly` type with NativeAOT
- `CodeBase` - unsupported feature - throws with: https://github.com/dotnet/runtime/blob/588dcd7a29f109daae2d8c999322acd8e853a8c6/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/General/ThunkedApis.cs#L70-L78
- `Location` - somewhat supported - empty string: https://github.com/dotnet/runtime/blob/588dcd7a29f109daae2d8c999322acd8e853a8c6/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/General/ThunkedApis.cs#L66

Both properties do not seem to make sense with NativeAOT, as the assembly is not preserved in its original format.
These limitations prevent using NUnitTestAssemblyRunner's Load overload which accepts `Assembly` object as parameter, which is discussed in the tracking issue: xamarin/xamarin-macios#17774

### Changes

The included change, changes the way the `TestSuite` is created by NUnit framework for all runtimes. 
This affects the created name of test suites in the following way e.g.,:
- `monotouchtest.dll` -> `monotouchtest`
- `EmbeddedResources.dll` -> `EmbeddedResources`
- ...etc

The change has been tested locally with monotouch-test running on iOS device using .NET7 mono runtime 

---

Fixes: xamarin/xamarin-macios#17774
@rolfbjarne
Copy link
Member

PR with the Touch.Unit bump: #17819

@ghost ghost locked as resolved and limited conversation to collaborators Apr 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
tests Anything related to tests
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants