Validate existing suppressions and remove unnecessary suppressions when re-generating suppression files #32964

ViktorHofer · 2023-06-01T13:52:57Z

Adds the following flags to the APICompat frontends:

MSBuild	CLI	Default value
/p:ApiCompatPreserveUnnecessarySuppressions	--preserve-unnecessary-suppressions	false
/p:ApiCompatPermitUnnecessarySuppressions	--permit-unnecessary-suppressions	false

dotnet/runtime will need to opt-out of these switches for packable projects which have a reference assembly as we do a two phase validation for these.

Note for reviewers: Hide whitespaces and exclude resx and xlf changes in the GH review pane.

ghost · 2023-06-01T13:53:04Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

src/ApiCompat/Microsoft.DotNet.ApiCompat.Shared/SuppressionFileHelper.cs

src/ApiCompat/Microsoft.DotNet.ApiCompat.Task/ValidatePackageTask.cs

src/ApiCompat/Microsoft.DotNet.ApiCompat.Tool/Program.cs

src/ApiCompat/Microsoft.DotNet.ApiCompat.Shared/CommonResources.resx

src/ApiCompat/Microsoft.DotNet.ApiCompat.Task/ValidatePackageTask.cs

ericstj · 2023-06-02T16:47:28Z

src/ApiCompat/Microsoft.DotNet.ApiCompat.Task/ValidatePackageTask.cs

+        /// <summary>
+        /// If true, validates that baseline suppressions aren't obsolete.
+        /// </summary>
+        public bool ValidateSuppressions { get; set; } = true;


Setting the default to true here will be breaking. Make sure you raise that as a breaking change.

Every new rule in APICompat that is on by default is considered a breaking change. As well as infrastructure changes that enable more validation, i.e. #32930. I will group both changes together into one breaking change notice.

That's true. Perhaps simply documenting the new validation is sufficient here.

src/ApiCompat/Microsoft.DotNet.ApiCompatibility/Logging/SuppressionEngine.cs

src/ApiCompat/Microsoft.DotNet.ApiCompat.Task/ValidatePackageTask.cs

src/ApiCompat/Microsoft.DotNet.ApiCompatibility/Logging/Suppression.cs

src/ApiCompat/Microsoft.DotNet.ApiCompatibility/Logging/SuppressionEngine.cs

ericstj · 2023-06-02T17:08:18Z

src/Tests/Microsoft.DotNet.ApiCompatibility.Tests/Logging/SuppressionEngineTests.cs

            engine.AddSuppression(newSuppression);
            string filePath = Path.Combine(Path.GetTempPath(), Path.GetTempFileName(), "DummyFile.xml");
-            Assert.True(engine.WriteSuppressionsToFile(filePath));
+            Assert.True(engine.WriteSuppressionsToFile(filePath, removeObsoleteSuppressions: false));


Can we add tests that cover the new behavior?

- Remove unnecessary suppressions in APICompat files. This is in preparation for dotnet/sdk#32964 which will validate the existing suppressions going forward. - Set the required APICompat properties for the future tooling support.

ViktorHofer · 2023-06-03T14:00:07Z

Just tried the changes out on runtime and submitted dotnet/runtime#87094 to remove the unnecessary suppressions and already set the required properties to opt-out of the new behavior for packable projects that also have a reference source project.

dotnet/runtime#87094

* Remove unnecessary suppressions in APICompat files - Remove unnecessary suppressions in APICompat files. This is in preparation for dotnet/sdk#32964 which will validate the existing suppressions going forward. - Set the required APICompat properties for the future tooling support. * Add suppressions back for two CoreLib flavors * Fix CoreLib suppression because of API attribute difference

ericstj

Changes look good. Thank you for addressing. I didn't see tests yet so I imagine that's still pending.

I left one small observation now that we've aligned the properties.

src/ApiCompat/Microsoft.DotNet.ApiCompat.Task/ValidateAssembliesTask.cs

ViktorHofer · 2023-07-11T16:10:13Z

@ericstj can you please take another look? I just pushed two more commits which add tests, refactor the existing ones. I also found one bug in the WriteSuppressionFilesToFile method which I fixed by creating a new HashSet based on the existing one for the suppressions.

The tests were quite messy so I spent some time cleaning them up.

ericstj

LGTM - modulo one nit on the tests. Thank you for adding them.

ericstj · 2023-07-11T19:16:46Z

src/Tests/Microsoft.DotNet.ApiCompatibility.Tests/Logging/SuppressionEngineTests.cs

-                Right = "lib/net6.0/tempValidation.dll"
-            }));
+            TestSuppressionEngine suppressionEngine = new();
+            suppressionEngine.LoadSuppressions("NonExistentFile.xml");


Is loading a non-existent file essential to these test cases? That seems like a single test case rather than something we need to do in every one.

Yes, that's essential as it trigger loading the test suppression file content. See TestSuppressionEngine.cs which has an override to get the readable stream. Without calling the newly introduced LoadSuppressions method, the baseline suppressions won't get loaded.

RussKie · 2023-07-12T03:15:24Z

Thank you :)

ghost added the untriaged Request triage from a team member label Jun 1, 2023

ViktorHofer force-pushed the ValidateSuppressions branch from 66971d3 to e84d7e0 Compare June 1, 2023 14:07

ViktorHofer added Area-ApiCompat and removed untriaged Request triage from a team member labels Jun 1, 2023

ViktorHofer force-pushed the ValidateSuppressions branch 2 times, most recently from 197d65a to 7b0a2a0 Compare June 2, 2023 15:25

Validate suppressions

196afa5

ViktorHofer force-pushed the ValidateSuppressions branch from 7b0a2a0 to 196afa5 Compare June 2, 2023 15:36

ViktorHofer commented Jun 2, 2023

View reviewed changes

src/ApiCompat/Microsoft.DotNet.ApiCompat.Shared/SuppressionFileHelper.cs Show resolved Hide resolved

ViktorHofer commented Jun 2, 2023

View reviewed changes

src/ApiCompat/Microsoft.DotNet.ApiCompat.Task/ValidatePackageTask.cs Show resolved Hide resolved

ViktorHofer commented Jun 2, 2023

View reviewed changes

src/ApiCompat/Microsoft.DotNet.ApiCompat.Tool/Program.cs Outdated Show resolved Hide resolved

ViktorHofer commented Jun 2, 2023

View reviewed changes

src/ApiCompat/Microsoft.DotNet.ApiCompat.Tool/Program.cs Outdated Show resolved Hide resolved

ViktorHofer changed the title ~~Validate suppressions~~ Validate existing suppressions and remove obsolete suppressions when re-generating suppression files Jun 2, 2023

ViktorHofer marked this pull request as ready for review June 2, 2023 16:00

ViktorHofer requested review from a team, ericstj and joperezr as code owners June 2, 2023 16:00

ericstj reviewed Jun 2, 2023

View reviewed changes

src/ApiCompat/Microsoft.DotNet.ApiCompat.Shared/CommonResources.resx Outdated Show resolved Hide resolved

ericstj requested a review from carlossanlop June 2, 2023 16:44

ericstj reviewed Jun 2, 2023

View reviewed changes

ViktorHofer added 2 commits June 3, 2023 10:59

PR feedback from EricStj

48d8baf

Fix wrong variable used for serialization

81c8adc

ViktorHofer mentioned this pull request Jun 3, 2023

Should APICompat PackageValidation's EnableStrictModeForCompatibleTfms default to true? #33011

Closed

ViktorHofer changed the title ~~Validate existing suppressions and remove obsolete suppressions when re-generating suppression files~~ Validate existing suppressions and remove unnecessary suppressions when re-generating suppression files Jun 3, 2023

ViktorHofer added 4 commits June 3, 2023 11:29

Fix tests

1f24eb9

Update comment

8e46524

Improve logging. Log unnecessary suppressions as well

079d725

Nit changes

925cdb0

ViktorHofer mentioned this pull request Jun 3, 2023

Remove unnecessary suppressions in APICompat files dotnet/runtime#87094

Merged

ericstj reviewed Jun 8, 2023

View reviewed changes

src/ApiCompat/Microsoft.DotNet.ApiCompat.Task/ValidateAssembliesTask.cs Show resolved Hide resolved

ViktorHofer added 4 commits July 5, 2023 17:54

Merge branch 'main' into ValidateSuppressions

99ccfc1

Update Program.cs

86323e5

Update Program.cs

d7a1951

Update Program.cs

e618de1

ViktorHofer self-assigned this Jul 5, 2023

ViktorHofer added 2 commits July 11, 2023 17:38

Add SuppressionEngine tests and clean-up and ref

b760bf9

Clean-up code

0244c36

ViktorHofer requested a review from ericstj July 11, 2023 16:02

ericstj approved these changes Jul 11, 2023

View reviewed changes

ViktorHofer merged commit a3303b6 into main Jul 11, 2023

ViktorHofer deleted the ValidateSuppressions branch July 11, 2023 19:32

ViktorHofer mentioned this pull request Aug 2, 2023

Fix ApiCompat.Tool execution #34406

Merged

Validate existing suppressions and remove unnecessary suppressions when re-generating suppression files #32964

Validate existing suppressions and remove unnecessary suppressions when re-generating suppression files #32964

Uh oh!

Conversation

ViktorHofer commented Jun 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jun 1, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ericstj Jun 2, 2023

Choose a reason for hiding this comment

Uh oh!

ViktorHofer Jun 3, 2023

Choose a reason for hiding this comment

Uh oh!

ericstj Jun 7, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ericstj Jun 2, 2023

Choose a reason for hiding this comment

Uh oh!

ViktorHofer commented Jun 3, 2023

Uh oh!

ericstj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ViktorHofer commented Jul 11, 2023

Uh oh!

ericstj left a comment

Choose a reason for hiding this comment

Uh oh!

ericstj Jul 11, 2023

Choose a reason for hiding this comment

Uh oh!

ViktorHofer Jul 11, 2023

Choose a reason for hiding this comment

Uh oh!

RussKie commented Jul 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ViktorHofer commented Jun 1, 2023 •

edited

Loading