Repository navigation
Fix dotCover parameter syntax - #4899
flaviolazzarini wants to merge 21 commits into
Conversation
|
@microsoft-github-policy-service agree company="exanicAG" |
381c835 to
8182caa
Compare
devlead
left a comment
There was a problem hiding this comment.
The direction looks sound and aligns with #4670, but I found one blocking regression and some medium/minor things that should be fixed before merge.
Blocking
DotCoverReport ignores outputFile in new syntax (the default path)
With UseLegacySyntax = false (default), the 3-arg API Report(sourceFile, outputFile, settings) never passes outputFile to the CLI. GenerateArguments only emits --snapshot-source and optional JsonReportOutput / XmlReportOutput from settings.
This breaks the common call pattern:
DotCoverReport(source, new FilePath("./result.xml"), new DotCoverReportSettings());Previously, this produced /Output=...; now it runs with no output path. The new-syntax tests always set OutputFile = null, so there's no coverage for the default fixture path with a non-null output file.
Suggested fix: When !UseLegacySyntax && outputFile != null, map to the appropriate new flag (e.g., --xml-report-output for default ReportType.XML, --json-report-output for ReportType.JSON). I'd also add a test for that path.
Medium
- Legacy filters are silently dropped on
cover:Scope,Filters,AttributeFilters,ProcessFilters, andDisableDefaultFiltersare ignored in the new syntax. Scripts using.WithFilter(...)without.WithLegacySyntax()will lose filters with no warning. I'd document this in the release notes and consider a runtime warning when legacy filter collections are populated, butUseLegacySyntaxis false. - HTML / DetailedXML / NDependXML are legacy-only: New
reportsyntax only supports--json-report-outputand--xml-report-output. The newDotCoverReportalias example still showsReportType = HTML, which won't work without legacy syntax.
Minor
DotCoverReportSettings:XmlReportOutput/XmlReportCoveringTestsScopeXML comments say "JSON report" (looks like copy-paste).DotCoverReporter.Report(source, settings)XML doc says "Cover" instead of "Report".- Test regions: typo "Paramter" → "Parameter".
- No
DotCoverMergealias for optional output (tool-level overload exists). analyseunchanged, fine with me if that's acknowledged and the command is deprecated in dotCover 2025.2+.
What looks good
merge/reportnew syntax (--snapshot-source,--snapshot-output,--temporary-directory,--log-file) looks correct to me.- Moving
UseLegacySyntaxto baseDotCoverSettingsmakes sense. - Lowercase command names match the 2025.2 convention.
- Good legacy vs new test split; build and unit tests pass locally on my machine.
Summary
| Area | My take |
|---|---|
merge |
Good |
report |
Please fix outputFile regression |
cover |
OK, I'd document filter migration |
| Tests | Missing main report output case |
devlead
left a comment
There was a problem hiding this comment.
Thanks for addressing the June review. The outputFile mapping, filter warnings, XML examples, comment typos, and merge-without-output overloads look good.
A few remaining items before merge:
Blocking
analyse+LogFile:GetArgumentsnow emits--log-fileunlessUseLegacySyntaxis true.DotCoverAnalysestill always uses legacy/Target*/Outputflags, so the default path mixes syntax. The analyser test was updated to setUseLegacySyntax = truerather than keeping analyse on/LogFile.- Unsupported
ReportType:HTML/DetailedXML/NDependXMLsilently map to--xml-report-output. - Settings mutation: mapping
outputFilewrites into the caller'sXmlReportOutput/JsonReportOutput.
Medium
DotCoverMerge(sourceFiles, settings)is missing[CakeMethodAlias](and the usual null checks), so Cake scripts will not see that overload.- The new 2-arg report example uses
DotCoverReportType.Xml(invalid; enum isXML). WithLegacySyntax()is still cover-only even though the flag now lives onDotCoverSettings.- The existing
DotCoverCoverexample still uses.WithFilter(...)without.WithLegacySyntax()or--exclude-assemblies, so it documents the drop-and-warn path.
Process
- CI on this PR is red (looks like the original June run). Please re-run after the follow-up.
- Please call out in release notes: default 2025.2 syntax, filter →
--exclude-*, report types XML/JSON only, optional merge output,analyseremaining legacy-only.
merge / report new flags look correct against current JetBrains docs.
|
@devlead thank you for the thorough review. I was finally able to make the requested changes |
# Conflicts: # src/Cake.Common.Tests/Unit/Tools/DotCover/Cover/DotCoverCovererTests.cs
31da4a1 to
3537c34
Compare
| if (settings.UseLegacySyntax) | ||
| { | ||
| if (settings.UseLegacySyntax) | ||
| { |
There was a problem hiding this comment.
settings.UseLegacySyntax duplicated
| IEnumerable<FilePath> sourceFiles, | ||
| DotCoverMergeSettings settings) | ||
| { | ||
| var merger = new DotCoverMerger( |
There was a problem hiding this comment.
Add ArgumentNullException.ThrowIfNull(context); ArgumentNullException.ThrowIfNull(settings);
| /// </summary> | ||
| /// <param name="settings">The settings.</param> | ||
| /// <param name="useLegacySyntax">Whether to use legacy syntax. Default is false (new syntax).</param> | ||
| /// <returns>The same <see cref="DotCoverCoverSettings"/> instance so that multiple calls can be chained.</returns> |
There was a problem hiding this comment.
Type should be DotCoverSettings
As of dotCover 2025.2 JetBrains made significant changes to the parameter syntax of dotCover. Some changes where already made in #4670. Unfortunately not all commands and parameters have been covered. This pull request adds compatibility for the following commands:
mergereportanalysehas not been covered, as it's not a supported command anymore.For a complete list of the commands and their parameters see https://www.jetbrains.com/help/dotcover/dotCover__Console_Runner_Commands.html#help
This fixes #4096