Skip to content
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 vstest.console.exe grabs exclusive read access to its test container #1660

Conversation

smadala
Copy link
Contributor

@smadala smadala commented Jun 25, 2018

Description

  • Use FileStream API to create stream.
  • Default FileShare for FileStream .ctor is FileShare.Read where as File.Open() default FileShare value is FileShare.None

Related issue

#1638

}
}
catch (Exception ex)
{
EqtTrace.Warning("GetFrameWorkFromMetadata: failed to determine TargetFrameworkVersion: {0} for assembly: {1}", ex, filePath);
EqtTrace.Warning("AssemblyMetadataProvider.GetFrameWork: failed to determine TargetFrameworkVersion: {0} for assembly: {1}", ex, filePath);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TargetFrameworkVersion: {0} Here {0} is ex and not TargetFrameworkVersion. Can you have a look?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parameters are interchanged.. also please inline it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TargetFrameworkVersion: {0} Here {0} is ex and not TargetFrameworkVersion. Can you have a look?

Done.

The parameters are interchanged.. also please inline it.

Making inline causes additional statement string.format which we don't want be cause most of time we don't in diag mode. See the below IL code.
image

@@ -135,6 +161,7 @@ public void GetFrameWorkForNativeDll()

private void TestDotnetAssemblyArch(string projectName, string framework, Architecture expectedArch, long expectedElapsedTime)
{
this.isManagedAssemblyArchitectureTest = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test for checking FileShare working or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually now we don't need FileShare. Added FileHelperTests.cs to add required test.

@@ -99,7 +99,7 @@ private static ISymbolReader GetSymbolReader(string binaryPath)
// For remote scenario, also look for pdb in current directory, (esp for UWP)
// The alternate search path should be an input from Adapters, but since it is not so currently adding a HACK
pdbFilePath = !File.Exists(pdbFilePath) ? Path.Combine(Directory.GetCurrentDirectory(), Path.GetFileName(pdbFilePath)) : pdbFilePath;
using (var stream = new FileHelper().GetStream(pdbFilePath, FileMode.Open, FileAccess.Read))
using (var stream = new FileHelper().GetStream(pdbFilePath, FileMode.Open, FileAccess.Read, FileShare.Read))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibility to have a single method for GetStreamForRead in FileHelper

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change not required now. we don't want to add new method which leads to additional learning . The helper is just wrapper to IO apis.


/// <summary>
/// Gets the instance.
/// </summary>
internal static AssemblyMetadataProvider Instance => instance ?? (instance = new AssemblyMetadataProvider());
internal static AssemblyMetadataProvider Instance => instance ?? (instance = new AssemblyMetadataProvider(new FileHelper()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why internal.. make it public.. the class is internal already

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

internal static AssemblyMetadataProvider Instance => instance ?? (instance = new AssemblyMetadataProvider());
internal static AssemblyMetadataProvider Instance => instance ?? (instance = new AssemblyMetadataProvider(new FileHelper()));

internal AssemblyMetadataProvider(IFileHelper fileHelper)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This we don't expect to used by any other assembly except test assembly. If we make this public we will lose the flexibility to change the signature.

@smadala smadala force-pushed the fix-running-test-for-same-assembly-in-parallel branch from 58054ab to c1b0202 Compare June 27, 2018 11:11
@smadala smadala merged commit 7b62482 into microsoft:master Jun 27, 2018
@smadala smadala deleted the fix-running-test-for-same-assembly-in-parallel branch June 27, 2018 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants