-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add assembly identity Api Check #18709
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
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/Compatibility/Microsoft.DotNet.ApiCompatibility/Resources.resx
Outdated
Show resolved
Hide resolved
src/Compatibility/Microsoft.DotNet.ApiCompatibility/Rules/AssemblyIdentityMustMatch.cs
Outdated
Show resolved
Hide resolved
src/Compatibility/Microsoft.DotNet.ApiCompatibility/Rules/AssemblyIdentityMustMatch.cs
Outdated
Show resolved
Hide resolved
src/Compatibility/Microsoft.DotNet.ApiCompatibility/Rules/AssemblyIdentityMustMatch.cs
Outdated
Show resolved
Hide resolved
src/Compatibility/Microsoft.DotNet.ApiCompatibility/Rules/AssemblyIdentityMustMatch.cs
Outdated
Show resolved
Hide resolved
src/Compatibility/Microsoft.DotNet.ApiCompatibility/Rules/AssemblyIdentityMustMatch.cs
Outdated
Show resolved
Hide resolved
src/Compatibility/Microsoft.DotNet.ApiCompatibility/Rules/AssemblyIdentityMustMatch.cs
Outdated
Show resolved
Hide resolved
src/Compatibility/Microsoft.DotNet.ApiCompatibility/Rules/AssemblyIdentityMustMatch.cs
Outdated
Show resolved
Hide resolved
src/Compatibility/Microsoft.DotNet.ApiCompatibility/Rules/AssemblyIdentityMustMatch.cs
Outdated
Show resolved
Hide resolved
src/Compatibility/Microsoft.DotNet.ApiCompatibility/Rules/AssemblyIdentityMustMatch.cs
Outdated
Show resolved
Hide resolved
| IEnumerable<CompatDifference> differences = differ.GetDifferences(left, right); | ||
|
|
||
| Assert.Single(differences); | ||
| Assert.Equal("CP0003 : left assembly name 'AssemblyA' does not match with right assembly name 'AssemblyB'.", differences.First().ToString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For these tests we are not asserting the messages because they are localized. Instead do a contains: AssemblyA and COntaines `AssemblyB'.
You can look at the other tests, but there is a CompatDifferenceComparer that doesn't compare the message. In order to assert the other properties of the difference.
Also, shouldn't this diagnostic ID be CP0004?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CP0004 is thrown when mapper doesnot find the exact assembly and set one of the inputs as null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A right. Should this have it's own diagnostic as well? Or you don't think it is worth it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont think its worth it. I am expecting the mapper scenario to be more common and we will hit that and version, culture and token are more interesting scenrios here.
src/Compatibility/Microsoft.DotNet.ApiCompatibility/Resources.resx
Outdated
Show resolved
Hide resolved
src/Compatibility/Microsoft.DotNet.ApiCompatibility/Rules/AssemblyIdentityMustMatch.cs
Outdated
Show resolved
Hide resolved
src/Tests/Microsoft.DotNet.ApiCompatibility.Tests/Rules/AssemblyIdentityMustMatchTests.cs
Outdated
Show resolved
Hide resolved
src/Compatibility/Microsoft.DotNet.ApiCompatibility/Resources.resx
Outdated
Show resolved
Hide resolved
src/Compatibility/Microsoft.DotNet.ApiCompatibility/Resources.resx
Outdated
Show resolved
Hide resolved
safern
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Assembly Identity rules
leftKeyTokenNull or retargetable flag set => always true
else leftKeyTokenNull == rightKeyTokenNull
added tests for each of these cases in strict and non-strict mode.