Add .NET 10 to the list of test TFMs and update CI #702
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds .NET 10 to the list of target framework monikers (TFMs) for the Giraffe project. The changes adapt the codebase to handle breaking changes in .NET 10's test infrastructure, specifically the deprecation of TestServer constructor patterns in favor of using HostBuilder with ConfigureWebHost.
- Updates target frameworks to include net10.0 across project files
- Modifies test infrastructure to use
HostBuilderpattern for .NET 10+ via conditional compilation - Updates CI/CD workflows to build and test against .NET 10.x SDK
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/Giraffe/Giraffe.fsproj |
Adds net10.0 to the target frameworks list |
tests/Giraffe.Tests/Giraffe.Tests.fsproj |
Adds net10.0 target framework and Microsoft.AspNetCore.TestHost 10.0.* package reference |
tests/Giraffe.Tests/Helpers.fs |
Implements .NET 10 compatibility using HostBuilder pattern with conditional compilation, adds Microsoft.Extensions.Hosting import |
.github/workflows/build-and-test.yml |
Adds .NET 10.x SDK to build matrix and updates analyzer job to use .NET 10.x |
.github/workflows/publish.yml |
Adds .NET 10.x SDK to both build and deploy job matrices |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 7.x | ||
| 8.x | ||
| 9.x | ||
| 10.x |
There was a problem hiding this comment.
Does this matrix make sense? Would just keeping 9.0 and 10.0 not make more sense?
There was a problem hiding this comment.
Indeed, I think it could work just keeping 9.0 and 10.0.
I'll send a commit to test.
There was a problem hiding this comment.
It was working before because GitHub Actions is currently automatically installing .NET {8, 9}.
Related comment: #702 (comment)
.github/workflows/build-and-test.yml
Outdated
| uses: actions/setup-dotnet@v5 | ||
| with: | ||
| dotnet-version: 9.x | ||
| dotnet-version: 10.x |
There was a problem hiding this comment.
Can this not pick up the version from the global.json?
There was a problem hiding this comment.
This project doesn't have a global.json. Perhaps we could add one in the future.
src/Giraffe/Giraffe.fsproj
Outdated
|
|
||
| <!-- Build settings --> | ||
| <TargetFrameworks>net6.0;net7.0;net8.0;net9.0</TargetFrameworks> | ||
| <TargetFrameworks>net6.0;net7.0;net8.0;net9.0;net10.0</TargetFrameworks> |
There was a problem hiding this comment.
Life is too short to support all these.
I would go for 9 & 10 only here.
There was a problem hiding this comment.
Im unsure why we need to add this version in the first place. Is there an api we’re missing if we dont add it?
There was a problem hiding this comment.
Is there an api we’re missing if we dont add it?
No relevant APIs missing for now, AFAIK.
However, I still think it's good to add this version there. Doing so, we grant that the code targeting this version compiles successfully.
There was a problem hiding this comment.
Newer versions of dotnet can consume older versions of TFMs.
See demystifyfp/FsToolkit.ErrorHandling#345 (review) for a more detailed example of when adding a TFM is a good idea.
There was a problem hiding this comment.
I know. My point is related to potential breaking changes being captured earlier in this codebase.
For example, when I added net10.0 to the TFMs at the tests project, I discovered this: aspnet/Announcements#526. And already added the code to deal with this breaking change.
My rationale was that adding net10.0 to the TFMs at the src/ project would help to identify this kind of problem earlier, or at least grant that the project is able to compile properly using .NET 10.
Perhaps this makes our life easier in the future in a scenario where we really want to update the base .NET version due to missing APIs.
There was a problem hiding this comment.
I removed the net10.0 from the src/ project and update the CI to install only .NET 10, since it might be enough to build the older versions.
- Update GitHub Actions configuration - Update tests due to .NET 10 breaking changes
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
64J0
changed the title
.github/workflows/build-and-test.yml
Outdated
| 7.x | ||
| 8.x | ||
| 9.x | ||
| dotnet-version: 10.x |
There was a problem hiding this comment.
I think we should keep the 8.x and 9.x versions explicitly since we're still running tests again 8 and 9 TFMs. While github runners come with them by default, I wouldn't want to rely on them for being in control and removing it.
There was a problem hiding this comment.
Ack, I'll send the patch later today
Description
With this PR, I'm adding .NET 10 to the list of test TFMs and updating CI to install only .NET 10.
How to test
Check that CI is working fine.