Solution based merging for data collector #1676

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

Jump to bottom

Open

daveMueller wants to merge 22 commits into coverlet-coverage:master

from daveMueller:1307_solution-based-merging-collector

Open

Solution based merging for data collector #1676

daveMueller wants to merge 22 commits into coverlet-coverage:master from daveMueller:1307_solution-based-merging-collector

+268 −32

Conversation

daveMueller

Copy link

Collaborator

@daveMueller daveMueller commented Jul 26, 2024

closes #1307

DavidMuellerGetinge and others added 9 commits

June 3, 2024 23:49

@DavidMuellerGetinge


 initial implementation

9627e72

@daveMueller


 prototyping

a6966ba

@DavidMuellerGetinge

nit

453d589

@daveMueller


 added some todos

00d38bb

@daveMueller


 Merge branch '1307_solution-based-merging-collector' of https://githu...

7a3a3ce

...b.com/daveMueller/coverlet into 1307_solution-based-merging-collector
# Conflicts:
#	src/coverlet.collector/ArtifactPostProcessor/CoverletCoveragePostProcessor.cs

@daveMueller


 further development for json format

62e4f58

@daveMueller


 refactorings

44785d9

@daveMueller

nit

92b0fea

@daveMueller

nit

1b912e4

@daveMueller

Copy link

Collaborator Author

daveMueller commented Jul 26, 2024

I guess I need some additional feedback/help here. The main problem is that I still don't fully understand how it is intended to implement IDataCollectorAttachmentProcessor. I see it beeing executed for every test project where the attachment then only is the projects test result. And also it gets executed once for the whole solution where the attachments are all the test runs results. Thus I could only check if attachments > 1 and then merge all of them together.
But from previous discussions it seems like there could be intermediate results that need to merged.
Therfore I added now a new runsettings option called MergeDirectory to store intermediate results. The idea behind that is that everytime the attachment processor triggers all attachments are merged plus the intermediate result in the MergeDirectory. The result of this merge is the new intermediate result.
Unfortunately the MergeDirectory path must be an absolute path so that this works reliable. If it is a relative path, the location changes depending on the if the processor is triggered from a project or from a solution.
cc: @MarcoRossignoli @bert2

@MarcoRossignoli

Copy link

Collaborator

MarcoRossignoli commented Aug 13, 2024 •

edited

Loading

Therfore I added now a new runsettings option called MergeDirectory to store intermediate results. The idea behind that is that everytime the attachment processor triggers all attachments are merged plus the intermediate result in the MergeDirectory. The result of this merge is the new intermediate result.

Why you need intermediate? take the directory from the first of the 2 attachments you receive and save in the same directory removing the 2 and send back. Something like a reduce. Attachments usually are inside the result directory specified in the config.

daveMueller and others added 12 commits

August 31, 2024 00:15

@daveMueller


 adaptions

f67b976

@daveMueller


 added new parameter, removed requirement for json format setting

c6f5f0e

@daveMueller


 refactoring (untested)

3e40ef5

@daveMueller


 Merge branch 'coverlet-coverage:master' into 1307_solution-based-merg...

ada8b1e

...ing-collector

@daveMueller


 debug

ea1cb6d

@DavidMuellerGetinge


 adaptions

5b29253

@daveMueller


 some changes

249ed87

@DavidMuellerGetinge


 changes

988cb9e

@daveMueller


 tests + cleanup

1b7d207

@daveMueller


 solution for deterministic cobertura reports

8281be7

@DavidMuellerGetinge


 added some documentation

d67bb46

@daveMueller


 docs

90768d6

@daveMueller daveMueller marked this pull request as ready for review

March 4, 2025 23:08

@daveMueller


 added troubleshoot documentation

69777e9

@Bertk Bertk requested a review from Copilot

June 9, 2025 13:40

Copilot

Copilot AI reviewed

Jun 9, 2025

View reviewed changes

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 PR implements solution based merging for the data collector by introducing a new ReportMerging option. Key changes include:

Adding a new ReportMerging flag and parsing logic via the ReportFormatParser.
Refactoring settings parsing and reporter creation to include JSON reports when merging is enabled.
Implementing a new post processor (CoverletCoveragePostProcessor) for merging multiple coverage reports and updating documentation accordingly.

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/coverlet.core/Helpers/SourceRootTranslator.cs	Added an additional constructor initializing mapping dictionaries.
src/coverlet.collector/Utilities/ReportFormatParser.cs	Introduced a new helper for parsing report formats and related configuration elements.
src/coverlet.collector/Utilities/CoverletConstants.cs	Added a new constant for the ReportMerging option.
src/coverlet.collector/DataCollection/CoverletSettingsParser.cs	Refactored to use ReportFormatParser and parse the new ReportMerging flag.
src/coverlet.collector/DataCollection/CoverletSettings.cs	Added the new ReportMerging property and updated ToString output.
src/coverlet.collector/DataCollection/CoverletCoverageCollector.cs	Added a new attribute to integrate the post processor.
src/coverlet.collector/DataCollection/CoverageManager.cs	Modified reporter creation to automatically include the JSON reporter when report merging is enabled.
src/coverlet.collector/ArtifactPostProcessor/CoverletCoveragePostProcessor.cs	Added a new post processor implementing the merging logic for coverage reports.
Documentation/VSTestIntegration.md	Updated runsettings documentation to include the ReportMerging option.
Documentation/Troubleshooting.md	Added guidance for debugging the post processor.
Documentation/Changelog.md	Documented the improvements and new merging feature.

