Migrate host.tests from Newtonsoft to STJ by am11 · Pull Request #76901 · dotnet/runtime

am11 · 2022-10-11T19:57:41Z

Also updated FluentAssertions and Moq packages to latest versions (which are available in internal nuget feed).

ghost · 2022-10-11T19:57:47Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

am11 · 2022-10-11T20:03:39Z

Newtonsoft PackageReference will still be there in assets, as some tests publish the project and verify that the types from external dependencies are loaded.

am11 · 2022-10-12T19:29:04Z

src/installer/tests/TestUtils/RuntimeConfig.cs

+            internal static Framework FromJson(JsonObject jobject)
            {
-                return new Framework((string)jobject["name"], (string)jobject["version"])
+                return new Framework(jobject["name"].ToString(), jobject["version"].ToString())


@eiriktsarpalis, the reason for changing this line was; given these two files:

{ "name": "foo" }

{ "name": false }

first one successfully casts to string but second one throws an exception about 'False' type conversion to System.String.

If this is not an intentional decision then IMO, we should allow explicit cast to string on JsonNode for all JSON types.

Yes, this is by design -- Any coercions from bool to string will need to be performed explicitly by the user.

eerhardt

DependencyModel tests LGTM

elinor-fung · 2022-10-13T20:55:51Z

src/installer/tests/TestUtils/Assertions/CommandResultAssertions.cs

        {
            Execute.Assertion.ForCondition(Result.ExitCode == expectedExitCode)
-                .FailWithPreformatted($"Expected command to exit with {expectedExitCode} but it did not.{GetDiagnosticsInfo()}");
+                .FailWith($"Expected command to exit with {expectedExitCode} but it did not.{GetDiagnosticsInfo()}");


I thought we had to explicitly use our extension and AddFailure (or I guess AddPreFormattedFailure in newer versions) because FailWith would always go through the the FluentAssertions formatter which messed with things like newlines. Does the new version not do that anymore?

There were a lot of API changes between v4 and the latest v6. AssertionScope.Succeeded etc. are not available on public surface anymore.

FailWith would always go through the the FluentAssertions formatter which messed with things like newlines.

If this is still a problem, we can change the behavior with AssertionOptions.FormattingOptions.UseLineBreaks = true (or fluent API .UsingLineBreaks).

elinor-fung

Thanks!

am11 · 2022-10-20T13:28:54Z

Ready to merge? :)

cc @ilonatommy, in case we want to drop Newtonsoft.Json dependency in favor of shared framework's System.Text.Json from BrowserDebugProxy, we can use this as reference for migration.

elinor-fung · 2022-10-20T15:54:44Z

Ah, thanks for the ping.

Are the wasm failures pre-existing/known? cc @lewing @radical
They all seem to be from hitting IndexOutOfRangeException when using mocks, so I'd like to check that it is not from moving to the newer Moq package.

From https://dev.azure.com/dnceng-public/public/_build/results?buildId=49477&view=ms.vss-test-web.build-test-results-tab&runId=999620&resultId=127840&paneView=debug:

[FAIL] System.ComponentModel.Tests.TypeDescriptionProviderTests.IsSupportedType_NullTypeWithParent_ThrowsArgumentNullException
System.IndexOutOfRangeException : Index was outside the bounds of the array.
   at Castle.DynamicProxy.Generators.MethodWithInvocationGenerator.BuildProxiedMethodBody(MethodEmitter , ClassEmitter , INamingScope )
   at Castle.DynamicProxy.Generators.MethodGenerator.Generate(ClassEmitter , INamingScope )
   at Castle.DynamicProxy.Contributors.CompositeTypeContributor.ImplementMethod(MetaMethod , ClassEmitter , OverrideMethodDelegate )
   at Castle.DynamicProxy.Contributors.CompositeTypeContributor.ImplementProperty(ClassEmitter , MetaProperty )
   at Castle.DynamicProxy.Contributors.CompositeTypeContributor.Generate(ClassEmitter )
   at Castle.DynamicProxy.Generators.BaseClassProxyGenerator.GenerateType(String , INamingScope )
   at Castle.DynamicProxy.Generators.BaseProxyGenerator.<>c__DisplayClass13_0.<GetProxyType>b__0(CacheKey )
   at Castle.Core.Internal.SynchronizedDictionary`2[[Castle.DynamicProxy.Generators.CacheKey, Castle.Core, Version=5.0.0.0, Culture=neutral, PublicKeyToken=407dd0808d44fbdc],[System.Type, System.Private.CoreLib, Version=7.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].GetOrAdd(CacheKey , Func`2 )
   at Castle.DynamicProxy.Generators.BaseProxyGenerator.GetProxyType()
   at Castle.DynamicProxy.DefaultProxyBuilder.CreateClassProxyType(Type , Type[] , ProxyGenerationOptions )
   at Castle.DynamicProxy.ProxyGenerator.CreateClassProxyType(Type , Type[] , ProxyGenerationOptions )
   at Castle.DynamicProxy.ProxyGenerator.CreateClassProxy(Type , Type[] , ProxyGenerationOptions , Object[] , IInterceptor[] )
   at Moq.CastleProxyFactory.CreateProxy(Type , IInterceptor , Type[] , Object[] )
   at Moq.Mock`1[[System.ComponentModel.TypeDescriptionProvider, System.ComponentModel.TypeConverter, Version=7.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]].InitializeInstance()
   at Moq.Mock`1[[System.ComponentModel.TypeDescriptionProvider, System.ComponentModel.TypeConverter, Version=7.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]].OnGetObject()
   at Moq.Mock.get_Object()
   at Moq.Mock`1[[System.ComponentModel.TypeDescriptionProvider, System.ComponentModel.TypeConverter, Version=7.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]].get_Object()
   at System.ComponentModel.Tests.TypeDescriptionProviderTests.IsSupportedType_NullTypeWithParent_ThrowsArgumentNullException()
   at System.Reflection.MethodInvoker.InterpretedInvoke(Object , Span`1 , BindingFlags )

radical · 2022-10-20T16:24:45Z

The wasm failures are being hit only when the tests are trimmed. So, this is likely missing something that needs to be preserved, like in https://github.com/dotnet/runtime/blob/main/eng/testing/ILLinkDescriptors/ILLink.Descriptors.Castle.xml

am11 · 2022-10-21T03:00:43Z

Remaining failures are unrelated #77282.

elinor-fung · 2022-10-21T03:20:28Z

Thanks, @am11!

ghost added the community-contribution Indicates that the PR has been added by a community member label Oct 11, 2022

am11 added the area-Infrastructure-installer label Oct 11, 2022

am11 force-pushed the feature/host/migrate-newtonsoft-stj branch 3 times, most recently from de46dd7 to c43f539 Compare October 11, 2022 20:15

am11 closed this Oct 11, 2022

am11 reopened this Oct 11, 2022

This was referenced Oct 12, 2022

Tracking issue for CI build timeouts #76454

Closed

Assertion failed: (*card_word)==0 in DynamicGenerics tests #76801

Closed

am11 force-pushed the feature/host/migrate-newtonsoft-stj branch 4 times, most recently from 6b70fd4 to 1e266f6 Compare October 12, 2022 10:26

Migrate host.tests from Newtonsoft to STJ

608150a

am11 force-pushed the feature/host/migrate-newtonsoft-stj branch from 1e266f6 to 608150a Compare October 12, 2022 10:49

am11 marked this pull request as ready for review October 12, 2022 12:42

am11 requested a review from vitek-karas October 12, 2022 12:45

am11 commented Oct 12, 2022

View reviewed changes

am11 requested review from eerhardt and elinor-fung October 13, 2022 13:35

eerhardt approved these changes Oct 13, 2022

View reviewed changes

elinor-fung reviewed Oct 13, 2022

View reviewed changes

elinor-fung approved these changes Oct 13, 2022

View reviewed changes

Revert MoqVersion

4d10094

elinor-fung merged commit b4a3c68 into dotnet:main Oct 21, 2022

am11 deleted the feature/host/migrate-newtonsoft-stj branch October 21, 2022 03:40

ghost locked as resolved and limited conversation to collaborators Nov 25, 2022

Conversation

am11 commented Oct 11, 2022

Uh oh!

ghost commented Oct 11, 2022

Uh oh!

am11 commented Oct 11, 2022

Uh oh!

am11 Oct 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eiriktsarpalis Oct 13, 2022

Choose a reason for hiding this comment

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

elinor-fung Oct 13, 2022

Choose a reason for hiding this comment

Uh oh!

am11 Oct 13, 2022

Choose a reason for hiding this comment

Uh oh!

elinor-fung left a comment

Choose a reason for hiding this comment

Uh oh!

am11 commented Oct 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elinor-fung commented Oct 20, 2022

Uh oh!

radical commented Oct 20, 2022

Uh oh!

am11 commented Oct 21, 2022

Uh oh!

elinor-fung commented Oct 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

am11 Oct 12, 2022 •

edited

Loading

am11 commented Oct 20, 2022 •

edited

Loading