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.
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
KnownReference: Add support for testing for referenced libraries #6726
KnownReference: Add support for testing for referenced libraries #6726
Changes from 9 commits
695358b
7547b03
6b5d8f4
5f39f96
c739408
d3842bd
92deb06
8422676
ec96228
ab95c9b
4caa689
af2cdc8
c4ae3a4
657c32e
9bef92a
9450d62
886b451
98364c7
abed89f
d619346
6d341b1
078a782
f9bbb8a
0cd2ee6
88e2f90
d2794d2
98ddc1b
0c21bd5
54ed0dd
5117f76
cb7633f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Does this need to be internal? For testing purposes, add more static fields with some well-known libraries (like the xunit) that will use the feature.
For some hell-versions, take a look at
TestMethodShouldContainAssertionTest
for inspirationThere 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'd rather like to test the single elements of the "DSL" for better test isolation. Relying on
public static KnownReference XUnit_Assert { get; }
and the like is problematic as these definitions change over time and any tests that rely on it will deteriorate. I did this to improve the situationPredicates
class[EditorBrowsable(EditorBrowsableState.Never)]
to suppress IntelliSense from showing the nested class as much as possible.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.
The
EditorBrowsable
is too strong solution. The nested class will do the trick.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.
Do you have a clear example in mind? Experience with KnownType shows that we typically exactly know what we're looking for.
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 StartsWith I was thinking of something like Azure.Storage, Azure.ResourceManager, AWSSDK or Microsoft.Extensions and for Contains something like IdentityModel.Tokens or SqlClient or Logger
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.
We will typically need to work with a price KnownType to do this check. And that one comes from a precisely known assembly => we should not need these.
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.
There are also checks like "IsTestProject" or "Is this targeting .Net Core" or "Is this a Web/WinForms/Console/WPF project" or "Is this a cloud app". These shortcuts can come in handy for such questions, especially for frameworks that have different implementations on different platforms like entity framework, SqlClient, or Asp.Net MVC.
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.
This is for cases like StackExchange.Redis which comes with a strong name or without (others are e.g. Dapper or NitoAsyncEx).
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 don't think we'll ever need to be that precise.
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.
Same as above
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.
If there is a strong name it is the most reliable source of identity. Also if we don't add the possibility to check for the strong name no one will ever add the functionality even in cases where it would be useful. The functionality is now there and fully covered by tests with our own public key.
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.
We don't need to be that precise at all. We'll likely use these for an early bailout, as you plan to use them. And there will be no harm if we trigger on something that is a false match.
Let's remove 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 also highly doubt that we would ever track a library that updates it's public key between versions.
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.
Could be file scoped namespace