src/coverlet.collector/DataCollection/CoverageManager.cs

Comment on lines +37 to +40

if (settings.ReportMerging && ! settings.ReportFormats.Contains("json"))

settings.ReportFormats = settings.ReportFormats.Append("json").ToArray();

return settings.ReportFormats.Select(format =>

Copy link

Preview

Copilot AI Jun 9, 2025

Choose a reason for hiding this comment

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

Mutating settings.ReportFormats in place may introduce unintended side effects; consider assigning the updated report formats to a new local variable to preserve immutability.

Suggested change

if (settings.ReportMerging && ! settings.ReportFormats.Contains("json"))

settings.ReportFormats = settings.ReportFormats.Append("json").ToArray();

return settings.ReportFormats.Select(format =>

IReporter[] updatedReportFormats = settings.ReportFormats;

if (settings.ReportMerging && ! updatedReportFormats.Contains("json"))

updatedReportFormats = updatedReportFormats.Append("json").ToArray();

return updatedReportFormats.Select(format =>

Copilot uses AI. Check for mistakes.

src/coverlet.collector/ArtifactPostProcessor/CoverletCoveragePostProcessor.cs

Comment on lines +78 to +79

if (! string.IsNullOrEmpty(directory) && Directory.Exists(directory))

Directory.Delete(directory, true);

Copy link

Preview

Copilot AI Jun 9, 2025

Choose a reason for hiding this comment

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

Consider adding checks or logging to ensure that deleting the directory does not inadvertently remove non-report files; additional filtering criteria may improve safety.

Suggested change

if (! string.IsNullOrEmpty(directory) && Directory.Exists(directory))

Directory.Delete(directory, true);

if (!string.IsNullOrEmpty(directory) && Directory.Exists(directory))

{

var files = Directory.GetFiles(directory);

var reportFiles = files.Where(file => file.EndsWith(".json") || file.EndsWith(".xml")).ToList();

if (reportFiles.Count == files.Length) // Ensure all files are report-related

{

Console.WriteLine($"Deleting directory: {directory}. Files: {string.Join(", ", reportFiles)}");

Directory.Delete(directory, true);

}

else

{

Console.WriteLine($"Skipping directory: {directory}. Contains non-report files.");

}

Copilot uses AI. Check for mistakes.

src/coverlet.core/Helpers/SourceRootTranslator.cs

Comment on lines +29 to +38

_sourceRootMapping = new Dictionary<string, List<SourceRootMapping>>();

_sourceToDeterministicPathMapping = new Dictionary<string, List<string>>();

}

public SourceRootTranslator(ILogger logger, IFileSystem fileSystem)

{

_logger = logger ?? throw new ArgumentNullException(nameof(logger));

_fileSystem = fileSystem ?? throw new ArgumentNullException(nameof(fileSystem));

_sourceRootMapping = new Dictionary<string, List<SourceRootMapping>>();

_sourceToDeterministicPathMapping = new Dictionary<string, List<string>>();

Copy link

Preview

Copilot AI Jun 9, 2025

Choose a reason for hiding this comment

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

[nitpick] Since similar dictionary initializations are repeated across constructors, consider consolidating them into a common initializer to reduce duplication.

Suggested change

_sourceRootMapping = new Dictionary<string, List<SourceRootMapping>>();

_sourceToDeterministicPathMapping = new Dictionary<string, List<string>>();

}

public SourceRootTranslator(ILogger logger, IFileSystem fileSystem)

{

_logger = logger ?? throw new ArgumentNullException(nameof(logger));

_fileSystem = fileSystem ?? throw new ArgumentNullException(nameof(fileSystem));

_sourceRootMapping = new Dictionary<string, List<SourceRootMapping>>();

_sourceToDeterministicPathMapping = new Dictionary<string, List<string>>();

InitializeMappings();

}

public SourceRootTranslator(ILogger logger, IFileSystem fileSystem)

{

_logger = logger ?? throw new ArgumentNullException(nameof(logger));

_fileSystem = fileSystem ?? throw new ArgumentNullException(nameof(fileSystem));

InitializeMappings();

Copilot uses AI. Check for mistakes.

Labels

None yet

3 participants

@daveMueller @MarcoRossignoli @DavidMuellerGetinge

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Solution based merging for data collector #1676

Are you sure you want to change the base?

Solution based merging for data collector #1676

Conversation

@daveMueller daveMueller commented Jul 26, 2024

Uh oh!

daveMueller commented Jul 26, 2024

Uh oh!

MarcoRossignoli commented Aug 13, 2024 •

edited

Loading

Uh oh!

Uh oh!

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Solution based merging for data collector #1676

Are you sure you want to change the base?

Solution based merging for data collector #1676

Conversation

@daveMueller daveMueller commented Jul 26, 2024

Uh oh!

daveMueller commented Jul 26, 2024

Uh oh!

MarcoRossignoli commented Aug 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MarcoRossignoli commented Aug 13, 2024 •

edited

Loading