Test Formatting assemblies w/ `net6.0` #384

dougbu · 2023-02-02T00:22:31Z

expand our text matrix to include a modern (and LTS) TFM
move to VS2022 and the latest 6.0 .NET SDK for net6.0 support
change how Formatting test assemblies are found
- netcoreapp is no longer the only relevant folder prefix
react to new Exception.Messages in netcoreapp3.1
- handle different formatting of argument info in ArgumentException.Messages
- handle slightly greater decimal precision in a JsonReaderException.Message
react to new Exception.Messages and other changes in net6.0
- handle different Message in InvalidOperationExceptions about invalid request URIs
- handle HttpResponseMessage.Content != null after construction (a test-only change fortunately)
  - see dotnet/runtime@b48900f which introduced this
- handle inability to mock a Stream if a writer passes a ReadOnlySpan<byte> in net6.0 - see System.NotSupportedException : Cannot create boxed ByRef-like values. devlooped/moq#829, Support ref structs on mock Setup parameter devlooped/moq#979, and Developers using reflection invoke should be able to use ref struct dotnet/runtime#45152 about the issue
nit: simplify define use now that NETCOREAPP3_1_OR_GREATER and so on are available

dougbu

Calling on lots of reviewers because this PR mixes a few things together as I expand our test matrix. Thanks for your help❕

build.cmd

dougbu · 2023-02-02T00:25:22Z

test/System.Net.Http.Formatting.Test/Formatting/JsonMediaTypeFormatterTests.cs

+        // Cannot Mock a Stream and let JsonWriter write to it. Writer will use ReadOnlySpan in this case and such
+        // parameters are not currently mockable. See moq/moq4#829, moq/moq4#979, and dotnet/runtime#45152.
+        // Override here avoids the Mock<Stream> and should confirm this Stream is not closed. Also adds an
+        // additional check of the written text.


@stephentoub this relates to one of our email conversations. Thanks for your pointer to dotnet/runtime#45152 and helping me understand what's happening.

Thoughts from all reviewers much appreciated: Should I simply replace MediaTypeFormatterTestBase.WriteToStreamAsync_WhenObjectIsNull_WritesDataButDoesNotCloseStream along these lines❔ Would need a protected property containing the expected Stream content for a null value. But I'm thinking verifying what's written makes sense. I'm also thinking the BSON case might be more difficult to confirm.

test/System.Net.Http.Formatting.Test/Handlers/ProgressMessageHandlerTest.cs

test/System.Net.Http.Formatting.Test/HttpMessageContentTests.cs

dougbu · 2023-02-02T06:55:51Z

/fyi successful runs on the CI will likely take a week because the v4.5 targeting pack is not available on VS2022 build agents right now

test/System.Net.Http.Formatting.Test/Formatting/BsonMediaTypeFormatterTests.cs

test/System.Net.Http.Formatting.Test/HttpMessageContentTests.cs

dougbu

@stephentoub, @Tratcher, @karelz, @MihaZupan, the last commit is directed toward making the Formatting behaviour more consistent on all platforms and I'd appreciate your feedback. The commit description lists what I believe will change from Formatting 5.2.9 to 6.0.0 (our next release) related to conversations in this PR.

side note: Total consistency won't come from this (because we have some restrictions in the netstandard1.3 assembly) and is probably impossible. But, with respect to HttpResponseMessage.Content, nothing changes when moving from net461 to net7.0.

src/System.Net.Http.Formatting/HttpMessageContent.cs

test/System.Net.Http.Formatting.Test/Formatting/JsonMediaTypeFormatterTests.cs

test/System.Web.Http.Test/Controllers/VoidResultConverterTest.cs

Tratcher · 2023-02-10T18:50:04Z

src/System.Net.Http.Formatting/HttpMessageContent.cs

@@ -354,7 +357,7 @@ private byte[] SerializeHeader()
            return Encoding.UTF8.GetBytes(message.ToString());
        }

-        private void ValidateStreamForReading(Stream stream)
+        private void ValidateStreamForReading(Stream stream) // ???


Yeah, this seems like a bug, it should have checked CanSeek before setting the Position, not CanRead. I have seen some Streams return CanRead false after they're done, so that might have avoided the bug.

dougbu · 2023-02-11T00:39:44Z

Ran enough tests locally to confirm the last commit here does what it says on the tin. Will remove that commit and instead comment out broken tests and fix the infrastructure issue seen on the CI. Expect a new PR built on the "Add Content to all HttpResponseMessages" commit as well as a fix for ValidateStreamForReading(...)…

