Skip to content

Use multi targets projects for coverlet.collector, coverlet.msbuild.tasks packages #1742

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

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

Bertk
Copy link
Collaborator

@Bertk Bertk commented Mar 29, 2025

This pull request updates the project to support .NET 8 and 9, modernizes dependencies, and refines the build and testing process. The most important changes include updating target frameworks, revising dependency versions, improving documentation, and enhancing build and test configurations.

Note

This PR should be merged after PR #1733.

- Set `NetCurrent` to `net9.0` and `NetMinimum` to `net8.0` in `Directory.Build.props`.
- Updated SDK version in `global.json` to `9.0.203`.
- Changed target frameworks in `coverlet.collector.csproj`, `coverlet.console.csproj`, `coverlet.core.csproj`, and `coverlet.msbuild.tasks.csproj` to support multiple frameworks.
- Modified `CoverletToolsPath` in `Directory.Build.targets` to include `netstandard2.0`.
- Updated coverage file naming in `Msbuild.cs` to reflect the build target framework.
@Bertk Bertk force-pushed the multi-targets-projects branch from b133a8e to 06ce08a Compare April 10, 2025 07:00
@Bertk Bertk changed the title Multi targets projects Multi targets projects coverlet collector, console, msbuild.tasks Apr 14, 2025
@Bertk Bertk changed the title Multi targets projects coverlet collector, console, msbuild.tasks Use multi targets projects for coverlet.collector, coverlet.msbuild.tasks packages Apr 14, 2025
@Bertk Bertk force-pushed the multi-targets-projects branch from 12f47ad to 3a1b667 Compare July 20, 2025 07:59
@Bertk Bertk force-pushed the multi-targets-projects branch from 3a1b667 to 8009584 Compare July 20, 2025 08:18
@Bertk Bertk marked this pull request as ready for review July 23, 2025 07:28
@Copilot Copilot AI review requested due to automatic review settings July 23, 2025 07:28
Copy link

@Copilot Copilot AI left a 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 pull request modernizes the project to support .NET 8 and 9 as the target frameworks, replacing the previous .NET 6.0 support. The most significant changes include updating target frameworks across all projects, migrating from Newtonsoft.Json to System.Text.Json, consolidating build configurations, and improving package management for multi-targeting scenarios.

Key Changes:

  • Updated target frameworks from .NET 6.0 to .NET 8.0 (minimum) and .NET 9.0 (current)
  • Migrated JSON handling from Newtonsoft.Json to System.Text.Json throughout the codebase
  • Restructured MSBuild tasks and collector packages to support multi-targeting with proper NuGet package layout

Reviewed Changes

Copilot reviewed 42 out of 44 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
test/coverlet.tests.utils/TestUtils.cs Updated framework detection logic to support .NET 8.0, 9.0, and 10.0, simplified test results path
test/coverlet.integration.tests/Msbuild.cs Updated test framework references from net6.0 to net8.0/net9.0
src/coverlet.msbuild.tasks/coverlet.msbuild.tasks.csproj Added multi-targeting support (netstandard2.0 and NetMinimum) with updated package layout
src/coverlet.core/Reporters/JsonReporter.cs Migrated from Newtonsoft.Json to System.Text.Json with proper serialization options
src/coverlet.core/Coverage.cs Replaced Newtonsoft.Json dependencies with System.Text.Json throughout coverage processing
global.json Updated .NET SDK version from 8.0.407 to 9.0.302
Directory.Packages.props Updated package versions and removed Newtonsoft.Json dependency
Comments suppressed due to low confidence (2)

src/coverlet.core/Instrumentation/CecilAssemblyResolver.cs:325

  • The variable name 'runtimeoptionselement' is inconsistent with the naming pattern used elsewhere in the method. It should be 'includedFrameworksElement' for clarity and consistency.
      if (runtimeOptionsElement.TryGetProperty("includedFrameworks", out JsonElement runtimeoptionselement))

test/coverlet.integration.tests/DeterministicBuild.cs:22

  • [nitpick] The field name '_testProjectTfms' uses an abbreviation 'Tfms'. Consider using the full name '_testProjectTargetFrameworks' for better clarity.
    private string[] _testProjectTfms = [];

Comment on lines +314 to +327
return new List<(string, string)>
{
(runtimeOptionsElement.GetProperty("framework").GetProperty("name").GetString(), runtimeOptionsElement.GetProperty("framework").GetProperty("version").GetString())
};
}

if (runtimeOptionsElement?["frameworks"] != null)
if (runtimeOptionsElement.TryGetProperty("frameworks", out JsonElement frameworksElement))
{
return runtimeOptionsElement["frameworks"].Select(x => (x["name"]?.Value<string>(), x["version"]?.Value<string>())).ToList();
return frameworksElement.EnumerateArray().Select(x => (x.GetProperty("name").GetString(), x.GetProperty("version").GetString())).ToList();
}

if (runtimeOptionsElement?["includedFrameworks"] != null)
if (runtimeOptionsElement.TryGetProperty("includedFrameworks", out JsonElement runtimeoptionselement))
{
return runtimeOptionsElement["includedFrameworks"].Select(x => (x["name"]?.Value<string>(), x["version"]?.Value<string>())).ToList();
return runtimeoptionselement.EnumerateArray().Select(x => (x.GetProperty("name").GetString(), x.GetProperty("version").GetString())).ToList();
Copy link
Preview

Copilot AI Jul 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The method returns a List<(string, string)> but could return a more specific type or use a proper data structure with named properties instead of tuples for better readability and maintainability.

Copilot uses AI. Check for mistakes.

return [(runtimeOptionsElement["framework"]["name"]?.Value<string>(), runtimeOptionsElement["framework"]["version"]?.Value<string>())];
return new List<(string, string)>
{
(runtimeOptionsElement.GetProperty("framework").GetProperty("name").GetString(), runtimeOptionsElement.GetProperty("framework").GetProperty("version").GetString())
Copy link
Preview

Copilot AI Jul 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The frameworkElement variable is already obtained but not used. This line redundantly calls GetProperty("framework") multiple times instead of using the existing frameworkElement variable.

Suggested change
(runtimeOptionsElement.GetProperty("framework").GetProperty("name").GetString(), runtimeOptionsElement.GetProperty("framework").GetProperty("version").GetString())
(frameworkElement.GetProperty("name").GetString(), frameworkElement.GetProperty("version").GetString())

Copilot uses AI. Check for mistakes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant