[NativeAot] Start running 8 more libs tests#73283
Conversation
|
Tagging subscribers to this area: @dotnet/area-meta Issue Detailsnull
|
|
/azp run runtime-extra-platforms |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| <Compile Include="$(CommonTestPath)System\RandomDataGenerator.cs" | ||
| Link="Common\System\RandomDataGenerator.cs" /> | ||
| </ItemGroup> | ||
| <ItemGroup> |
There was a problem hiding this comment.
As you suggested, I had to add an rd.xml file to preserve UnicodeCategory for the other Globalization project test and it looks like there are similar failures with this project
| if (string.Empty.Length > 0) | ||
| limiterType = Type.GetType("System.Threading.RateLimiting.DefaultPartitionedRateLimiter`2, System.Threading.RateLimiting"); |
There was a problem hiding this comment.
| if (string.Empty.Length > 0) | |
| limiterType = Type.GetType("System.Threading.RateLimiting.DefaultPartitionedRateLimiter`2, System.Threading.RateLimiting"); | |
| var expectedLimiterType = Type.GetType("System.Threading.RateLimiting.DefaultPartitionedRateLimiter`2, System.Threading.RateLimiting"); | |
| Assert.Equal(expectedLimiterType, limiterType); |
We can validate the assumptions as part of the test.
There was a problem hiding this comment.
That would still not fix the dataflow analysis because the analysis doesn't understand limiterType is now same as expectedLimiterType. I'm becoming allergic to these kinds of tests because it has been the same issues for 3 weeks now.
There was a problem hiding this comment.
I understand that it would not help dataflow analysis, but it would eagerly catch cases where somebody renames the type that is used as internal implementation detail.
|
/azp run runtime-extra-platforms |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| } | ||
|
|
||
| [Theory, MemberData(nameof(KeyComponents))] | ||
| [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotBuiltWithAggressiveTrimming))] |
There was a problem hiding this comment.
It would be better to replace the insane expensive way to get the type using Linq by just calling GetType.
There was a problem hiding this comment.
There's also dynamic keyword below that wasn't obvious to me why it was added. I assume there's a reason and not just whoever wrote it doesn't know what they're doing.
There was a problem hiding this comment.
I assume there's a reason and not just whoever wrote it doesn't know what they're doing.
I do not think there is a reason. This whole method was clearly written by somebody who did not know what they were doing.
|
/azp run runtime-extra-platforms |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-extra-platforms |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-extra-platforms |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| } | ||
|
|
||
| [Fact] | ||
| [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] |
There was a problem hiding this comment.
Feel free to cherry-pick that commit, or I can rebase my PR if this gets merged.
There was a problem hiding this comment.
Thanks! Merged your change and I'll back out this part.
|
/azp run runtime-extra-platforms |
|
Azure Pipelines successfully started running 1 pipeline(s). |
ghost
locked as resolved and limited conversation to collaborators
5 participants
No description provided.