-
Notifications
You must be signed in to change notification settings - Fork 255
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
Adding filtering support at discovery #271
Adding filtering support at discovery #271
Conversation
@abhishekkumawat23, |
ITestCaseFilterExpression filterExpression = this.TestMethodFilter.GetFilterExpression(discoveryContext, logger, out filterHasError); | ||
if (filterHasError) | ||
{ | ||
return; |
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 would not return anything. Is this the spec?
Should we log in that case?
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 discussed with Pratap. Discovery should not happen in case filter has error
{ | ||
try | ||
{ | ||
// GetTestCaseFilter is present in DiscoveryContext but not in IDiscoveryContext interface. |
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.
Was this present in Dev11 as well? Did we validate this scenario in Dev11/Dev12?
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.
Validated in dev 12
public void GetFilterExpressionForDiscoveryContextWithoutGetTestCaseFilterReturnsNullTestCaseFilterExpression() | ||
{ | ||
TestableTestExecutionRecorder recorder = new TestableTestExecutionRecorder(); | ||
var dummyFilterExpression = new TestableTestCaseFilterExpression(); |
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.
nit: not required.
} | ||
catch (TargetInvocationException ex) | ||
{ | ||
throw ex.InnerException; |
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.
and who would handle this if it isn't a TestPlatformException or for that matter a non-TargetInvocationException is thrown?
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.
platform handles it automatically. message is logged in case of exception by platform.
Thanks for the clarifications. Approved! |
Revert "Include sign target as part of csproj. (microsoft#269)"
TPv2 now exposes filtering support at discovery.
Using that support via reflection (As filtering support is not exposed to IDiscoveryContext)