Don't make web requests in HttpClient trimming test

[MichalStrehovsky]

The purpose of this test is just to check for presence of a file on the file system, doing actual networking makes it a reliability nightmare.

Cc @dotnet/ncl, this test has been pretty reliably failing in the CI on macOS only for a while with the below exception, it's a bit worrisome we can't make a web request to microsoft.com. See failures in the pipeline: https://dev.azure.com/dnceng-public/public/_build?definitionId=139&_a=summary

Unhandled exception. System.Net.Http.HttpRequestException: The SSL connection could not be established, see inner exception.
   ---> System.ArgumentOutOfRangeException: Specified argument was out of the range of valid values.
     at System.Net.Security.SslStream.ProcessTlsFrame(Int32)
     at System.Net.Security.SslStream.ForceAuthenticationAsync[TIOAdapter](Boolean, Byte[], CancellationToken)
     at System.Net.Http.ConnectHelper.EstablishSslConnectionAsync(SslClientAuthenticationOptions, HttpRequestMessage, Boolean, Stream, CancellationToken)
     --- End of inner exception stack trace ---
     at System.Net.Http.ConnectHelper.EstablishSslConnectionAsync(SslClientAuthenticationOptions, HttpRequestMessage, Boolean, Stream, CancellationToken)
     at System.Net.Http.HttpConnectionPool.ConnectAsync(HttpRequestMessage, Boolean, CancellationToken)
     at System.Net.Http.HttpConnectionPool.CreateHttp11ConnectionAsync(HttpRequestMessage, Boolean, CancellationToken)
     at System.Net.Http.HttpConnectionPool.InjectNewHttp11ConnectionAsync(QueueItem)
     at System.Threading.Tasks.TaskCompletionSourceWithCancellation`1.WaitWithCancellationAsync(CancellationToken)
     at System.Net.Http.HttpConnectionPool.SendWithVersionDetectionAndRetryAsync(HttpRequestMessage, Boolean, Boolean, CancellationToken)
     at System.Net.Http.RedirectHandler.SendAsync(HttpRequestMessage, Boolean, CancellationToken)
     at System.Net.Http.HttpClient.<SendAsync>g__Core|83_0(HttpRequestMessage, HttpCompletionOption, CancellationTokenSource, Boolean, CancellationTokenSource, CancellationToken)
     at Program.Main(String[]) in /_/artifacts/bin/trimmingTests/projects/System.Net.Http.TrimmingTests/HttpClientTest/osx-x64/HttpClientTest.cs:line 20

[dotnet-policy-service]

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

[ManickaP]

cc @wfurt @rzikm

[rzikm]

The specific SslStream failure is fixed by #104606

I don't know much about how trimming tests work, but it seems to me that a call to HttpClient method is necessary for "rooting" the file in question?

[MichalStrehovsky]

I don't know much about how trimming tests work, but it seems to me that a call to HttpClient method is necessary for "rooting" the file in question?

I guess so. People who know about trimming don't know what this test is for (#101416 (comment)) so I hope that at least someone in @dotnet/ncl would understand the purpose.

Placing the network request under the environment variable check doesn't affect trimming at all because trimming cannot prove anything about the environment variable so it will be kept.

What this does is that it avoids doing the web request at runtime. I don't believe we have concerns that web requests would be broken after trimming that would warrant actually doing a request. We don't write random tests for "I wonder if this API works with trimming/ReadyToRun/AOT" because making sure trimming/ReadyToRun/AOT don't break random APIs is the job of trimming/ReadyToRun/AOT testing.

[rzikm]

Placing the network request under the environment variable check doesn't affect trimming at all because trimming cannot prove anything about the environment variable so it will be kept.

Oh, I missed this is a PR not an issue, I see how this could work.

I guess so. People who know about trimming don't know what this test is for (#101416 (comment)) so I hope that at least someone in @dotnet/ncl would understand the purpose.

Since the test was introduced by @ManickaP in #49261, I will let her do the review :)

[ManickaP]

#49261 (comment)

It's fine to leave it. Once the other platforms come on line this will add value.

The intention is to test whether System.Net.Quic.dll was trimmed in case HTTP/3 is not supported. There was an issue with size that was fixed and this was to ensure we don't introduce a regression. When I wrote the test, I didn't know we didn't have a platform coverage (like mobile platforms) for trimming tests so I suggested removing the test, but I was told to keep. Feel free to remove it, we don't have a way prevent regression with size anyway...

Also, we don't have a good story for AOT with QUIC, it's been punted once again.

[MichalStrehovsky]

Feel free to remove it, we don't have a way prevent regression with size anyway...

Size is typically checked in the perf lab - we have size tests for Android, iOS, WASM, etc. A test like this would probably make more sense there, because such test would also detect all the other ways size can regress (E.g. what if webrequest suddenly depends on System.Text.Json, or suddenly depends on a long chain of types that were previously not referenced, or something like that). Checking for one DLL is fine, but it's not really a size test.

Only concerned about it because trimming tests are rather expensive (each one costs 10-20 seconds of CI time since we need to do whole program analysis and typically run it with both PublishTrimmed and PublishAot). We run trimming tests on all PRs. The usual purpose of trimming tests is not checking size, but ensuring places where we suppressed trimming warnings actually work after trimming.

[MichalStrehovsky]

Can someone signoff on this? Or would you prefer just deleting this test?

[ManickaP]

Can someone signoff on this? Or would you prefer just deleting this test?

I thought you're gonna delete it. I don't have a preference either way.

[MichalStrehovsky]

/ba-g timeouts in unrelated legs

[MihaZupan]

/backport to release/8.0-staging

[github-actions]

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/12806456701

[github-actions]

@MihaZupan backporting to "release/8.0-staging" failed, the patch most likely resulted in conflicts:

$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch

Applying: Don't make web requests in HttpClient trimming test
Applying: Update System.Net.Http.TrimmingTests.proj
Using index info to reconstruct a base tree...
M	src/libraries/System.Net.Http/tests/TrimmingTests/System.Net.Http.TrimmingTests.proj
Falling back to patching base and 3-way merge...
Auto-merging src/libraries/System.Net.Http/tests/TrimmingTests/System.Net.Http.TrimmingTests.proj
CONFLICT (content): Merge conflict in src/libraries/System.Net.Http/tests/TrimmingTests/System.Net.Http.TrimmingTests.proj
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0002 Update System.Net.Http.TrimmingTests.proj
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!