-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Add WorkItem trait discoverer #78438
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
| var arguments = traitAttribute.GetConstructorArguments().ToArray(); | ||
| if (arguments is [int id, string url]) | ||
| { | ||
| yield return new KeyValuePair<string, string>("WorkItemId", id.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.
Where do "WorkItemId" and "WorkItemUrl" show up? Do we have to type a query like Trait: "WorkItemId=12345" or would Trait: 12345 work? #Closed
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 latter works. I think you could write the former to be more specific though.
| } | ||
| else if (arguments is [string onlyUrl]) | ||
| { | ||
| yield return new KeyValuePair<string, string>("WorkItemUrl", onlyUrl); |
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.
In the case where the WorkItem only contains a URL (which is what we now commonly do), maybe we should extract the ID and make it available to search on too? #Closed
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.
Even if there is just WorkItem(url) attribute, searching for Trait:IssueId works as well (i.e., a substring match is enough)
| { | ||
| public IEnumerable<KeyValuePair<string, string>> GetTraits(IAttributeInfo traitAttribute) | ||
| { | ||
| var arguments = traitAttribute.GetConstructorArguments().ToArray(); |
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.
Should we check that we're dealing with the WorkItem attribute? #Closed
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 could but it's not necessary - the WorkItemTraitDiscoverer is only used for WorkItemAttribute thanks to the corresponding [TraitDiscoverer("Microsoft.CodeAnalysis.Test.Utilities.WorkItemTraitDiscoverer", assemblyName: "Microsoft.CodeAnalysis.Test.Utilities")]. This is modeled after https://github.com/dotnet/roslyn/blob/3f3096be868cee0fa3d99875da1739caeb742553/src/Compilers/Test/Core/Traits/CompilerTraitDiscoverer.cs which also doesn't check the attribute type
|
|
||
| namespace Microsoft.CodeAnalysis.Test.Utilities; | ||
|
|
||
| public sealed class WorkItemTraitDiscoverer : ITraitDiscoverer |
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.
Allows searching for tests marked with WorkItemAttribute, e.g., in VS Test Explorer via Trait:"...issue url..."
Could you give an example/screenshot of what the experience looks like once the discoverer is in place?
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.
Added screenshot to PR description.
jcouv
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 Thanks (iteration 1)
Allows searching for tests marked with
WorkItemAttribute, e.g., in VS Test Explorer viaTrait:"...issue url..."orTrait:IssueNumber: