Enable .NET E2E tests to run on .NET Framework (net472)#1358
Merged
Conversation
The E2E tests were previously excluded from the net472 build due to missing APIs on .NET Framework. This change adds test-only polyfills and makes minimal test code adjustments to enable the full E2E suite on net472. Key changes: - Add test/Polyfills/ with shims for string, IO, and Task APIs that are unavailable on .NET Framework (kept separate from src polyfills to avoid accidental production use) - Add Microsoft.Bcl.Memory package for System.Index/System.Range support - Add System.Net.Http.Json and System.Net.Http references for net472 - Replace net472-incompatible APIs in test code: - ProcessStartInfo.ArgumentList -> Arguments string - TcpListener using statement -> try/finally (not IDisposable on net472) - await using FileStream -> using (not IAsyncDisposable on net472) - Dictionary(IReadOnlyDictionary) constructor -> concrete Dictionary param - Add SkipOnNetFrameworkAttribute for SQLite tests (native library loading unsupported on .NET Framework) - Accommodate .NET Framework pipe error message in stdio transport test - Remove E2E/Harness file exclusions from the net472 build Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR enables the .NET SDK’s E2E test suite to compile and run on .NET Framework (net472) by removing prior compile-time exclusions and introducing test-only polyfills plus a few targeted test adjustments.
Changes:
- Re-enable compilation of the E2E + Harness test code on
net472and add net472-only dependencies needed for downlevel builds. - Add test-only polyfills for APIs missing on .NET Framework (Task/String/IO), guarded by
#if !NET8_0_OR_GREATER. - Adjust a handful of tests for net472 compatibility (process start args, TcpListener disposal pattern, async disposals, targeted skips, and more permissive assertions for stdio error messages).
Show a summary per file
| File | Description |
|---|---|
| dotnet/test/Polyfills/TaskExtensions.cs | Adds a test-only Task.WaitAsync(TimeSpan) polyfill for downlevel frameworks. |
| dotnet/test/Polyfills/StringExtensions.cs | Adds test-only string API polyfills used by tests on net472. |
| dotnet/test/Polyfills/IoExtensions.cs | Adds test-only IO polyfills (e.g., Path.Join, File.*Async, File.Move(overwrite)), used to unblock net472 compilation. |
| dotnet/test/Harness/SkipOnNetFrameworkAttribute.cs | Introduces an xUnit attribute to skip tests on .NET Framework. |
| dotnet/test/Harness/E2ETestContext.cs | Tweaks environment handling to use concrete Dictionary for net472 compatibility. |
| dotnet/test/Harness/CapiProxy.cs | Adds System.Net.Http import to support net472 compilation/use. |
| dotnet/test/GitHub.Copilot.SDK.Test.csproj | Removes net472 compile excludes and adds net472-only references/packages to support running E2E on net472. |
| dotnet/test/E2E/SessionFsSqliteE2ETests.cs | Skips SQLite-based tests on .NET Framework due to native loading limitations. |
| dotnet/test/E2E/SessionFsE2ETests.cs | Replaces await using for FileStream with using to support net472. |
| dotnet/test/E2E/RpcExtensionsLoadedE2ETests.cs | Switches from ArgumentList to Arguments and sets UseShellExecute=false for net472 support. |
| dotnet/test/E2E/PerSessionAuthE2ETests.cs | Adjusts helper signature to accept Dictionary for downlevel compatibility. |
| dotnet/test/E2E/ClientOptionsE2ETests.cs | Removes using var on TcpListener (not disposable on net472) and relies on Stop(). |
| dotnet/test/E2E/ClientE2ETests.cs | Loosens assertions to accept net472 pipe error messaging differences. |
| dotnet/Directory.Packages.props | Adds centrally-managed versions for Microsoft.Bcl.Memory and System.Net.Http.Json. |
Copilot's findings
- Files reviewed: 14/14 changed files
- Comments generated: 1
The custom SkipOnNetFrameworkAttribute file was not being resolved by Roslyn on CI (SDK 10.0.300) despite being correctly committed. Replace with #if NETFRAMEWORK preprocessor directives which are guaranteed to work regardless of SDK version. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Cross-SDK Consistency Review ✅This PR makes no changes to the production SDK code in any language — all 13 modified files are in
Since no public API surface is added or changed in any SDK, there are no cross-language consistency concerns. The
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
The .NET SDK E2E tests were previously excluded from the net472 build via explicit
<Compile Remove>directives in the test csproj. This meant the SDK's .NET Framework compatibility was only validated by unit tests, not by the full E2E suite that exercises the real client-proxy-CLI pipeline.Approach
Rather than modifying the production polyfills (which could lead to accidentally shipping inefficient implementations), this PR adds test-only polyfills in
dotnet/test/Polyfills/that shim missing .NET Framework APIs just well enough for tests to compile and run. The productionsrc/Polyfills/directory is unchanged.Key pieces:
test/Polyfills/): String extensions (Contains,ReplaceLineEndings,Split, etc.), IO extensions (File.ReadAllTextAsync,Path.Join,File.Movewith overwrite,FileSystemInfo.LinkTarget), and Task extensions (Task.WaitAsync(TimeSpan)). All gated with#if !NET8_0_OR_GREATER.Microsoft.Bcl.Memory(providesSystem.Index/System.Range),System.Net.Http.Json, and a framework reference toSystem.Net.Http-- all conditional on net472.ProcessStartInfo.ArgumentListwithArgumentsstring, removeusingonTcpListener(notIDisposableon net472), changeawait usingonFileStreamtousing, accept concreteDictionaryinstead ofIReadOnlyDictionarywhere the framework lacks the constructor overload.SkipOnNetFrameworkAttributefor 2 SQLite tests that fail due to native library loading limitations on .NET Framework.Results
On net472: 462 passed, 0 failed, 4 skipped (2 SQLite + 2 pre-existing skips).
On net8.0: No regressions -- all tests continue to pass as before.
CI will exercise net472 on the Windows runner automatically since
dotnet testruns all target frameworks.