-
Notifications
You must be signed in to change notification settings - Fork 323
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
Create new RuntimeProvider to be associated with each ProxyOperationManager #653
Create new RuntimeProvider to be associated with each ProxyOperationManager #653
Conversation
@mayankbansal018, |
@@ -146,8 +146,6 @@ public virtual void Close() | |||
finally | |||
{ | |||
this.initialized = false; | |||
this.testHostManager.HostExited -= this.TestHostManagerHostExited; |
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.
Please add it back.
@@ -126,14 +126,17 @@ public HostProviderEventArgs(string message) | |||
this.ErrroCode = 0; | |||
} | |||
|
|||
public HostProviderEventArgs(string message, int errorCode) | |||
public HostProviderEventArgs(string message, int errorCode, int processId) |
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:
Why don't we have separate cs file for these ?
Please add xml comments for anything that is public.
var isDataCollectorEnabled = XmlRunSettingsUtilities.IsDataCollectionEnabled(testRunCriteria.TestRunSettings); | ||
|
||
// SetupChannel ProxyExecutionManager with data collection if data collectors are specififed in run settings. | ||
Func<IProxyExecutionManager> proxyExecutionManagerCreator = |
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.
Don't we need tests for these changes ?
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.
yes, we do, I was thinking to put them in next PR, as to unblock the CI flow?
Since RuntimeProvider manages individual testruntime & also has state w.r.t a runtime's errorstream, & raises events w.r.t that runtime. It is better to associate a new runtimeprovider with each ProxyOperationManager(in parallel scenario)