-
Notifications
You must be signed in to change notification settings - Fork 398
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
base: master
Are you sure you want to change the base?
Changes from all commits
9627e72
a6966ba
453d589
00d38bb
7a3a3ce
62e4f58
44785d9
92b0fea
1b912e4
f67b976
c6f5f0e
3e40ef5
ada8b1e
ea1cb6d
5b29253
249ed87
988cb9e
1b7d207
8281be7
d67bb46
90768d6
69777e9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,162 @@ | ||||||||||||||||||||||||||||||||||
| // 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) | ||||||||||||||||||||||||||||||||||
|
Comment on lines
+34
to
+36
|
||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||
| _reportFormatParser ??= new ReportFormatParser(); | ||||||||||||||||||||||||||||||||||
| _coverageResult ??= new CoverageResult(); | ||||||||||||||||||||||||||||||||||
| _coverageResult.Modules ??= new Modules(); | ||||||||||||||||||||||||||||||||||
| _logger = logger; | ||||||||||||||||||||||||||||||||||
|
Comment on lines
+38
to
+41
|
||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| 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); | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
|
Comment on lines
+59
to
+60
|
||||||||||||||||||||||||||||||||||
| 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); |
Copilot
AI
Feb 23, 2026
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.
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)) |
Copilot
AI
Jun 9, 2025
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.
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."); | |
| } | |
| } |
Copilot
AI
Feb 23, 2026
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.
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); |
Copilot
AI
Feb 23, 2026
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.
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); | |
| } |
Copilot
AI
Feb 23, 2026
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.
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.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -26,24 +26,32 @@ internal class CoverageManager | |||||||||||||||||||
| public CoverageManager(CoverletSettings settings, TestPlatformEqtTrace eqtTrace, TestPlatformLogger logger, ICoverageWrapper coverageWrapper, | ||||||||||||||||||||
| IInstrumentationHelper instrumentationHelper, IFileSystem fileSystem, ISourceRootTranslator sourceRootTranslator, ICecilSymbolHelper cecilSymbolHelper) | ||||||||||||||||||||
| : this(settings, | ||||||||||||||||||||
| settings.ReportFormats.Select(format => | ||||||||||||||||||||
| { | ||||||||||||||||||||
| var reporterFactory = new ReporterFactory(format); | ||||||||||||||||||||
| if (!reporterFactory.IsValidFormat()) | ||||||||||||||||||||
| { | ||||||||||||||||||||
| eqtTrace.Warning($"Invalid report format '{format}'"); | ||||||||||||||||||||
| return null; | ||||||||||||||||||||
| } | ||||||||||||||||||||
| else | ||||||||||||||||||||
| { | ||||||||||||||||||||
| return reporterFactory.CreateReporter(); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| }).Where(r => r != null).ToArray(), | ||||||||||||||||||||
| CreateReporters(settings, eqtTrace), | ||||||||||||||||||||
| new CoverletLogger(eqtTrace, logger), | ||||||||||||||||||||
| coverageWrapper, instrumentationHelper, fileSystem, sourceRootTranslator, cecilSymbolHelper) | ||||||||||||||||||||
| { | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| private static IReporter[] CreateReporters(CoverletSettings settings, TestPlatformEqtTrace eqtTrace) | ||||||||||||||||||||
| { | ||||||||||||||||||||
| if (settings.ReportMerging && ! settings.ReportFormats.Contains("json")) | ||||||||||||||||||||
|
||||||||||||||||||||
| if (settings.ReportMerging && ! settings.ReportFormats.Contains("json")) | |
| if (settings.ReportMerging && !settings.ReportFormats.Contains("json")) |
Copilot
AI
Feb 23, 2026
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.
String comparison is case-sensitive. The check for "json" format won't match if the user specifies "JSON" or "Json" in the configuration. Since report format names in coverlet are consistently lowercase (as seen in the reporter classes), consider using a case-insensitive comparison or converting the format to lowercase before comparison: settings.ReportFormats.Any(f => f.Equals("json", StringComparison.OrdinalIgnoreCase)).
| if (settings.ReportMerging && ! settings.ReportFormats.Contains("json")) | |
| if (settings.ReportMerging && !settings.ReportFormats.Any(f => f.Equals("json", StringComparison.OrdinalIgnoreCase))) |
Copilot
AI
Jun 9, 2025
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.
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 => |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| // 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.Xml; | ||
| using System.Linq; | ||
|
|
||
| namespace Coverlet.Collector.Utilities | ||
| { | ||
| internal class ReportFormatParser | ||
| { | ||
| internal string[] ParseReportFormats(XmlElement configurationElement) | ||
| { | ||
| string[] formats = Array.Empty<string>(); | ||
| if (configurationElement != null) | ||
| { | ||
| XmlElement reportFormatElement = configurationElement[CoverletConstants.ReportFormatElementName]; | ||
| formats = SplitElement(reportFormatElement); | ||
| } | ||
|
|
||
| return formats is null || formats.Length == 0 ? new[] { CoverletConstants.DefaultReportFormat } : formats; | ||
| } | ||
|
|
||
| private static string[] SplitElement(XmlElement element) | ||
| { | ||
| return element?.InnerText?.Split(new char[] { ',' }, StringSplitOptions.RemoveEmptyEntries).Where(value => !string.IsNullOrWhiteSpace(value)).Select(value => value.Trim()).ToArray(); | ||
| } | ||
|
|
||
| internal bool ParseUseSourceLink(XmlElement configurationElement) | ||
| { | ||
| XmlElement useSourceLinkElement = configurationElement[CoverletConstants.UseSourceLinkElementName]; | ||
| bool.TryParse(useSourceLinkElement?.InnerText, out bool useSourceLink); | ||
| return useSourceLink; | ||
| } | ||
|
|
||
| internal bool ParseDeterministicReport(XmlElement configurationElement) | ||
| { | ||
| XmlElement deterministicReportElement = configurationElement[CoverletConstants.DeterministicReport]; | ||
| bool.TryParse(deterministicReportElement?.InnerText, out bool deterministicReport); | ||
| return deterministicReport; | ||
| } | ||
|
|
||
| internal bool ParseReportMerging(XmlElement configurationElement) | ||
| { | ||
| XmlElement mergeWithElement = configurationElement[CoverletConstants.ReportMerging]; | ||
| bool.TryParse(mergeWithElement?.InnerText, out bool mergeWith); | ||
| return mergeWith; | ||
| } | ||
| } | ||
| } | ||
|
Comment on lines
+1
to
+50
|
||
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.
Typo in documentation: "uses" should be "used". The sentence should read "Post processing is used for automatically merging coverage reports..."