-
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
Resolve testhost from source directory if we couldnt resolve it via nuget cache #690
Conversation
Fix: Try to resolve testhost from source location if you couldn’t resolve it via nuget.
@Faizan2304, |
string testHostFullPath = @"C:\packages\microsoft.testplatform.testhost/15.0.0-Dev\lib/netstandard1.5/testhost.dll"; | ||
this.mockFileHelper.Setup(ph => ph.Exists(testHostFullPath)).Returns(false); | ||
|
||
string testHostPath = Path.Combine($"{Path.DirectorySeparatorChar}tmp", "testhost.dll"); |
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.
string testHostPath = Path.Combine($"{Path.DirectorySeparatorChar}tmp", "testhost.dll"); [](start = 12, length = 88)
Reuse class variable declared above:
Line No : 63 : this.defaultTestHostPath = @"\tmp\testhost.dll";
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.
👍 with a suggestion.
// Try resolving testhost from output directory of test project. This is required if user has published the test project | ||
// and is running tests in an isolated machine. A second scenario is self test: test platform unit tests take a project | ||
// dependency on testhost (instead of nuget dependency), this drops testhost to output path. | ||
testHostPath = Path.Combine(sourceDirectory, "testhost.dll"); |
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 should handle case if testhost.dll does not exist in sourceDirectory too for some reason.
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.
Interesting. Would this publish the Extensions folder, that has the default extensions here too?
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.
@smadala : We are handling the case if testhost.dll doesn't exist here https://github.com/Microsoft/vstest/blob/master/src/Microsoft.TestPlatform.TestHostProvider/Hosting/DotnetTestHostManager.cs#L243
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.
@AbhitejJohn : very interesting catch 👍 . we are planning to handle thing related to Extension in separate task.
} | ||
}"; | ||
|
||
MemoryStream runtimeConfigStream = new MemoryStream(Encoding.UTF8.GetBytes(runtimeConfigFileContent)); |
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 use using...
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 cant use using here as these memory stream are getting use in mock
Issue: #657
Fix: Try to resolve testhost from source location if you couldn’t resolve it via nuget.
Test: Added UT to cover change and tested the fix manually