Bumps npm dependencies and DevSkim .NET dependencies, Fix #697 #698
Conversation
Introduces SarifWriterTests to verify SARIF help field population for rules with various combinations of recommendation, description, and rule info. Tests cover fallback logic, markdown formatting, and edge cases such as empty or whitespace recommendations.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
danfiedler-msft
left a comment
There was a problem hiding this comment.
A few small suggestions and nits to take or leave; overall LGTM.
Extracted markdown description building into a new BuildMarkdownDescription method for improved readability and maintainability. The logic for constructing the SARIF rule's Help.Markdown field is now encapsulated in a dedicated helper function.
Replaces class-level StringWriter and SarifWriter fields with local variables in each test method. This improves test isolation and resource management by using 'using' statements for disposable objects.
Replaced calls to the ParseSarifOutput helper with direct usage of JObject.Parse in SarifWriterTests. Removed the now-unused ParseSarifOutput method for simplification.
Test method names in SarifWriterTests.cs were updated to use descriptive, behavior-driven naming. This improves readability and makes test purposes clearer for future maintenance.
Eliminated the Patterns property from the test case object initialization in SarifWriterTests.cs, as it was not required for the test.
Moved the logic for building SARIF rule text descriptions into a dedicated BuildTextDescription method for improved readability and maintainability.
Introduces a unit test to verify that when a rule has no recommendation and no rule info, the SARIF 'help.markdown' field is empty or null, while 'help.text' falls back to the rule description.
Added details for version 1.0.63 including a fix for Sarif Markdown recommendation value population (#697), new test cases for SarifWriter, and updated dependencies. Fixed some section header levels to improve formatting.
| "integrity": "sha1-9y12C+Cbf3bQjtj66Ysomo0F+rM=", | ||
| "dev": true | ||
| }, | ||
| "node_modules/brace-expansion": { |
There was a problem hiding this comment.
Question: Should we create a follow-up issue to change the other dependencies to use the artifact feed (rather than npm registry) as well? (and same for DevSkim-VSCode-Plugin/package-lock.json)
There was a problem hiding this comment.
No. It is intentional that the package-lock.json uses public sources when checked in - using the private feed would render the VS Code extention impossible for external contributors to build/reproduce without blowing up or modifying the package-lock.json, which obviates its purpose. It also would downgrade the integrity values in the package-lock.json to sha1 due to a longstanding issue with azure artifact feeds: https://developercommunity.visualstudio.com/t/npm-packages-from-azure-artifact-defaulting-to-sha/957522
As part of the previous work to enable the .npmrc in I added extra steps to the pipeline build that swap out npmjs.org with the private feed before building in the pipeline.
There is a special task in the package.json that performs the base uri swapping before building:
DevSkim/DevSkim-VSCode-Plugin/package.json
Line 318 in 6afdd9b
By calling this script: https://github.com/microsoft/DevSkim/blob/main/DevSkim-VSCode-Plugin/scripts/updatePackageLock.js
There was a problem hiding this comment.
I see now what you meant, good catch, it seems that only the packages in the root level but not the client folder had been updated with the internal feed. This was inadvertent. I intended for all packages in the checked in package-lock to continue using npmjs.org.
Deleted the test 'When_rule_has_recommendation_and_rule_info_then_markdown_includes_both' from SarifWriterTests.cs as it was redundant with `When_rule_has_recommendation_and_rule_info_then_markdown_is_properly_formatted`
The internal repository is substituted during pipeline build to allow for external contributor use
Introduced a CreateHelpUri method in SarifWriter to safely construct help URIs for DevSkim rules, handling null or empty RuleInfo values. Updated related unit tests to use the new baseHelpUri constant for consistency and maintainability.
Replaces references to SarifWriter.baseHelpUri with SarifWriter.CreateHelpUri in SarifWriterTests to ensure help URIs are generated consistently. This improves test accuracy and future-proofs against changes in URI construction.
Updated the constant baseHelpUri to use PascalCase (BaseHelpUri) for consistency with naming conventions. Adjusted references to the constant accordingly.
Changed tests to use Assert.AreEqual with expected markdown and help text strings instead of Assert.IsTrue with Contains. This ensures stricter validation of the output format.
Replaces repeated calls to SarifWriter.CreateHelpUri with a local helpUri variable in test assertions for expectedMarkdown. This improves readability and reduces redundant method calls.
Updated the visibility of the BaseHelpUri constant from public to private in SarifWriter.cs to restrict its access within the class.
The CreateHelpUri method in SarifWriter was changed from public to internal to restrict its visibility within the assembly. This helps encapsulate implementation details and limits external usage.
There was a problem hiding this comment.
Pull Request Overview
This pull request fixes a bug in the DevSkim SARIF output where recommendations were not properly included in the SARIF Help field, updates npm and .NET dependencies, and adds comprehensive test coverage.
- Fixes SARIF markdown value failing to populate rule recommendation value (#697)
- Updates
brace-expansiondependency versions in package-lock.json files - Adds extensive test cases for SarifWriter to validate recommendation and help field handling
Reviewed Changes
Copilot reviewed 3 out of 5 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| DevSkim-DotNet/Microsoft.DevSkim.Tests/SarifWriterTests.cs | Adds comprehensive test suite for SarifWriter with 12 test methods covering various recommendation and help text scenarios |
| DevSkim-DotNet/Microsoft.DevSkim.CLI/Writers/SarifWriter.cs | Refactors SARIF help text generation logic into separate methods and fixes recommendation handling |
| Changelog.md | Documents version 1.0.63 changes including the bug fix, dependency updates, and new test cases |
Files not reviewed (2)
- DevSkim-VSCode-Plugin/client/package-lock.json: Language not supported
- DevSkim-VSCode-Plugin/package-lock.json: Language not supported
Comments suppressed due to low confidence (2)
DevSkim-DotNet/Microsoft.DevSkim.Tests/SarifWriterTests.cs:168
- This test case creates a rule with null description but the test name and assertion suggest it should test the fallback behavior when both recommendation and description are missing. Consider using an empty string or ensuring the test validates the actual fallback logic.
var rule = CreateTestRule("TEST007", "Test Rule", null, null, ruleInfo);
DevSkim-DotNet/Microsoft.DevSkim.CLI/Writers/SarifWriter.cs:372
- [nitpick] The variable name 'markdownDescriptionBuilder' is verbose. Consider shortening it to 'markdownBuilder' for better readability.
StringBuilder markdownDescriptionBuilder = new();
| var baseUri = new Uri(BaseHelpUri); | ||
|
|
||
| if (string.IsNullOrEmpty(ruleInfo)) | ||
| { | ||
| // Return base URI if no specific rule info is provided | ||
| return baseUri; | ||
| } | ||
|
|
||
| // Use Uri constructor to safely combine base URI with relative path | ||
| return new Uri(baseUri, ruleInfo); | ||
| } | ||
|
|
||
| /// <summary> |
There was a problem hiding this comment.
Creating a new Uri object for the base URI on every method call is inefficient. Consider making this a static readonly field to avoid repeated object creation.
| var baseUri = new Uri(BaseHelpUri); | |
| if (string.IsNullOrEmpty(ruleInfo)) | |
| { | |
| // Return base URI if no specific rule info is provided | |
| return baseUri; | |
| } | |
| // Use Uri constructor to safely combine base URI with relative path | |
| return new Uri(baseUri, ruleInfo); | |
| } | |
| /// <summary> | |
| if (string.IsNullOrEmpty(ruleInfo)) | |
| { | |
| // Return base URI if no specific rule info is provided | |
| return BaseUri; | |
| } | |
| // Use Uri constructor to safely combine base URI with relative path | |
| return new Uri(BaseUri, ruleInfo); | |
| } | |
| /// <summary> | |
| /// Cached base URI for help links | |
| /// </summary> | |
| private static readonly Uri BaseUri = new Uri(BaseHelpUri); | |
| /// <summary> |
Added InternalsVisibleTo for Microsoft.DevSkim.Tests in the CLI project file to allow unit testing of internal members. Changed CreateHelpUri from public to internal in SarifWriter to restrict its visibility to within the assembly.
Deleted the unused 'oneUpPath' variable from the test setup to clean up the code.
Deleted the unused 'oneUpPath' variable from the test method to clean up the code.
Eliminated the unused exception variable in the catch block of the regex creation method. Added a TODO comment noting the need to refactor for logging since the logger is not accessible in the static context.
This pull request introduces several improvements and updates across the project, including bug fixes, dependency updates, enhancements to the
SarifWriterlogic, and the addition of comprehensive test cases. Below is a summary of the most important changes:Bug Fixes and Enhancements:
SarifWriterto ensure recommendations, descriptions, and rule information are appropriately handled in SARIF output. (DevSkim-DotNet/Microsoft.DevSkim.CLI/Writers/SarifWriter.cs, [1] [2]Testing:
SarifWriterto validate the handling of recommendations, descriptions, and fallback logic in SARIF Help fields. These tests ensure robust behavior for various rule configurations. (DevSkim-DotNet/Microsoft.DevSkim.Tests/SarifWriterTests.cs, DevSkim-DotNet/Microsoft.DevSkim.Tests/SarifWriterTests.csR1-R360)Dependency Updates:
brace-expansiondependency in multiplepackage-lock.jsonfiles to address potential security or compatibility concerns:1.1.11to1.1.12inDevSkim-VSCode-Plugin/client/package-lock.json. [1] [2]2.0.1to2.0.2inDevSkim-VSCode-Plugin/package-lock.json.Documentation:
Changelog.mdfile to document the changes introduced in version1.0.63, including the bug fix, dependency updates, and new test cases.