Solution based merging for data collector#1676
Solution based merging for data collector#1676daveMueller wants to merge 22 commits intocoverlet-coverage:masterfrom
Conversation
…b.com/daveMueller/coverlet into 1307_solution-based-merging-collector # Conflicts: # src/coverlet.collector/ArtifactPostProcessor/CoverletCoveragePostProcessor.cs
|
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 |
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. |
There was a problem hiding this comment.
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. |
| if (settings.ReportMerging && ! settings.ReportFormats.Contains("json")) | ||
| settings.ReportFormats = settings.ReportFormats.Append("json").ToArray(); | ||
|
|
||
| return settings.ReportFormats.Select(format => |
There was a problem hiding this comment.
Mutating settings.ReportFormats in place may introduce unintended side effects; consider assigning the updated report formats to a new local variable to preserve immutability.
| 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 => |
| if (! string.IsNullOrEmpty(directory) && Directory.Exists(directory)) | ||
| Directory.Delete(directory, true); |
There was a problem hiding this comment.
Consider adding checks or logging to ensure that deleting the directory does not inadvertently remove non-report files; additional filtering criteria may improve safety.
| 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."); | |
| } | |
| } |
| _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>>(); |
There was a problem hiding this comment.
[nitpick] Since similar dictionary initializations are repeated across constructors, consider consolidating them into a common initializer to reduce duplication.
| _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(); |
| public Task<ICollection<AttachmentSet>> ProcessAttachmentSetsAsync(XmlElement configurationElement, | ||
| ICollection<AttachmentSet> attachments, IProgress<int> progressReporter, | ||
| IMessageLogger logger, CancellationToken cancellationToken) |
There was a problem hiding this comment.
The cancellationToken parameter is not used anywhere in the method. While the current implementation uses synchronous operations, the method signature suggests async cancellation support. Consider either utilizing the cancellationToken for long-running operations (like file I/O, JSON deserialization, and directory deletion), or document why it's not needed. At minimum, cancellationToken.ThrowIfCancellationRequested() should be called before expensive operations.
|
|
||
| private static bool IsFileWithJsonExt(UriDataAttachment x) | ||
| { | ||
| return IsFileAttachment(x) && Path.GetExtension(x.Uri.AbsolutePath).Equals(".json"); |
There was a problem hiding this comment.
String comparison is case-sensitive. The extension comparison with ".json" won't match files with uppercase extensions like ".JSON" or ".Json". Consider using Path.GetExtension(x.Uri.AbsolutePath).Equals(".json", StringComparison.OrdinalIgnoreCase) for case-insensitive comparison.
| return IsFileAttachment(x) && Path.GetExtension(x.Uri.AbsolutePath).Equals(".json"); | |
| return IsFileAttachment(x) && Path.GetExtension(x.Uri.AbsolutePath).Equals(".json", StringComparison.OrdinalIgnoreCase); |
| _reportFormatParser ??= new ReportFormatParser(); | ||
| _coverageResult ??= new CoverageResult(); | ||
| _coverageResult.Modules ??= new Modules(); | ||
| _logger = logger; |
There was a problem hiding this comment.
Thread safety concern: The class uses instance fields (_coverageResult, _reportFormatParser, _logger) that are initialized with null-coalescing operators and potentially shared across multiple calls to ProcessAttachmentSetsAsync. If this method is called concurrently or multiple times, there's potential for race conditions or state corruption. Since SupportsIncrementalProcessing is set to true (line 30), the class instance may be reused. Consider whether these fields should be method-local variables instead, or document thread-safety guarantees.
|
|
||
| private static IReporter[] CreateReporters(CoverletSettings settings, TestPlatformEqtTrace eqtTrace) | ||
| { | ||
| if (settings.ReportMerging && ! settings.ReportFormats.Contains("json")) |
There was a problem hiding this comment.
Spacing inconsistency: there's an extra space after the exclamation mark in this condition. Consider removing it for consistency with standard formatting conventions (e.g., !settings.ReportFormats.Contains("json") instead of ! settings.ReportFormats.Contains("json")).
| if (settings.ReportMerging && ! settings.ReportFormats.Contains("json")) | |
| if (settings.ReportMerging && !settings.ReportFormats.Contains("json")) |
| fileAttachments.ForEach(x => | ||
| { | ||
| string directory = Path.GetDirectoryName(x.Uri.LocalPath); | ||
| if (! string.IsNullOrEmpty(directory) && Directory.Exists(directory)) |
There was a problem hiding this comment.
Spacing inconsistency: there's an extra space after the exclamation mark in this condition. Consider removing it for consistency with standard formatting conventions (e.g., !string.IsNullOrEmpty(directory) instead of ! string.IsNullOrEmpty(directory)).
| if (! string.IsNullOrEmpty(directory) && Directory.Exists(directory)) | |
| if (!string.IsNullOrEmpty(directory) && Directory.Exists(directory)) |
|
|
||
| if (!reportMerging) return Task.FromResult(attachments); | ||
|
|
||
| IList<IReporter> reporters = CreateReporters(formats).ToList(); |
There was a problem hiding this comment.
If all report formats are invalid, the reporters list will be empty, but the code proceeds to use it without checking. When WriteCoverageReports is called with an empty reporters list (line 65), it will create an empty AttachmentSet, which may not be the desired behavior. Consider checking if the reporters list is empty and either logging a warning or returning early.
| string mergeFilePath = Path.GetDirectoryName(fileAttachments.First().Uri.LocalPath); | ||
|
|
There was a problem hiding this comment.
Potential bug: calling First() on an empty collection will throw an InvalidOperationException. If there are no file attachments in the collection, this will cause a runtime error. Consider adding a check to ensure fileAttachments is not empty before calling First(), or use FirstOrDefault() with a null check.
| string mergeFilePath = Path.GetDirectoryName(fileAttachments.First().Uri.LocalPath); | |
| var firstFileAttachment = fileAttachments.FirstOrDefault(); | |
| if (firstFileAttachment == null) | |
| { | |
| _logger?.SendMessage(TestMessageLevel.Warning, "No file attachments found to merge coverage reports."); | |
| return Task.FromResult(attachments); | |
| } | |
| string mergeFilePath = Path.GetDirectoryName(firstFileAttachment.Uri.LocalPath); |
| public SourceRootTranslator() | ||
| { | ||
| _sourceRootMapping = new Dictionary<string, List<SourceRootMapping>>(); | ||
| _sourceToDeterministicPathMapping = new Dictionary<string, List<string>>(); | ||
| } |
There was a problem hiding this comment.
The parameterless constructor leaves the _logger and _fileSystem fields as null. While the comment on line 127 states that "empty source root translator returns the original path," methods like ResolveFilePath (line 147) will attempt to access _fileSystem which could cause NullReferenceException. Either document that the parameterless constructor is only meant for limited use cases where these methods won't be called, or ensure all methods handle null fields gracefully.
| string json = File.ReadAllText(filePath); | ||
| coverageResult.Merge(JsonConvert.DeserializeObject<Modules>(json)); |
There was a problem hiding this comment.
Missing error handling: JSON deserialization can fail for various reasons (malformed JSON, incompatible schema, file read errors). If deserialization fails, it will throw an exception that propagates up without a meaningful error message. Consider wrapping this in a try-catch block and providing a more descriptive error message that includes the file path.
| string json = File.ReadAllText(filePath); | |
| coverageResult.Merge(JsonConvert.DeserializeObject<Modules>(json)); | |
| try | |
| { | |
| string json = File.ReadAllText(filePath); | |
| Modules modules = JsonConvert.DeserializeObject<Modules>(json); | |
| coverageResult.Merge(modules); | |
| } | |
| catch (Exception ex) | |
| { | |
| throw new CoverletDataCollectorException( | |
| $"{CoverletConstants.DataCollectorName}: Failed to merge coverage result from file '{filePath}'", | |
| ex); | |
| } |
| // Copyright (c) Toni Solarin-Sodara | ||
| // Licensed under the MIT license. See LICENSE file in the project root for full license information. | ||
|
|
||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.IO; | ||
| using System.Linq; | ||
| using System.Threading; | ||
| using System.Threading.Tasks; | ||
| using System.Xml; | ||
| using Coverlet.Collector.Utilities; | ||
| using Coverlet.Core; | ||
| using Coverlet.Core.Abstractions; | ||
| using Coverlet.Core.Reporters; | ||
| using Microsoft.VisualStudio.TestPlatform.ObjectModel; | ||
| using Microsoft.VisualStudio.TestPlatform.ObjectModel.DataCollection; | ||
| using Microsoft.VisualStudio.TestPlatform.ObjectModel.Logging; | ||
| using Newtonsoft.Json; | ||
| using System.Diagnostics; | ||
| using Coverlet.Core.Helpers; | ||
|
|
||
| namespace coverlet.collector.ArtifactPostProcessor | ||
| { | ||
| public class CoverletCoveragePostProcessor : IDataCollectorAttachmentProcessor | ||
| { | ||
| private CoverageResult _coverageResult; | ||
| private ReportFormatParser _reportFormatParser; | ||
| private IMessageLogger _logger; | ||
|
|
||
| public bool SupportsIncrementalProcessing => true; | ||
|
|
||
| public IEnumerable<Uri> GetExtensionUris() => new[] { new Uri(CoverletConstants.DefaultUri) }; | ||
|
|
||
| public Task<ICollection<AttachmentSet>> ProcessAttachmentSetsAsync(XmlElement configurationElement, | ||
| ICollection<AttachmentSet> attachments, IProgress<int> progressReporter, | ||
| IMessageLogger logger, CancellationToken cancellationToken) | ||
| { | ||
| _reportFormatParser ??= new ReportFormatParser(); | ||
| _coverageResult ??= new CoverageResult(); | ||
| _coverageResult.Modules ??= new Modules(); | ||
| _logger = logger; | ||
|
|
||
| string[] formats = _reportFormatParser.ParseReportFormats(configurationElement); | ||
| bool deterministic = _reportFormatParser.ParseDeterministicReport(configurationElement); | ||
| bool useSourceLink = _reportFormatParser.ParseUseSourceLink(configurationElement); | ||
| bool reportMerging = _reportFormatParser.ParseReportMerging(configurationElement); | ||
|
|
||
| AttachDebugger(); | ||
|
|
||
| if (!reportMerging) return Task.FromResult(attachments); | ||
|
|
||
| IList<IReporter> reporters = CreateReporters(formats).ToList(); | ||
|
|
||
| if (attachments.Count > 1) | ||
| { | ||
| _coverageResult.Parameters = new CoverageParameters() {DeterministicReport = deterministic, UseSourceLink = useSourceLink }; | ||
|
|
||
| var fileAttachments = attachments.SelectMany(x => x.Attachments.Where(IsFileAttachment)).ToList(); | ||
| string mergeFilePath = Path.GetDirectoryName(fileAttachments.First().Uri.LocalPath); | ||
|
|
||
| MergeExistingJsonReports(attachments); | ||
|
|
||
| RemoveObsoleteReports(fileAttachments); | ||
|
|
||
| AttachmentSet mergedFileAttachment = WriteCoverageReports(reporters, mergeFilePath, _coverageResult); | ||
|
|
||
| attachments = new List<AttachmentSet> { mergedFileAttachment }; | ||
| } | ||
|
|
||
| return Task.FromResult(attachments); | ||
| } | ||
|
|
||
| private static void RemoveObsoleteReports(List<UriDataAttachment> fileAttachments) | ||
| { | ||
| fileAttachments.ForEach(x => | ||
| { | ||
| string directory = Path.GetDirectoryName(x.Uri.LocalPath); | ||
| if (! string.IsNullOrEmpty(directory) && Directory.Exists(directory)) | ||
| Directory.Delete(directory, true); | ||
| }); | ||
| } | ||
|
|
||
| private void MergeExistingJsonReports(IEnumerable<AttachmentSet> attachments) | ||
| { | ||
| foreach (AttachmentSet attachmentSet in attachments) | ||
| { | ||
| attachmentSet.Attachments.Where(IsFileWithJsonExt).ToList().ForEach(x => | ||
| MergeWithCoverageResult(x.Uri.LocalPath, _coverageResult) | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| private AttachmentSet WriteCoverageReports(IEnumerable<IReporter> reporters, string directory, CoverageResult coverageResult) | ||
| { | ||
| var attachment = new AttachmentSet(new Uri(CoverletConstants.DefaultUri), string.Empty); | ||
| foreach (IReporter reporter in reporters) | ||
| { | ||
| string report = GetCoverageReport(coverageResult, reporter); | ||
| var file = new FileInfo(Path.Combine(directory, Path.ChangeExtension(CoverletConstants.DefaultFileName, reporter.Extension))); | ||
| file.Directory?.Create(); | ||
| File.WriteAllText(file.FullName, report); | ||
| attachment.Attachments.Add(new UriDataAttachment(new Uri(file.FullName),string.Empty)); | ||
| } | ||
| return attachment; | ||
| } | ||
|
|
||
| private static bool IsFileWithJsonExt(UriDataAttachment x) | ||
| { | ||
| return IsFileAttachment(x) && Path.GetExtension(x.Uri.AbsolutePath).Equals(".json"); | ||
| } | ||
|
|
||
| private static bool IsFileAttachment(UriDataAttachment x) | ||
| { | ||
| return x.Uri.IsFile; | ||
| } | ||
|
|
||
| private void MergeWithCoverageResult(string filePath, CoverageResult coverageResult) | ||
| { | ||
| string json = File.ReadAllText(filePath); | ||
| coverageResult.Merge(JsonConvert.DeserializeObject<Modules>(json)); | ||
| } | ||
|
|
||
| private string GetCoverageReport(CoverageResult coverageResult, IReporter reporter) | ||
| { | ||
| try | ||
| { | ||
| // empty source root translator returns the original path for deterministic report | ||
| return reporter.Report(coverageResult, new SourceRootTranslator()); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| throw new CoverletDataCollectorException( | ||
| $"{CoverletConstants.DataCollectorName}: Failed to get coverage report", ex); | ||
| } | ||
| } | ||
|
|
||
| private void AttachDebugger() | ||
| { | ||
| if (int.TryParse(Environment.GetEnvironmentVariable("COVERLET_DATACOLLECTOR_POSTPROCESSOR_DEBUG"), out int result) && result == 1) | ||
| { | ||
| Debugger.Launch(); | ||
| Debugger.Break(); | ||
| } | ||
| } | ||
|
|
||
| private IEnumerable<IReporter> CreateReporters(IEnumerable<string> formats) | ||
| { | ||
| IEnumerable<IReporter> reporters = formats.Select(format => | ||
| { | ||
| var reporterFactory = new ReporterFactory(format); | ||
| if (!reporterFactory.IsValidFormat()) | ||
| { | ||
| _logger.SendMessage(TestMessageLevel.Warning, $"Invalid report format '{format}'"); | ||
| return null; | ||
| } | ||
| return reporterFactory.CreateReporter(); | ||
| }).Where(r => r != null); | ||
|
|
||
| return reporters; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The new CoverletCoveragePostProcessor class, which is the core of this feature, lacks test coverage. Given the complexity of the attachment processing logic (merging JSON reports, handling file deletions, creating merged attachments), this class should have comprehensive unit tests to cover various scenarios such as: multiple attachments, empty attachments, malformed JSON files, missing directories, and the interaction with the merging flag.
closes #1307