-
-
Notifications
You must be signed in to change notification settings - Fork 141
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
Use attribute instead of base class for MSTest #1193
Use attribute instead of base class for MSTest #1193
Conversation
I haven't forgotten about this. I got pulled into since stuff at work and have to put this on hold for another week or two. |
- Code generation attribute missing a quote - Fix reuse of generic parameter name in test - Fix incorrect nesting depth braces
src/Verify.MSTest.SourceGenerator.Tests/Tests.HasAttributeInNamespace.verified.txt
Show resolved
Hide resolved
src/Verify.MSTest/Verifier.cs
Outdated
/// <param name="testContext">The <see cref="TestContext"/> of the current test.</param> | ||
/// <param name="type">The <see cref="Type"/> of the currently running test.</param> | ||
/// <returns><c>true</c> if the reflection succeeded; <c>false</c> otherwise.</returns> | ||
static bool TryGetTypeFromTestContext(string typeName, TestContext testContext, [NotNullWhen(true)] out Type? type) |
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.
why so defensive in this method. IMO it should assume the correct shape and throw if that shape is not correct
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'm hesitant to assert on the specific shape here because I'm reaching into the internals of MSTest and not relying on any public API contracts.
More specifically, I'm concerned that this code would break and become an adoption blocker for early adopters of TestAnywhere (I'm not a user of it myself, but I did verify this code works with the current version).
In general I'm not super thrilled with the amount of reflection going on here, but I think reducing the amount would require digging more into Verify's internals, which feels to me like it should be a separate change.
Thus, I'd prefer leaving it as-is, but I'll do whatever you'd like. I'll volunteer to do follow up work in this space to reduce the amount of reflection overall, but I'd need some direction or a brain dump from you on how to proceed.
Let me know if this makes sense. Thanks!
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.
An idea that just hit me would be to augment what we stash in the AsyncLocal.
Rather than try to get back to the test class after-the-fact, grab everything we need in the TestContext property setter (which means we're already in the test class). Then Verifier
or VerifyBase
doesn't need to do any work.
If possible I'd like to keep this PR source generator focused, so if this sounds good to you I can do that as a follow up change, or even as a standalone change and then merge that into this branch.
Thoughts?
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.
An idea that just hit me would be to augment what we stash in the AsyncLocal.
sounds good. i will merge this now. can u have a go at an AsyncLocal PR next?
{ | ||
if (classToGenerate.Namespace is not null) | ||
{ | ||
builder.Append("namespace ").AppendLine(classToGenerate.Namespace) |
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 did a partial revert of the transformation of
builder.Append("namespace ").AppendLine(classToGenerate.Namespace)
to
builder.AppendLine($"namespace {classToGenerate.Namespace}")
(and related changes) for performance.
Because source generators must target netstandard2.0
, the underlying StringBuilder
doesn't have any interpolated string handling. That means interpolated strings will result in an intermediate allocation.
We could write handling for interpolated strings ourselves, but the code to do so looks nontrivial: https://github.com/dotnet/runtime/pull/51653/files#diff-072e8df6b3d249d58da2b688cbd9369ae5d1e9fb4c1c1da77a19384f3f7711a0.
I also added a comment to the top of IndentedStringBuilder
explaining the same decision.
Let me know if you strongly disagree and I can try to find another solution.
We can't overload on return type anyways, so no point in checking the type name of the property.
OK, all comments addressed. The two items to specifically take a look at are:
In all other cases I think I took your recommendations / suggestions. If you have any other feedback or thoughts, please let me know. Thanks! |
Fixes #1184
Overview
Introduce an attribute
[UsesVerify]
for MSTest so that tests can set up Verify without needing to use theVerifyBase
class. Relaxing the base class requirement makes it easier to use Verify in more cases, where other test scenarios may also require using a base class.The attribute triggers a source generator that adds the required
TestContext
property. The setter then stashes the context on an async local for use by the Verifier.Highlights
Verifier
class like the other test frameworks do and added it to the implict usingsVerifyBase
and forward calls toVerifier
TestContext
to get theType
andMethodInfo
used for deriving path infoVerifyBase
to[UsesVerify]
Open Issues
VerifyBase
inheritanceForAttributeWithMetadataName
, which limits users to .NET 7 SDK+ (but can still use any supported TFM)VerifyBase
tests?