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

[NativeAOT] Fix System.Collections.Immutable.Tests #84552

Merged
merged 5 commits into from
Apr 11, 2023

Conversation

VSadov
Copy link
Member

@VSadov VSadov commented Apr 10, 2023

Seems like immutable collections tests are broken again,
at least when run locally.

Let's see if it is a local issue or the tests are indeed broken.

@ghost
Copy link

ghost commented Apr 10, 2023

Tagging subscribers to this area: @dotnet/area-system-collections
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author: VSadov
Assignees: VSadov
Labels:

area-System.Collections

Milestone: -

@VSadov
Copy link
Member Author

VSadov commented Apr 10, 2023

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ghost
Copy link

ghost commented Apr 10, 2023

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

Seems like frozen collections are broken again,
at least when run locally.

Author: VSadov
Assignees: VSadov
Labels:

area-System.Collections, area-NativeAOT-coreclr

Milestone: -

@EgorBo
Copy link
Member

EgorBo commented Apr 10, 2023

Seems to be a real issue?

@VSadov
Copy link
Member Author

VSadov commented Apr 10, 2023

perhaps a testcase issue with some incompatible use of reflection that requires adding more stuff to rd.xml

@VSadov
Copy link
Member Author

VSadov commented Apr 10, 2023

System.NotSupportedException : 'Xunit.Sdk.AssertEqualityComparer`1[System.Collections.Generic.HashSet`1[System.Collections.Generic.KeyValuePair`2[System.ValueTup
  le`2[System.Collections.Frozen.Tests.SimpleNonComparableStruct, System.Collections.Frozen.Tests.SimpleNonComparableStruct], System.Int32]]].CompareTypedSets[Syst
  em.Collections.Generic.KeyValuePair`2[System.ValueTuple`2[System.Collections.Frozen.Tests.SimpleNonComparableStruct, System.Collections.Frozen.Tests.SimpleNonCom
  parableStruct], System.Int32]](System.Collections.IEnumerable,System.Collections.IEnumerable)' is missing native code. MethodInfo.MakeGenericMethod() is not comp
  atible with AOT compilation. Inspect and fix AOT related warnings that were generated when the app was published. For more information see https://aka.ms/nativea
  ot-compatibility

@VSadov VSadov changed the title [NativeAOT] no-op change to see if tests are passing [NativeAOT] Fix System.Collections.Immutable.Tests Apr 10, 2023
@VSadov
Copy link
Member Author

VSadov commented Apr 10, 2023

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@VSadov
Copy link
Member Author

VSadov commented Apr 11, 2023

There are errors on windows in Microsoft.Extensions.Hosting.WindowsServiceLifetimeTests that look like:


Error message
System.PlatformNotSupportedException : RemoteExecutor is not supported on this platform.


Stack trace
   at Microsoft.DotNet.RemoteExecutor.RemoteExecutor.Invoke(MethodInfo method, String[] args, RemoteInvokeOptions options, Boolean pasteArguments) + 0x4b1
   at Microsoft.Extensions.Hosting.WindowsServiceLifetimeTests.ExceptionOnStopIsPropagated() + 0x73
   at Microsoft.Extensions.Hosting.WindowsServices.Tests!<BaseAddress>+0x85e2f4
   at System.Reflection.DynamicInvokeInfo.Invoke(Object, IntPtr, Object[], BinderBundle, Boolean) + 0xe6

@MichalStrehovsky - is that supposed to work?

@MichalStrehovsky
Copy link
Member

@MichalStrehovsky - is that supposed to work?

We don't currently support it. It is doable, the work just wasn't done. The test should be conditioned on RemoteExecutor being supported, like this:

[ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]

Maybe we should start running a NativeAot leg on all PRs to this repo. We can now run a leg in 90 minutes or so. It's starting to be somewhat annoying that we need to mop up these things pretty much every week. Or if we could trigger it on any libs test change (not sure if that makes a meaningful difference). @agocke do you have an opinion?

@VSadov
Copy link
Member Author

VSadov commented Apr 11, 2023

We don't currently support it. It is doable, the work just wasn't done.

I think that will have little added value, so should not be a priority.

Copy link
Member

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

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

Thank you!

@VSadov
Copy link
Member Author

VSadov commented Apr 11, 2023

Maybe we should start running a NativeAot leg on all PRs to this repo. We can now run a leg in 90 minutes or so. It's starting to be somewhat annoying that we need to mop up these things pretty much every week.

Yes. Every time these tests need to run it starts with fixing all kind of issues.
I think running the fastest combination could be useful.

I think that could be osx-arm64, since that does not run in a tiny VM.

I've just checked and ./build clr+libs+libs.tests -rc Checked -lc Release /p:TestNativeAot=true -test takes ~40min from a clean state on a fairly standard M1 with 16Gb memory.

@VSadov
Copy link
Member Author

VSadov commented Apr 11, 2023

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@VSadov
Copy link
Member Author

VSadov commented Apr 11, 2023

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MichalStrehovsky MichalStrehovsky merged commit a6a5dfb into dotnet:main Apr 11, 2023
@VSadov VSadov deleted the fixtests3 branch April 12, 2023 00:21
@VSadov
Copy link
Member Author

VSadov commented Apr 12, 2023

Thanks!!

@ghost ghost locked as resolved and limited conversation to collaborators May 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants