-
Notifications
You must be signed in to change notification settings - Fork 253
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
Fix assembly resolution error #2948
Conversation
#else | ||
false); | ||
#endif | ||
#if NET |
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.
It was getting complex to read so I have split .NET logic from .NET FX logic. We end up with a little bit of duplicated code but a way better readability.
I'll fix the issue on Monday. @nohwnd test for SO is resolving resources is now triggering, I'll be copying the logic defined on VSTest for this case. |
@@ -335,7 +336,7 @@ protected virtual | |||
if (EqtTrace.IsInfoEnabled) | |||
{ | |||
EqtTrace.Info( | |||
"AssemblyResolver: {0}: Failed to create assemblyName. Reason:{1} ", | |||
"MSTest.AssemblyResolver.OnResolve: Failed to create assemblyName '{0}'. Reason: {1} ", |
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.
Prefix all assembly resolver messages with MSTest
and do a little reword to make it easier when debugging logs to understand if it's MSTest or VSTest assembly resolver being triggered.
incrementalSearchDirectory.Add(currentNode.DirectoryPath); | ||
// required assembly is not present in searchDirectories?? | ||
// see, if we can find it in user specified search directories. | ||
while (assembly == null && _directoryList?.Count > 0) |
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.
Remove the un-needed if and reduce nesting.
For future readers: Alternative (similar) implementation that is present in runtime: https://github.com/dotnet/corert/blob/master/src/System.Private.CoreLib/shared/System/SR.cs#L96 |
Thanks @nohwnd, I am tempted to refactor to use runtime impl as it's more efficient. |
In the PR #2654 that was simplifying resolver context to solve some internal issue with .NET, I accidentally changed a behavior that's causing the resolver to fail in VS.
It's not entirely clear to me why this wasn't spotted before as this change is from 6th of April... I cannot think of an easy way to add a test for that. Hopefully with some changes in Test Explorer repository we should finally have some nightly test of MSTest and be able to uncover such kind of issue. I'll setup a document to share with vendor testing team to ensure this specific scenario is tested regularly.
Fixes #2922.