Expose new .netcoreapp1.1 APIs in System.Runtime#12226
Expose new .netcoreapp1.1 APIs in System.Runtime#12226weshaggard merged 5 commits intodotnet:masterfrom hughbe:runtime-expose-11
Conversation
|
@yufeih I think there might be a bug in your implementation of the non-generic |
|
I followed the behavior of TryParse The following code throws ArgumentException: |
|
Ah okay, that's fine then. Was just checking! |
|
@dotnet-bot test this please (CI down) |
| yield return new object[] { "99", false, (SimpleEnum)99 }; | ||
| yield return new object[] { "-42", false, (SimpleEnum)(-42) }; | ||
| yield return new object[] { " -42", false, (SimpleEnum)(-42) }; | ||
| yield return new object[] { " -42 ", false, (SimpleEnum)(-42) }; |
There was a problem hiding this comment.
Is there a reason to prefer this approach over the InlineData approach? I don't have a strong preference I'm just trying to understand the motivation
There was a problem hiding this comment.
Nevermind I see now you did this so you can share the test cases.
| <Compile Include="Helpers.cs" /> | ||
| <Compile Include="System\ActivatorTests.cs" /> | ||
| <Compile Include="System\ArrayTests.cs" /> | ||
| <Compile Include="System\ArrayTests.netcoreapp1.1.cs" Condition="'$(TargetGroup)' == 'netcoreapp1.1'" /> |
There was a problem hiding this comment.
I would prefer these all being grouped in one ItemGroup with that ItemGroup having the condition.
|
Looks like some of the System.Runtime tests are hitting some kind of interesting infinite loop or something in Release builds for your new set of tests. Please try to take a look at a Release test run for your new configuration. |
And add some tests
|
Thanks Wes, turns out it was a bad test - the release CLR inlined a method, meaning that it never reached stack limit. Fixed by adding a |
| }, | ||
| "netcoreapp1.1": { | ||
| "dependencies": { | ||
| "System.Runtime.Serialization.Formatters": "4.3.0-beta-24530-03" |
There was a problem hiding this comment.
What test actually has a dependency on this library?
There was a problem hiding this comment.
I'm afk right now so can't remove the dependency and see what fails, but it was introduced in f6f97c2#diff-29df68f37e76e464ad7ef9130076abae
There was a problem hiding this comment.
Thanks for the pointer looks like @alexperovich added it to do some BinaryFormatter testing. I'm OK with this for testing purposes.
weshaggard
left a comment
weshaggard
left a comment
There was a problem hiding this comment.
LGTM - Thanks for exposing these.
| } | ||
|
|
||
| [Fact] | ||
| public static void TryEnsureSufficientExecutionStack_NoSpaceAvaiable_ReturnsFalse() |
There was a problem hiding this comment.
Small typo in Available.
| if (!RuntimeHelpers.TryEnsureSufficientExecutionStack()) | ||
| { | ||
| Assert.Throws<InsufficientExecutionStackException>(() => RuntimeHelpers.EnsureSufficientExecutionStack()); | ||
| return; |
There was a problem hiding this comment.
Is this case ever hit? I.e. do we construct a deep stack in some test to assert this behavior?
There was a problem hiding this comment.
Yes. If I remove this check, a StackOverflow exception is thrown - the lines below your comment recurse the method
…'t recurse into olivion.
weshaggard
left a comment
weshaggard
left a comment
There was a problem hiding this comment.
LGTM Thanks @hughbe for your contribution.
* Expose new .netcoreapp1.1 APIs in System.Runtime And add some tests Sometimes, CLR optimizations make this test invalid. Make sure we don't recurse into oblivion. Commit migrated from dotnet/corefx@7bc1e6d
6 participants
And add some tests
Fixes #10127
Fixes #8592
Fixes #2352
Fixes #692
Fixes #11090
/cc @justinvp @jamesqo @jkotas @yufeih @bartdesmet @weshaggard @danmosemsft