Add cross-platform netstandard2.0 test coverage#1235
Conversation
Co-authored-by: eiriktsarpalis <2813363+eiriktsarpalis@users.noreply.github.com>
Co-authored-by: eiriktsarpalis <2813363+eiriktsarpalis@users.noreply.github.com>
| @@ -0,0 +1,83 @@ | |||
| <Project Sdk="Microsoft.NET.Sdk"> | |||
|
|
|||
| <PropertyGroup> | |||
There was a problem hiding this comment.
@copilot Please avoid using a separate test project. Ideally this should use the exact same test project, but non-Windows environments should be configured automatically accordingly.
There was a problem hiding this comment.
Changed to use the existing test project with conditional SetTargetFramework on the ModelContextProtocol.Core project reference. On non-Windows platforms, net8.0 tests now use the netstandard2.0 build automatically. See commit 753cc88.
…f separate project On non-Windows platforms, the net8.0 test target now forces netstandard2.0 build of ModelContextProtocol.Core via conditional SetTargetFramework. This provides cross-platform netstandard2.0 test coverage without requiring a separate test project. Co-authored-by: eiriktsarpalis <2813363+eiriktsarpalis@users.noreply.github.com>
| <ProjectReference Include="..\..\src\ModelContextProtocol.Core\ModelContextProtocol.Core.csproj" /> | ||
| <ProjectReference Include="..\..\src\ModelContextProtocol.Core\ModelContextProtocol.Core.csproj"> | ||
| <!-- On non-Windows platforms, force net8.0 tests to use netstandard2.0 build for cross-platform netstandard2.0 coverage --> | ||
| <SetTargetFramework Condition="'$(TargetFramework)' == 'net8.0' AND !$([MSBuild]::IsOSPlatform('Windows'))">TargetFramework=netstandard2.0</SetTargetFramework> |
There was a problem hiding this comment.
@copilot you probably also need to remove DefaultTestTargetFrameworks which is used by the earlier attempt to fix this same issue.
There was a problem hiding this comment.
Removed the platform-specific conditional logic from DefaultTestTargetFrameworks and made SetTargetFramework apply to all platforms. All platforms now test the same target frameworks (net10.0, net9.0, net8.0) with net8.0 using netstandard2.0 builds everywhere. See commit db11c07.
…getFramework to all platforms - Removed conditional net472 addition to DefaultTestTargetFrameworks on Windows - Changed SetTargetFramework to apply to all platforms (not just non-Windows) - Now all platforms test net10.0, net9.0, and net8.0, with net8.0 using netstandard2.0 build - This provides consistent netstandard2.0 test coverage across all platforms Co-authored-by: eiriktsarpalis <2813363+eiriktsarpalis@users.noreply.github.com>
This reverts the changes from commit db11c07. The approach of removing net472 on Windows was incorrect. We need to maintain both: 1. net472 tests on Windows (for netstandard2.0 testing on .NET Framework) 2. Conditional SetTargetFramework on non-Windows (for netstandard2.0 testing on modern .NET) Restoring the configuration where: - Windows tests: net10.0, net9.0, net8.0, net472 - Non-Windows tests: net10.0, net9.0, net8.0 (with net8.0 using netstandard2.0 build) Co-authored-by: eiriktsarpalis <2813363+eiriktsarpalis@users.noreply.github.com>
eiriktsarpalis
left a comment
There was a problem hiding this comment.
@copilot ideally we would want a way to still verify the net482 tests build in non windows platforms. We should either keep the target while ensuring tests aren't being run at all, or perhaps add a non-Windows specific netstandard2.0 target acting as a proxy for net482.
I attempted to add netstandard2.0 as a proxy target on non-Windows, but encountered package compatibility issues - the test packages (xunit, Microsoft.Extensions.TimeProvider.Testing) don't support netstandard2.0, causing NU1701 errors that can't be suppressed during restore. Would you prefer:
|
@copilot This would default to having the tests run using mono, which is something we want to avoid. |
Understood. Given that:
The current solution maintains the original goal: providing netstandard2.0 test coverage on non-Windows via net8.0 with SetTargetFramework, while Windows continues to test both net472 (for .NET Framework) and the modern TFMs. The net472 build verification on non-Windows appears infeasible without mono or additional tooling. |
|
@copilot what are the package compatibility issues with netstandard? |
When I attempted to add netstandard2.0 as a test target, the following test packages caused NU1701 errors during NuGet restore:
These packages were restored using .NET Framework targets instead of netstandard2.0, causing NU1701 warnings that became errors due to |
@copilot yes that is expected and desirable. |
I attempted to implement netstandard2.0 as a build-only target (Library), but encountered fundamental issues:
Even with all suppressions, xUnit's hard requirement for Exe makes the Library approach non-viable for a test project using xUnit. Would a separate, minimal test project targeting netstandard2.0 without xUnit dependencies work instead? |
eiriktsarpalis
left a comment
There was a problem hiding this comment.
@stephentoub are you ok with net8.0 silently using the ns2.0 build in non-Windows platforms?
|
@copilot please update the PR title and description to accurately reflect what is being changed. |
Updated the PR title and description to accurately reflect the single file change: adding conditional |
My preference would be a separate .csproj as outlined at #1235 (comment) so that it's very clear, but I don't feel super strongly about it. @halter73 ? |
…-project # Conflicts: # tests/ModelContextProtocol.Tests/ModelContextProtocol.Tests.csproj
|
@copilot The ubuntu CI test runs are seeing assembly load failures, so you may want to look into that. But before you do, can you explain why you didn't go with the suggested solution?
It's more code, but this seems cleaner and much clearer to me. |
|
@halter73 Unfortunately I hit an unexpected error while processing your comment. I've automatically reported this to GitHub. You can ask me to try again later by mentioning me in a new comment. If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: Sorry for the inconvenience! |
|
@copilot Can you get back to me on my last comment? |
On why I didn't originally use a separate project: earlier in this PR @eiriktsarpalis explicitly asked to avoid a separate test project ("Please avoid using a separate test project... non-Windows environments should be configured automatically") and then asked me to revert the separate-project commits, so I switched to the conditional 1. 2. Evidence: manually swapping just This is exactly why option (1) works (net472 loads the ns2.0/net462 assets for the whole closure, so the To make option (2) green I'd have to force the entire ns2.0 product dependency closure (AI, AI.Abstractions, System.Net.ServerSentEvents, and any future init-using deps) to deploy their netstandard2.0 assets into the net8.0 test output, which cascades to their own transitive deps and is fragile. How would you like to proceed?
Happy to implement whichever you prefer. |
Co-authored-by: halter73 <54385+halter73@users.noreply.github.com>
Sorry for the earlier error reply — here's the full picture. Why I didn't originally go with the separate project: earlier in this PR @eiriktsarpalis explicitly asked me to avoid a separate test project and then to revert the separate-project commits, so I switched to the conditional The Ubuntu assembly-load failures — two distinct causes:
This is why net472-on-Windows works (the whole closure loads ns2.0/net462 assets, so all Recommendation — pick one and I'll implement it:
Happy to push (b) immediately (it's mostly the dependency-asset deployment target plus a small exclusion list) if you're OK with the trade-off, or wire up (a) if you'd rather keep it clean. |
Summary
Adds a dedicated test project that exercises the
netstandard2.0build of the MCP libraries on a modern .NET runtime, providing netstandard2.0 test coverage on non-Windows platforms (wherenet472is not run to avoid mono). On Windows, the netstandard2.0 assets continue to be validated through thenet472target ofModelContextProtocol.Tests.Changes
Added:
tests/ModelContextProtocol.Netstandard2_0.Tests/ModelContextProtocol.Netstandard2_0.Tests.csprojnet8.0so it runs cross-platform without mono.ModelContextProtocolandModelContextProtocol.Corewith<SetTargetFramework>TargetFramework=netstandard2.0</SetTargetFramework>to force the product references to resolve to their netstandard2.0 build.ModelContextProtocol.Tests(excluding the external-process/stdio integration tests, which launch a separate net8.0 server process).Microsoft.Bcl.AsyncInterfacesso the netstandard2.0 product loads the required 10.x assembly instead of a transitive 6.0.0.Modified:
ModelContextProtocol.slnx,Directory.Packages.propsMicrosoft.Bcl.AsyncInterfacespackage version entry.Modified:
tests/ModelContextProtocol.Tests/ModelContextProtocol.Tests.csprojSetTargetFrameworkexperiment so the main test project once again uses matching TFMs per platform (net10.0,net9.0,net8.0, plusnet472on Windows).Implementation
Behavior
Windows
ModelContextProtocol.Tests:net10.0,net9.0,net8.0,net472(net472 validates netstandard2.0 on .NET Framework).ModelContextProtocol.Netstandard2_0.Tests:net8.0running the netstandard2.0 product build.Non-Windows (Linux/macOS)
ModelContextProtocol.Tests:net10.0,net9.0,net8.0.ModelContextProtocol.Netstandard2_0.Tests:net8.0running the netstandard2.0 product build (provides the netstandard2.0 coverage previously only available via net472 on Windows).Known limitation
Running the netstandard2.0 product on a modern .NET runtime cannot currently be made fully green. The test assembly is compiled against the
net8.0reference assets of the product's dependencies (Microsoft.Extensions.AI(.Abstractions),System.Net.ServerSentEvents) but runs against theirnetstandard2.0implementations, which define their own internalIsExternalInit. Anyinit-only member touched by net8.0-compiled code (e.g.AIJsonSchemaCreateOptions.TransformSchemaNode,SseItem.EventId) therefore throwsMissingMethodExceptiondue to differingIsExternalInittype identity. Deploying the netstandard2.0 assets of the dependency closure into the test output reduces failures substantially (from ~537 to ~13), but the residual cases are fundamental to this hybrid configuration and require a maintainer decision on how to proceed (build-only verification, a curated subset, or relying solely on net472-on-Windows).Original prompt
This pull request was created from Copilot chat.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.