test: verify expected license type from mapping in UrlToLicenseMapping tests#494
Closed
vincent067 wants to merge 3 commits into
Closed
test: verify expected license type from mapping in UrlToLicenseMapping tests#494vincent067 wants to merge 3 commits into
vincent067 wants to merge 3 commits into
Conversation
This commit adds support for CSV output format, which is useful for: - Importing license data into Excel - Compliance and audit reporting - Feeding data into internal tools and CI/CD pipelines Changes: - Added Csv to OutputType enum - Created CsvOutputFormatter class with proper escaping - Updated CommandLineOptionsParser to handle CSV format - Updated help text to include CSV option Closes sensslen#423
This commit adds the ability to filter which columns are displayed in the output, addressing issue sensslen#331. New features: - Added --include-columns (-c) option to specify which columns to display - Supports semicolon-separated list of column names - Works with Table, Markdown, and CSV output formats - Available columns: Package, Version, LicenseInformationOrigin, LicenseExpression, LicenseUrl, Copyright, Authors, PackageProjectUrl, Error, ErrorContext Example usage: nuget-license -i project.csproj -c Package
…g tests The test now verifies that the license content fetched from the URL actually matches the expected license type defined in the mapping. This addresses issue sensslen#492 where tests only verified content availability but did not validate that the content corresponds to the expected license type. Changes: - Modified CompareLicense to accept expectedLicenseType parameter - Added VerifyLicenseType method to validate license content against expected type - Updated test method to pass the mapped license value to the comparer Fixes sensslen#492
📝 WalkthroughWalkthroughThis PR extends the output formatting system to support CSV export with selective column inclusion. It adds an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 4❌ Failed checks (4 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/NuGetUtility.UrlToLicenseMapping.Test/UrlToLicenseMappingTest.cs (1)
143-158: Consider failing explicitly for unhandled license types instead of defaulting totrue.The
_ => truecatch-all allows unknown license types to pass validation silently. While all currently mapped license types (MIT, Apache-2.0, BSD-3-Clause, MS-PL) are covered, if someone adds a new mapping usingLicenseExpressions.Gpl20,Gpl30, orBsd20fromLicenseExpressions.cs, the test would pass without actual validation—exactly the false positive scenario issue#492aimed to prevent.♻️ Proposed fix to fail fast on unknown license types
"MS-PL" => normalizedContent.Contains("MICROSOFT") || normalizedContent.Contains("MS-PL") || normalizedContent.Contains("MICROSOFT PUBLIC LICENSE"), - _ => true // If we don't have specific validation for this license type, allow it to pass + _ => throw new NotSupportedException($"No validation pattern defined for license type: {expectedLicenseType}. Please add a case to VerifyLicenseType.") };This ensures that adding new license mappings without corresponding validation patterns causes an immediate, actionable test failure rather than a silent pass.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/NuGetUtility.UrlToLicenseMapping.Test/UrlToLicenseMappingTest.cs` around lines 143 - 158, The switch's catch-all currently returns true, allowing unknown license types to silently pass; update the switch on expectedLicenseType (which inspects normalizedContent) to fail explicitly for unhandled types—e.g., replace the default arm (_ => true) with a failing result (return false or throw an InvalidOperationException/assertion) so any new mapping (e.g., LicenseExpressions.Gpl20/Gpl30/Bsd20) without a corresponding validation case causes the test to fail and forces adding a proper validation branch.src/NuGetLicense/CommandLineOptionsParser.cs (1)
180-195: Consider deduplicating semicolon-list parsing logic.
ParseIncludedColumnsand the inline-list branch inParseStringArrayOrFilenow implement near-identical splitting/trimming behavior, which can drift over time.Possible refactor
+ private static string[] ParseSemicolonSeparatedValues(string value) + { + string[] parts = value.Split([';'], StringSplitOptions.RemoveEmptyEntries); + for (int i = 0; i < parts.Length; i++) + { + parts[i] = parts[i].Trim(); + } + return Array.FindAll(parts, part => part.Length > 0); + } + private string[] ParseStringArrayOrFile(string? value) { if (value is null) { return Array.Empty<string>(); } @@ - string[] parts = value.Split([';'], StringSplitOptions.RemoveEmptyEntries); - // Trim each part manually for .NET Framework compatibility - for (int i = 0; i < parts.Length; i++) - { - parts[i] = parts[i].Trim(); - } - return Array.FindAll(parts, part => part.Length > 0); + return ParseSemicolonSeparatedValues(value); } private string[] ParseIncludedColumns(string? value) { if (value is null) { return Array.Empty<string>(); } - // Parse as semicolon-separated inline values - string[] parts = value.Split([';'], StringSplitOptions.RemoveEmptyEntries); - // Trim each part manually for .NET Framework compatibility - for (int i = 0; i < parts.Length; i++) - { - parts[i] = parts[i].Trim(); - } - return Array.FindAll(parts, part => part.Length > 0); + return ParseSemicolonSeparatedValues(value); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NuGetLicense/CommandLineOptionsParser.cs` around lines 180 - 195, ParseIncludedColumns duplicates the semicolon-splitting/trimming logic found in the inline-list branch of ParseStringArrayOrFile; extract that logic into a single helper (e.g., SplitAndTrimSemicolonList or ParseSemicolonSeparatedValues) that accepts a nullable string and returns string[] (splits on ';', trims each entry, removes empties) while preserving the manual Trim loop for .NET Framework compatibility, then replace the body of ParseIncludedColumns and the inline-list branch in ParseStringArrayOrFile to call that helper.src/NuGetLicense/Output/Csv/CsvOutputFormatter.cs (1)
55-57: Normalize included column names consistently with table output matching.Table output strips spaces before matching, but CSV currently requires exact key names. The same CLI input can behave differently across formats.
Proposed refactor
- var columnsToInclude = _includedColumns.Length > 0 - ? allColumns.Where(c => _includedColumns.Contains(c.Key, StringComparer.OrdinalIgnoreCase)).ToList() - : allColumns.ToList(); + static string Normalize(string value) => value.Replace(" ", "", StringComparison.Ordinal); + HashSet<string> requested = _includedColumns + .Select(Normalize) + .ToHashSet(StringComparer.OrdinalIgnoreCase); + + var columnsToInclude = _includedColumns.Length > 0 + ? allColumns.Where(c => requested.Contains(Normalize(c.Key))).ToList() + : allColumns.ToList();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NuGetLicense/Output/Csv/CsvOutputFormatter.cs` around lines 55 - 57, The CSV formatter currently matches _includedColumns against column keys exactly whereas the table output strips spaces; update the columnsToInclude logic in CsvOutputFormatter (the block that defines columnsToInclude) to normalize both sides the same way the table does: create a normalizedIncluded collection by trimming/stripping spaces and applying case-insensitivity to each entry from _includedColumns, and then filter allColumns by comparing a similarly normalized c.Key (remove spaces and use OrdinalIgnoreCase) against that normalizedIncluded set so CSV and table formats behave identically.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/NuGetLicense/Output/Csv/CsvOutputFormatter.cs`:
- Around line 39-63: The CSV column map in CsvOutputFormatter (the allColumns
Dictionary<string, Func<LicenseValidationResult, string?>>) is missing the
"PackageProjectUrl" key, so "--include-columns PackageProjectUrl" doesn't match;
add an entry like ["PackageProjectUrl"] = r => r.PackageProjectUrl (or the
appropriate property on LicenseValidationResult) to the allColumns initializer
so the existing include/filter logic (_includedColumns, columnsToInclude) will
recognize and output that column consistently with other formats.
In `@src/NuGetLicense/Program.cs`:
- Around line 71-72: The CLI help text for the IncludedColumns option uses a
non-existent column token "License"; update the Option attribute Description on
the IncludedColumns property so the example uses the correct column key
"LicenseExpression" (e.g., "Package;Version;LicenseExpression") and ensure the
Available columns list still includes LicenseExpression; modify the Description
string in the IncludedColumns property to reflect this replacement.
---
Nitpick comments:
In `@src/NuGetLicense/CommandLineOptionsParser.cs`:
- Around line 180-195: ParseIncludedColumns duplicates the
semicolon-splitting/trimming logic found in the inline-list branch of
ParseStringArrayOrFile; extract that logic into a single helper (e.g.,
SplitAndTrimSemicolonList or ParseSemicolonSeparatedValues) that accepts a
nullable string and returns string[] (splits on ';', trims each entry, removes
empties) while preserving the manual Trim loop for .NET Framework compatibility,
then replace the body of ParseIncludedColumns and the inline-list branch in
ParseStringArrayOrFile to call that helper.
In `@src/NuGetLicense/Output/Csv/CsvOutputFormatter.cs`:
- Around line 55-57: The CSV formatter currently matches _includedColumns
against column keys exactly whereas the table output strips spaces; update the
columnsToInclude logic in CsvOutputFormatter (the block that defines
columnsToInclude) to normalize both sides the same way the table does: create a
normalizedIncluded collection by trimming/stripping spaces and applying
case-insensitivity to each entry from _includedColumns, and then filter
allColumns by comparing a similarly normalized c.Key (remove spaces and use
OrdinalIgnoreCase) against that normalizedIncluded set so CSV and table formats
behave identically.
In `@tests/NuGetUtility.UrlToLicenseMapping.Test/UrlToLicenseMappingTest.cs`:
- Around line 143-158: The switch's catch-all currently returns true, allowing
unknown license types to silently pass; update the switch on expectedLicenseType
(which inspects normalizedContent) to fail explicitly for unhandled types—e.g.,
replace the default arm (_ => true) with a failing result (return false or throw
an InvalidOperationException/assertion) so any new mapping (e.g.,
LicenseExpressions.Gpl20/Gpl30/Bsd20) without a corresponding validation case
causes the test to fail and forces adding a proper validation branch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4eb23be4-5673-4ad6-9cdc-3c9cc8630337
📒 Files selected for processing (9)
src/NuGetLicense/CommandLineOptionsParser.cssrc/NuGetLicense/ICommandLineOptions.cssrc/NuGetLicense/ICommandLineOptionsParser.cssrc/NuGetLicense/LicenseValidationOrchestrator.cssrc/NuGetLicense/Output/Csv/CsvOutputFormatter.cssrc/NuGetLicense/Output/Table/TableOutputFormatter.cssrc/NuGetLicense/Program.cssrc/NuGetUtility/OutputType.cstests/NuGetUtility.UrlToLicenseMapping.Test/UrlToLicenseMappingTest.cs
Comment on lines
+39
to
+63
| var allColumns = new Dictionary<string, Func<LicenseValidationResult, string?>>(StringComparer.OrdinalIgnoreCase) | ||
| { | ||
| ["Package"] = r => r.PackageId, | ||
| ["Version"] = r => r.PackageVersion.ToString(), | ||
| ["LicenseInformationOrigin"] = r => r.LicenseInformationOrigin.ToString(), | ||
| ["LicenseExpression"] = r => r.License, | ||
| ["LicenseUrl"] = r => r.LicenseUrl, | ||
| ["Copyright"] = r => r.Copyright, | ||
| ["Authors"] = r => r.Authors, | ||
| ["Description"] = r => r.Description, | ||
| ["Summary"] = r => r.Summary, | ||
| ["Error"] = r => r.ValidationErrors.Any() ? string.Join("; ", r.ValidationErrors.Select(e => e.Error)) : null, | ||
| ["ErrorContext"] = r => r.ValidationErrors.Any() ? string.Join("; ", r.ValidationErrors.Select(e => e.Context)) : null | ||
| }; | ||
|
|
||
| // Determine which columns to include | ||
| var columnsToInclude = _includedColumns.Length > 0 | ||
| ? allColumns.Where(c => _includedColumns.Contains(c.Key, StringComparer.OrdinalIgnoreCase)).ToList() | ||
| : allColumns.ToList(); | ||
|
|
||
| // If no valid columns specified, use all columns | ||
| if (columnsToInclude.Count == 0) | ||
| { | ||
| columnsToInclude = allColumns.ToList(); | ||
| } |
There was a problem hiding this comment.
PackageProjectUrl is missing from CSV column definitions, breaking a documented include-column value.
--include-columns PackageProjectUrl currently yields no match here and can fall back to all columns, which is surprising. Program advertises this column, and table output supports it.
Proposed fix
var allColumns = new Dictionary<string, Func<LicenseValidationResult, string?>>(StringComparer.OrdinalIgnoreCase)
{
["Package"] = r => r.PackageId,
["Version"] = r => r.PackageVersion.ToString(),
["LicenseInformationOrigin"] = r => r.LicenseInformationOrigin.ToString(),
["LicenseExpression"] = r => r.License,
["LicenseUrl"] = r => r.LicenseUrl,
+ ["PackageProjectUrl"] = r => r.PackageProjectUrl,
["Copyright"] = r => r.Copyright,
["Authors"] = r => r.Authors,
["Description"] = r => r.Description,
["Summary"] = r => r.Summary,
["Error"] = r => r.ValidationErrors.Any() ? string.Join("; ", r.ValidationErrors.Select(e => e.Error)) : null,
["ErrorContext"] = r => r.ValidationErrors.Any() ? string.Join("; ", r.ValidationErrors.Select(e => e.Context)) : null
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var allColumns = new Dictionary<string, Func<LicenseValidationResult, string?>>(StringComparer.OrdinalIgnoreCase) | |
| { | |
| ["Package"] = r => r.PackageId, | |
| ["Version"] = r => r.PackageVersion.ToString(), | |
| ["LicenseInformationOrigin"] = r => r.LicenseInformationOrigin.ToString(), | |
| ["LicenseExpression"] = r => r.License, | |
| ["LicenseUrl"] = r => r.LicenseUrl, | |
| ["Copyright"] = r => r.Copyright, | |
| ["Authors"] = r => r.Authors, | |
| ["Description"] = r => r.Description, | |
| ["Summary"] = r => r.Summary, | |
| ["Error"] = r => r.ValidationErrors.Any() ? string.Join("; ", r.ValidationErrors.Select(e => e.Error)) : null, | |
| ["ErrorContext"] = r => r.ValidationErrors.Any() ? string.Join("; ", r.ValidationErrors.Select(e => e.Context)) : null | |
| }; | |
| // Determine which columns to include | |
| var columnsToInclude = _includedColumns.Length > 0 | |
| ? allColumns.Where(c => _includedColumns.Contains(c.Key, StringComparer.OrdinalIgnoreCase)).ToList() | |
| : allColumns.ToList(); | |
| // If no valid columns specified, use all columns | |
| if (columnsToInclude.Count == 0) | |
| { | |
| columnsToInclude = allColumns.ToList(); | |
| } | |
| var allColumns = new Dictionary<string, Func<LicenseValidationResult, string?>>(StringComparer.OrdinalIgnoreCase) | |
| { | |
| ["Package"] = r => r.PackageId, | |
| ["Version"] = r => r.PackageVersion.ToString(), | |
| ["LicenseInformationOrigin"] = r => r.LicenseInformationOrigin.ToString(), | |
| ["LicenseExpression"] = r => r.License, | |
| ["LicenseUrl"] = r => r.LicenseUrl, | |
| ["PackageProjectUrl"] = r => r.PackageProjectUrl, | |
| ["Copyright"] = r => r.Copyright, | |
| ["Authors"] = r => r.Authors, | |
| ["Description"] = r => r.Description, | |
| ["Summary"] = r => r.Summary, | |
| ["Error"] = r => r.ValidationErrors.Any() ? string.Join("; ", r.ValidationErrors.Select(e => e.Error)) : null, | |
| ["ErrorContext"] = r => r.ValidationErrors.Any() ? string.Join("; ", r.ValidationErrors.Select(e => e.Context)) : null | |
| }; | |
| // Determine which columns to include | |
| var columnsToInclude = _includedColumns.Length > 0 | |
| ? allColumns.Where(c => _includedColumns.Contains(c.Key, StringComparer.OrdinalIgnoreCase)).ToList() | |
| : allColumns.ToList(); | |
| // If no valid columns specified, use all columns | |
| if (columnsToInclude.Count == 0) | |
| { | |
| columnsToInclude = allColumns.ToList(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/NuGetLicense/Output/Csv/CsvOutputFormatter.cs` around lines 39 - 63, The
CSV column map in CsvOutputFormatter (the allColumns Dictionary<string,
Func<LicenseValidationResult, string?>>) is missing the "PackageProjectUrl" key,
so "--include-columns PackageProjectUrl" doesn't match; add an entry like
["PackageProjectUrl"] = r => r.PackageProjectUrl (or the appropriate property on
LicenseValidationResult) to the allColumns initializer so the existing
include/filter logic (_includedColumns, columnsToInclude) will recognize and
output that column consistently with other formats.
Comment on lines
+71
to
+72
| [Option("-c|--include-columns", Description = "Specifies which columns to include in the output. Provide a semicolon-separated list of column names (e.g., \"Package;Version;License\"). Available columns: Package, Version, LicenseInformationOrigin, LicenseExpression, LicenseUrl, Copyright, Authors, PackageProjectUrl, Error, ErrorContext. If omitted, all relevant columns are shown.")] | ||
| public string? IncludedColumns { get; set; } |
There was a problem hiding this comment.
CLI help example uses a non-existent column token.
The example shows License, but the implemented column key is LicenseExpression. This can cause user confusion and failed filtering attempts.
Proposed wording fix
- [Option("-c|--include-columns", Description = "Specifies which columns to include in the output. Provide a semicolon-separated list of column names (e.g., \"Package;Version;License\"). Available columns: Package, Version, LicenseInformationOrigin, LicenseExpression, LicenseUrl, Copyright, Authors, PackageProjectUrl, Error, ErrorContext. If omitted, all relevant columns are shown.")]
+ [Option("-c|--include-columns", Description = "Specifies which columns to include in the output. Provide a semicolon-separated list of column names (e.g., \"Package;Version;LicenseExpression\"). Available columns: Package, Version, LicenseInformationOrigin, LicenseExpression, LicenseUrl, Copyright, Authors, PackageProjectUrl, Error, ErrorContext. If omitted, all relevant columns are shown.")]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| [Option("-c|--include-columns", Description = "Specifies which columns to include in the output. Provide a semicolon-separated list of column names (e.g., \"Package;Version;License\"). Available columns: Package, Version, LicenseInformationOrigin, LicenseExpression, LicenseUrl, Copyright, Authors, PackageProjectUrl, Error, ErrorContext. If omitted, all relevant columns are shown.")] | |
| public string? IncludedColumns { get; set; } | |
| [Option("-c|--include-columns", Description = "Specifies which columns to include in the output. Provide a semicolon-separated list of column names (e.g., \"Package;Version;LicenseExpression\"). Available columns: Package, Version, LicenseInformationOrigin, LicenseExpression, LicenseUrl, Copyright, Authors, PackageProjectUrl, Error, ErrorContext. If omitted, all relevant columns are shown.")] | |
| public string? IncludedColumns { get; set; } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/NuGetLicense/Program.cs` around lines 71 - 72, The CLI help text for the
IncludedColumns option uses a non-existent column token "License"; update the
Option attribute Description on the IncludedColumns property so the example uses
the correct column key "LicenseExpression" (e.g.,
"Package;Version;LicenseExpression") and ensure the Available columns list still
includes LicenseExpression; modify the Description string in the IncludedColumns
property to reflect this replacement.
Owner
|
@vincent067 thank you for your contribution but I cannot accept this PR as 1. it contains a lot of code from a prior PR of yours that I will not accept as csv output support is already in main and released. 2. This test does not add anything. The expected license texts are already included as text files in the repository. Adding this code just complicates maintanance and does not improve test coverage. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR addresses issue #492 by adding verification that the license content fetched from URLs actually matches the expected license type defined in the
UrlToLicenseMapping.Problem
The existing tests verified that license URLs were accessible and returned content, but they did not validate that the content corresponds to the expected license type (e.g., MIT, Apache-2.0) as defined in the mapping.
Solution
CompareLicensemethod to accept theexpectedLicenseTypeparameterVerifyLicenseTypemethod that validates license content against known patterns for each license typeLicense Type Validation
The new validation checks for:
Testing
The existing verified snapshots continue to work, and the additional license type validation ensures the mapping is correct.
Fixes #492
Summary by CodeRabbit
Release Notes
New Features
--include-columnscommand-line option to filter displayed columns in outputTests