Fix dotCover parameter syntax#4899
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 |
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