-
Notifications
You must be signed in to change notification settings - Fork 389
use .NET 8.0 target framework for coverlet.core and remove Newtonsoft.Json #1733
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
78c4071
to
b62cff1
Compare
b62cff1
to
ee6d285
Compare
ee6d285
to
cef7021
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR raises the minimum .NET SDK/runtime requirement to 8.0 and swaps out all Newtonsoft.Json
usages in favor of System.Text.Json
across core and test projects.
- Projects and tests are retargeted and package references updated to .NET 8.0.
- JSON serialization calls are migrated from
Newtonsoft.Json
toSystem.Text.Json
. - Integration tests, docs, and config files are adjusted to reflect the new framework version and package versions.
Reviewed Changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
test/coverlet.integration.tests/Msbuild.cs | Retarget tests to net8.0 , refactor file-path assertions. |
test/coverlet.integration.tests/DotnetTool.cs | Removed stray Unicode BOM from file header. |
test/coverlet.integration.tests/DeterministicBuild.cs | Refined props file creation and XDocument initialization. |
test/coverlet.integration.template/nuget.config | Renamed NuGet source key from nuget to nuget.org . |
test/coverlet.integration.determisticbuild/coverlet.integration.determisticbuild.csproj | Updated csproj to target net8.0 . |
test/coverlet.core.tests/coverlet.core.tests.csproj | Cleaned up BOM character in csproj header. |
test/coverlet.core.tests.samples.netstandard/coverlet.core.tests.samples.netstandard.csproj | Bumped System.Threading.Tasks.Extensions to 4.6.3 . |
src/coverlet.msbuild.tasks/coverlet.msbuild.targets | Raised _CoverletSdkMinVersionWithDependencyTarget to 8.0.100 . |
src/coverlet.core/coverlet.core.csproj | Added net8.0 target, removed explicit VersionOverride , suppressed IDE0057. |
src/coverlet.core/Reporters/JsonReporter.cs | Replaced JsonConvert with JsonSerializer and configured options. |
src/coverlet.core/Instrumentation/CecilAssemblyResolver.cs | Swapped Newtonsoft.Json.Linq for System.Text.Json parsing. |
src/coverlet.core/Helpers/InstrumentationHelper.cs | Scoped IDE0057 suppression around substring calls. |
src/coverlet.core/Exceptions.cs | Removed [Serializable] and serialization ctor stubs. |
src/coverlet.core/CoverageResult.cs | Added [JsonConstructor] and new collection expression syntax. |
src/coverlet.core/Coverage.cs | Swapped Newtonsoft.Json calls for System.Text.Json APIs. |
global.json | Bumped SDK version to 8.0.409 . |
README.md | Updated notes and requirements for .NET 8 and styled as markdown note. |
Documentation/VSTestIntegration.md | Updated example paths from net6.0 to net8.0 . |
Directory.Packages.props | Bumped multiple package versions and removed Newtonsoft.Json . |
CONTRIBUTING.md | Replaced SDK list with STS 9.0 and LTS 8.0 entries. |
Comments suppressed due to low confidence (3)
CONTRIBUTING.md:7
- [nitpick] This entry is missing a markdown bullet marker (e.g.,
-
or*
); add one to keep consistent list formatting.
STS version [.NET SDK 9.0](https://dotnet.microsoft.com/en-us/download/dotnet/9.0) for development environment
src/coverlet.core/Helpers/InstrumentationHelper.cs:431
- [nitpick] Instead of suppressing IDE0057, use the C# range operator (e.g.,
x[1..x.IndexOf(']')]
) for clearer substring logic.
x.Substring(1, x.IndexOf(']') - 1)
test/coverlet.integration.tests/DotnetTool.cs:1
- There's a stray BOM character at the start of this file which may cause encoding issues; remove it.
// Copyright (c) Toni Solarin-Sodara
@@ -47,7 +49,7 @@ private void CreateDeterministicTestPropsFile() | |||
_testProjectTfm = XElement.Load(Path.Combine(_testProjectPath, "coverlet.integration.determisticbuild.csproj"))!. | |||
Descendants("PropertyGroup")!.Single().Element("TargetFramework")!.Value; | |||
|
|||
deterministicTestProps.Save(Path.Combine(_testProjectPath, PropsFileName)); | |||
deterministicTestProps.Save(Path.Combine(propsFile)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Unnecessary use of Path.Combine
with a single argument. Call Save(propsFile)
directly for readability.
deterministicTestProps.Save(Path.Combine(propsFile)); | |
deterministicTestProps.Save(propsFile); |
Copilot uses AI. Check for mistakes.
JToken rootElement = configuration.Root; | ||
JToken runtimeOptionsElement = rootElement["runtimeOptions"]; | ||
JsonElement rootElement = configuration.RootElement; | ||
JsonElement runtimeOptionsElement = rootElement.GetProperty("runtimeOptions"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using GetProperty
will throw if runtimeOptions
is missing. Consider using TryGetProperty
to handle absence gracefully.
JsonElement runtimeOptionsElement = rootElement.GetProperty("runtimeOptions"); | |
if (!rootElement.TryGetProperty("runtimeOptions", out JsonElement runtimeOptionsElement)) | |
{ | |
throw new InvalidOperationException($"The 'runtimeOptions' property is missing in the runtime configuration file {_runtimeConfigFile}."); | |
} |
Copilot uses AI. Check for mistakes.
related: #1713