Fix related to XML suppressions not working by vamsipolavarapu-msft · Pull Request #705 · microsoft/DevSkim

vamsipolavarapu-msft · 2025-10-13T19:32:02Z

Fix related to XML suppressions not working (#588).

Changes:

Added xml suppression comments syntax requirements in comments.json
Added a test class for testing suppression comments. File: DevSkimRuleProcessorTests.cs

Changes: - Added xml suppression comments syntax requirements in comments.json - Added a test class for testing suppression comments. File: DevSkimRuleProcessorTests.cs

gfs · 2025-10-14T18:03:11Z

/azp run

azure-pipelines · 2025-10-14T18:03:34Z

Azure Pipelines successfully started running 3 pipeline(s).

DevSkim-DotNet/Microsoft.DevSkim/resources/comments.json

DevSkim-DotNet/Microsoft.DevSkim.Tests/DevSkimRuleProcessorTests.cs

vamsipolavarapu-msft · 2025-10-22T00:12:42Z

Sample XML suppression

danfiedler-msft · 2025-10-22T15:43:19Z

/azp run

azure-pipelines · 2025-10-22T15:43:44Z

Azure Pipelines successfully started running 3 pipeline(s).

Copilot

Pull Request Overview

This PR fixes an issue where XML suppression comments were not working in DevSkim. The fix adds XML-specific comment syntax definitions and introduces comprehensive test coverage for suppression functionality across multiple languages.

Key changes:

Added XML comment syntax () to the comments configuration
Created new test class DevSkimRuleProcessorTests.cs with unit tests for suppression generation across various languages
Enhanced existing SuppressionsTest.cs with integration tests specifically for XML suppressions

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File	Description
DevSkim-DotNet/Microsoft.DevSkim/resources/comments.json	Added XML comment syntax definition with prefix `<!--` and suffix `-->`
DevSkim-DotNet/Microsoft.DevSkim.Tests/SuppressionsTest.cs	Added XML-specific integration tests and TestContext property
DevSkim-DotNet/Microsoft.DevSkim.Tests/DevSkimRuleProcessorTests.cs	New test class covering suppression generation for multiple languages including XML

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

DevSkim-DotNet/Microsoft.DevSkim.Tests/SuppressionsTest.cs

DevSkim-DotNet/Microsoft.DevSkim.Tests/DevSkimRuleProcessorTests.cs

gfs · 2025-10-22T18:51:46Z

This looks like it'd be the correct way to specify this to me. The test failures I think are a pipeline permission issue, maybe some setting needs to be updated?

…evSkimRuleProcessorTests

vamsipolavarapu-msft · 2025-10-22T19:01:55Z

This looks like it'd be the correct way to specify this to me. The test failures I think are a pipeline permission issue, maybe some setting needs to be updated?

Yes, the assert statements are in the correct format. And Dan is checking on the pipeline permissions issue. I cleaned up the other unused test method as well.

gfs · 2025-10-22T20:11:06Z

@vamsipolavarapu-msft To be able to merge you need to accept the CLA and also add an update to the changelog to satisfy the CI check.

vamsipolavarapu-msft · 2025-10-22T21:11:18Z

@microsoft-github-policy-service agree company="Microsoft"

Enabling suppressions for XML files to address bug microsoft#588.

gfs · 2025-10-22T23:03:19Z

/azp run

azure-pipelines · 2025-10-22T23:03:38Z

Azure Pipelines successfully started running 3 pipeline(s).

gfs · 2025-12-02T18:01:21Z

/azp run

azure-pipelines · 2025-12-02T18:01:27Z

Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command.

gfs · 2026-02-04T21:30:44Z

/azp run

azure-pipelines · 2026-02-04T21:30:52Z

No pipelines are associated with this pull request.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

Comments suppressed due to low confidence (2)

DevSkim-DotNet/Microsoft.DevSkim.Tests/SuppressionsTest.cs:433

Assert.Contains is not a method available in MSTest framework. This should use StringAssert.Contains(result, "<!-- DevSkim: ignore") instead, or alternatively use Assert.IsTrue(result.Contains("<!-- DevSkim: ignore")).

            Assert.Contains("<!-- DevSkim: ignore", result, "XML suppression comment should be present");

DevSkim-DotNet/Microsoft.DevSkim.Tests/SuppressionsTest.cs:457

This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.

            foreach (string line in lines)
            {
                if (line.Contains("<!-- DevSkim: ignore"))
                {
                    foundSuppression = true;
                    Suppression suppression = new Suppression(line);
                    Assert.IsTrue(suppression.IsInEffect, "Suppression should be in effect");
                    
                    if (!string.IsNullOrEmpty(reviewerName))
                    {
                        Assert.AreEqual(reviewerName, suppression.Reviewer, "Reviewer name should match");
                    }
                    
                    if (duration > 0)
                    {
                        //Assert.IsNotNull(suppression.ExpirationDate, "Expiration date should be set");
                        Assert.AreEqual(expectedExpiration.Date, suppression.ExpirationDate, "Expiration date should match");
                    }
                }
            }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-04T21:37:43Z

DevSkim-DotNet/Microsoft.DevSkim.Tests/SuppressionsTest.cs

+        [TestMethod]
+        [DataRow("", 30, DisplayName = "XML Suppression with Duration only")]
+        [DataRow("TestReviewer", 30, DisplayName = "XML Suppression with Reviewer and Duration")]
+        [DataRow("TestReviewer", 0, DisplayName = "XML Suppression with Reviewer only")]
+        public void ExecuteSuppressionsForXMLWithReviewerAndDuration(string reviewerName, int duration)


This test method uses [DataRow] attributes but is marked with [TestMethod] instead of [DataTestMethod]. The correct attribute should be [DataTestMethod] for parameterized tests. Without this, the test will fail to execute properly.

Copilot · 2026-02-04T21:37:43Z

DevSkim-DotNet/Microsoft.DevSkim.Tests/SuppressionsTest.cs

+
+                    if (duration > 0)
+                    {
+                        //Assert.IsNotNull(suppression.ExpirationDate, "Expiration date should be set");


This assertion is commented out without explanation. If the assertion is not needed, the commented code should be removed to keep the codebase clean. If it's needed, it should be uncommented or the reason for commenting it out should be documented.

Suggested change

//Assert.IsNotNull(suppression.ExpirationDate, "Expiration date should be set");

Copilot · 2026-02-04T21:37:43Z

DevSkim-DotNet/Microsoft.DevSkim.Tests/SuppressionsTest.cs

    [TestClass]
    public class SuppressionTest
    {
+        public TestContext? TestContext { get; set; }


This TestContext property is declared but never used in any test methods. If it's not needed, it should be removed to keep the code clean. TestContext is typically used for accessing test run information, logging, or data-driven test data sources, none of which appear to be used in this class.

Suggested change

public TestContext? TestContext { get; set; }

Copilot · 2026-02-04T21:37:44Z

DevSkim-DotNet/Microsoft.DevSkim.Tests/SuppressionsTest.cs

+            Assert.Contains("<!-- DevSkim: ignore", result, "XML suppression comment should be present");
+            Assert.Contains("-->", result, "XML suppression comment should be properly closed");


Assert.Contains is not a method available in MSTest framework. MSTest uses StringAssert.Contains(string value, string substring) with the arguments in reverse order. These lines should use StringAssert.Contains(result, "") instead, or alternatively use Assert.IsTrue(result.Contains(...)).

Suggested change

Assert.Contains("", result, "XML suppression comment should be properly closed");

StringAssert.Contains(result, "", "XML suppression comment should be properly closed");

Copilot · 2026-02-04T21:37:44Z

DevSkim-DotNet/Microsoft.DevSkim.Tests/SuppressionsTest.cs

+                    foundSuppression = true;
+                    Suppression suppression = new Suppression(line);
+                    Assert.IsTrue(suppression.IsInEffect, "Suppression should be in effect");
+                    Assert.IsGreaterThan(0,suppression.GetSuppressedIds.Length, "Should have at least one suppressed rule ID");


Assert.IsGreaterThan is not a method available in MSTest framework. The correct assertion would be Assert.IsTrue(suppression.GetSuppressedIds.Length > 0, "Should have at least one suppressed rule ID"). Note that the argument order in the current code is also incorrect - the expected value should come first.

Suggested change

Assert.IsGreaterThan(0,suppression.GetSuppressedIds.Length, "Should have at least one suppressed rule ID");

Assert.IsTrue(suppression.GetSuppressedIds.Length > 0, "Should have at least one suppressed rule ID");

Copilot · 2026-02-04T21:37:44Z

DevSkim-DotNet/Microsoft.DevSkim.Tests/SuppressionsTest.cs

+            foreach (string line in lines)
+            {
+                if (line.Contains("<!-- DevSkim: ignore"))
+                {
+                    foundSuppression = true;
+                    Suppression suppression = new Suppression(line);
+                    Assert.IsTrue(suppression.IsInEffect, "Suppression should be in effect");
+                    Assert.IsGreaterThan(0,suppression.GetSuppressedIds.Length, "Should have at least one suppressed rule ID");
+                }
+            }


This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.

Committing changes related XML suppressions not working (microsoft#588).

43e9975

Changes: - Added xml suppression comments syntax requirements in comments.json - Added a test class for testing suppression comments. File: DevSkimRuleProcessorTests.cs

gfs reviewed Oct 14, 2025

View reviewed changes

DevSkim-DotNet/Microsoft.DevSkim/resources/comments.json Outdated Show resolved Hide resolved

gfs reviewed Oct 14, 2025

View reviewed changes

DevSkim-DotNet/Microsoft.DevSkim.Tests/DevSkimRuleProcessorTests.cs Show resolved Hide resolved

vamsipolavarapu added 2 commits October 21, 2025 15:46

New integration tests for XML suppressions

325a1fc

Refactored the tests in DevSkimRuleProcessorTests.cs file

09fd8e7

gfs requested a review from Copilot October 22, 2025 18:50

Copilot AI reviewed Oct 22, 2025

View reviewed changes

Cleaned up the unused method GenerateSuppressionByLanguage from the D…

0e6135c

…evSkimRuleProcessorTests

Update Changelog for version 1.0.67

2004e3f

Enabling suppressions for XML files to address bug microsoft#588.

Merge branch 'main' into SuppressionAndProgressBarChanges

e0212e3

gfs requested a review from Copilot February 4, 2026 21:32

Copilot started reviewing on behalf of gfs February 4, 2026 21:32 View session

Copilot AI mentioned this pull request Feb 4, 2026

Fix XML suppression comments in CLI suppress command #717

Draft

13 tasks

Copilot AI reviewed Feb 4, 2026

View reviewed changes

		Assert.Contains("<!-- DevSkim: ignore", result, "XML suppression comment should be present");
		Assert.Contains("-->", result, "XML suppression comment should be properly closed");

	Assert.IsGreaterThan(0,suppression.GetSuppressedIds.Length, "Should have at least one suppressed rule ID");
	Assert.IsTrue(suppression.GetSuppressedIds.Length > 0, "Should have at least one suppressed rule ID");

Conversation

vamsipolavarapu-msft commented Oct 13, 2025

Uh oh!

gfs commented Oct 14, 2025

Uh oh!

azure-pipelines bot commented Oct 14, 2025

Uh oh!

Uh oh!

Uh oh!

vamsipolavarapu-msft commented Oct 22, 2025

Uh oh!

danfiedler-msft commented Oct 22, 2025

Uh oh!

azure-pipelines bot commented Oct 22, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gfs commented Oct 22, 2025

Uh oh!

vamsipolavarapu-msft commented Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gfs commented Oct 22, 2025

Uh oh!

vamsipolavarapu-msft commented Oct 22, 2025

Uh oh!

gfs commented Oct 22, 2025

Uh oh!

azure-pipelines bot commented Oct 22, 2025

Uh oh!

gfs commented Dec 2, 2025

Uh oh!

azure-pipelines bot commented Dec 2, 2025

Uh oh!

gfs commented Feb 4, 2026

Uh oh!

azure-pipelines bot commented Feb 4, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vamsipolavarapu-msft commented Oct 22, 2025 •

edited

Loading