- expand our text matrix to include a modern (and LTS) TFM - change how Formatting test assemblies are found - `netcoreapp` is no longer the only relevant folder prefix - use the latest .NET 6 SDK - install the 2.1.x runtime in the pipeline - don't require .NET in VS - will use binplaced `msbuild` instead - have `git` ignore the new .msbuild/ folder - react to new `Exception.Message`s in `netcoreapp3.1` - handle different formatting of argument info in `ArgumentException.Message`s - handle slightly greater `decimal` precision in a `JsonReaderException.Message` - react to new `Exception.Message`s and other changes in `net6.0` - handle different `Message` in `InvalidOperationException`s about invalid request URIs - react to other changes in `net6.0` - handle inability to mock a `Stream` if a writer passes a `ReadOnlySpan<byte>` in `net6.0` - see devlooped/moq#829, devlooped/moq#979, and dotnet/runtime#45152 about the issue - skip tests failing due to `HttpResponseMessage` changes - see dotnet/runtime@b48900f which introduced this - nits: - simplify define use now that `NETCOREAPP3_1_OR_GREATER` and so on are available - clean up .gitignore

dougbu · 2023-02-11T05:23:33Z

test/System.Net.Http.Formatting.Test/System.Net.Http.Formatting.Test.csproj

@@ -20,7 +20,7 @@
    <Reference Include="System.Buffers, Version=4.0.3.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51, processorArchitecture=MSIL">
      <HintPath>..\..\packages\System.Buffers.4.5.1\lib\net461\System.Buffers.dll</HintPath>
      <SpecificVersion>False</SpecificVersion>
-      <Private>False</Private>
+      <Private>True</Private>


Wasn't reliably copied in local builds for me

dougbu · 2023-02-11T05:25:04Z

eng/GetXCopyMSBuild.ps1

@@ -0,0 +1,37 @@
+# Lifted from https://github.com/dotnet/arcade/blob/main/eng/common/tools.ps1


@MattGal this is pretty much what you suggested in dotnet/arcade#12402. Incorporating it in build.cmd was the only bit I added.

Looks good to me.

- nit: clean up a few comments and tighten scripting up

dougbu · 2023-02-12T01:35:33Z

/ping reviewers

I reset reviews because this works differently than the original PR. But it's working cleanly both locally and in the CI 😁

Suggest reviewers pick and choose what they sign off on e.g.

Updates to use binplaced msbuild from VS2022 on a VS2019 agent i.e. supporting net6.0 in msbuild while having old .NET Framework targeting packs available.
Switch to net6.0 and related infrastructure changes
Adjust tests to handle new netcoreapp3.1 and net6.0 behaviours
- Note compiled-out tests will be restored (and fixed) in my next PR

wtgodbe

Signing off on msbuild bits + .yml, global.json, .gitignore, and build.cmd

MattGal · 2023-02-13T17:17:32Z

eng/GetXCopyMSBuild.ps1

@@ -0,0 +1,37 @@
+# Lifted from https://github.com/dotnet/arcade/blob/main/eng/common/tools.ps1


Looks good to me.

test/System.Net.Http.Formatting.Test/Handlers/ProgressMessageHandlerTest.cs

- fix comming **Real Soon Now:tm:** :grin:

- fix aspnet#386 - reenable `net6.0` tests disabled in aspnet#384 for this issue - also, check `Stream.CanSeek` less when calculating `HttpMessageContent.ContentLength` value - use inner `HttpContent.Headers.ContentLength` if available - this changes behaviour slightly for both requests and responses - observable as `HttpMessageContent.Headers.ContentLength!=null` for empty messages - for responses, avoids `ContentLength==null` versus `ContentLength==0` inconsistencies (depending on platform) - `ContentLength==null` may still occur in some corner cases (which existed before) - e.g. when inner `HttpContent` doesn't know its length and `ReadAsStreamAsync()` hasn't completed - main change means `HttpResponseMessage`s we expose to user code are consistent across platforms - note: user code won't see `EmptyContent` in `HttpResponseMessage`s we create

- fix #386 - reenable `net6.0` tests disabled in #384 for this issue - also, check `Stream.CanSeek` less when calculating `HttpMessageContent.ContentLength` value - use inner `HttpContent.Headers.ContentLength` if available - this changes behaviour slightly for both requests and responses - observable as `HttpMessageContent.Headers.ContentLength!=null` for empty messages - for responses, avoids `ContentLength==null` versus `ContentLength==0` inconsistencies (depending on platform) - `ContentLength==null` may still occur in some corner cases (which existed before) - e.g. when inner `HttpContent` doesn't know its length and `ReadAsStreamAsync()` hasn't completed - main change means `HttpResponseMessage`s we expose to user code are consistent across platforms - note: user code won't see `EmptyContent` in `HttpResponseMessage`s we create

dougbu requested review from Tratcher, stephentoub, MackinnonBuck, TanayParikh, Nick-Stanton, mkArtakMSFT and a team February 2, 2023 00:22

dougbu commented Feb 2, 2023

View reviewed changes

Tratcher approved these changes Feb 2, 2023

View reviewed changes

test/System.Net.Http.Formatting.Test/Formatting/BsonMediaTypeFormatterTests.cs Show resolved Hide resolved

test/System.Net.Http.Formatting.Test/HttpMessageContentTests.cs Outdated Show resolved Hide resolved

dougbu mentioned this pull request Feb 2, 2023

Add the .NET Framework v4.5 targeting pack windows.vs2022.amd64[.open] queue. dotnet/arcade#12402

Closed

5 tasks

stephentoub reviewed Feb 2, 2023

View reviewed changes

test/System.Net.Http.Formatting.Test/HttpMessageContentTests.cs Outdated Show resolved Hide resolved

dougbu commented Feb 10, 2023

View reviewed changes

Tratcher reviewed Feb 10, 2023

View reviewed changes

Tratcher approved these changes Feb 10, 2023

View reviewed changes

dougbu force-pushed the dougbu/Newtonsoft.Json.net6 branch from 0ac8d00 to 8d4b667 Compare February 11, 2023 05:16

dougbu requested review from Tratcher, stephentoub and MattGal and removed request for TanayParikh February 11, 2023 05:17

dougbu commented Feb 11, 2023

View reviewed changes

Move %Path% setting up into build.cmd

28faf44

- nit: clean up a few comments and tighten scripting up

wtgodbe approved these changes Feb 13, 2023

View reviewed changes

MattGal approved these changes Feb 13, 2023

View reviewed changes

eng/GetXCopyMSBuild.ps1

@@ -0,0 +1,37 @@

# Lifted from https://github.com/dotnet/arcade/blob/main/eng/common/tools.ps1

Copy link

MattGal Feb 13, 2023

Choose a reason for hiding this comment

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

Looks good to me.

Tratcher approved these changes Feb 13, 2023

View reviewed changes

test/System.Net.Http.Formatting.Test/Handlers/ProgressMessageHandlerTest.cs Outdated Show resolved Hide resolved

dougbu mentioned this pull request Feb 13, 2023

Do not expose HttpResponseMessage.Content == null to user code #386

Closed

!fixup! Reference aspnet#386

a74ea59

- fix comming **Real Soon Now:tm:** :grin:

dougbu merged commit 0f4c624 into aspnet:main Feb 13, 2023

dougbu deleted the dougbu/Newtonsoft.Json.net6 branch February 13, 2023 20:39

dougbu mentioned this pull request Feb 17, 2023

Add Content to all HttpResponseMessages #387

Merged

dougbu mentioned this pull request Feb 23, 2023

MessageHttpContext does not handle unreadable/unseekable Streams well #389

Closed

dougbu added this to the 3.3.0 (5.3.0) milestone Feb 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test Formatting assemblies w/ `net6.0` #384

Test Formatting assemblies w/ `net6.0` #384

dougbu commented Feb 2, 2023

dougbu left a comment

dougbu Feb 2, 2023

dougbu commented Feb 2, 2023

dougbu left a comment

Tratcher Feb 10, 2023

dougbu commented Feb 11, 2023

dougbu Feb 11, 2023

dougbu Feb 11, 2023

MattGal Feb 13, 2023

dougbu commented Feb 12, 2023

wtgodbe left a comment

MattGal Feb 13, 2023

		@@ -0,0 +1,37 @@
		# Lifted from https://github.com/dotnet/arcade/blob/main/eng/common/tools.ps1

Test Formatting assemblies w/ net6.0 #384

Test Formatting assemblies w/ net6.0 #384

Conversation

dougbu commented Feb 2, 2023

dougbu left a comment

Choose a reason for hiding this comment

dougbu Feb 2, 2023

Choose a reason for hiding this comment

dougbu commented Feb 2, 2023

dougbu left a comment

Choose a reason for hiding this comment

Tratcher Feb 10, 2023

Choose a reason for hiding this comment

dougbu commented Feb 11, 2023

dougbu Feb 11, 2023

Choose a reason for hiding this comment

dougbu Feb 11, 2023

Choose a reason for hiding this comment

MattGal Feb 13, 2023

Choose a reason for hiding this comment

dougbu commented Feb 12, 2023

wtgodbe left a comment

Choose a reason for hiding this comment

MattGal Feb 13, 2023

Choose a reason for hiding this comment

Test Formatting assemblies w/ `net6.0` #384

Test Formatting assemblies w/ `net6.0` #384