-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Stricter implementation of test session events for MTP #50703
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
Stricter implementation of test session events for MTP #50703
Conversation
if (TestOptions.HasFilterMode && !ModulePathExists()) | ||
{ | ||
return ExitCode.GenericFailure; | ||
} |
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.
Note: What we used to do here is that we early return GenericFailure, with explanation only available when logging is enabled. Now, the implementation will simply attempt to start the process, which will throw exception if something is wrong with the path. The exception will bubble up to TestApplicationActionQueue
and will be handled correctly there (we print the message + log it + ensure GenericFailure exit code + continue the rest test apps normally)
// TODO: If _handshakeInfo is null, we should error. | ||
// We shouldn't be getting any session event messages without a previous handshake. | ||
|
||
if (sessionEvent.SessionType == SessionEventTypes.TestSessionStart) |
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.
can we receive multiple session start events for a single session uid?
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.
Good question. I don't know. @mariam-abdulla ?
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.
Nope, MTP should be sending only 1 TestSessionStart and 1 TestSessionEnd.
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.
@mariam-abdulla I think that's generally correct yes. Maybe with HotReload being an exception? but I have no idea what the current status of HotReload with dotnet test is.
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 would keep it as-is for now and follow-up later if needed. The PR already improves what we had before where we didn't do any validations of the test session events.
Related to #50610. The PR addresses the concern, but keeping the issue open to add tests.