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.

Sign up for GitHub

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

Jump to bottom

Understanding assembly type from source #1529

Merged
merged 4 commits into from
Apr 9, 2018
Merged

Understanding assembly type from source #1529

merged 4 commits into from
Apr 9, 2018

Conversation

abhishkk
Copy link
Contributor

@abhishkk abhishkk commented Apr 5, 2018

Part 2 of RFC:
Contains changes for understanding assembly type from sources.

smadala
smadala suggested changes Apr 5, 2018
View reviewed changes
src/Microsoft.TestPlatform.Common/Utilities/PEReaderHelper.cs Outdated

try
{
using (var assemblyStream = File.Open(filePath, FileMode.Open, FileAccess.Read))
Copy link
Contributor

Choose a reason for hiding this comment

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

Use FileHelper interface.

Copy link
Contributor

Choose a reason for hiding this comment

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

Use FileShare.

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. Added API in FileHelper also.

TestPlatform.sln Outdated
@@ -159,6 +159,8 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "perf", "perf", "{0D4DF78D-7
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "vstest.console.PlatformTests", "test\vstest.console.PlatformTests\vstest.console.PlatformTests.csproj", "{8C068694-23A2-47A2-A0DD-DB82D0AF0142}"
EndProject
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "ConsoleManagedApp", "test\TestAssets\ConsoleManagedApp\ConsoleManagedApp.csproj", "{A4AAB1DF-BAD4-4C75-A0FD-10EDC2F38106}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Add to TestAssets.sln

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

test/Microsoft.TestPlatform.Common.UnitTests/Utilities/PEReaderHelperTests.cs Outdated
using Microsoft.VisualStudio.TestTools.UnitTesting;

[TestClass]
public class PEReaderHelperTests : IntegrationTestBase
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this to common platform tests.

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

test/TestAssets/CPPSimpleTestProject/CPPSimpleProj/CPPSimpleProj.vcxproj
@@ -22,7 +22,7 @@
<ProjectGuid>{076CE7E6-92E8-49FD-9D98-57D377FAA46E}</ProjectGuid>
<Keyword>Win32Proj</Keyword>
<RootNamespace>CPPSimpleProj</RootNamespace>
<WindowsTargetPlatformVersion>10.0.15063.0</WindowsTargetPlatformVersion>
<WindowsTargetPlatformVersion>10.0.16299.0</WindowsTargetPlatformVersion>
<ProjectName>Microsoft.TestPlatform.TestAsset.NativeCPP</ProjectName>
</PropertyGroup>
<Import Project="$(VCTargetsPath)\Microsoft.Cpp.Default.props" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Why ConsoleNativeApp.cpp is binary file?

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

test/TestAssets/CPPSimpleTestProject/ConsoleNativeApp/ConsoleNativeApp.vcxproj.filters
<Filter>Source Files</Filter>
</ClCompile>
</ItemGroup>
</Project>
Copy link
Contributor

Choose a reason for hiding this comment

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

Below files should not be binary files.

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

src/Microsoft.TestPlatform.Common/Utilities/PEReaderHelper.cs Outdated
{
var peHeaders = peReader.PEHeaders;
var corHeader = peHeaders.CorHeader;
var corHeaderStartOffset = peHeaders.CorHeaderStartOffset;
Copy link
Contributor

Choose a reason for hiding this comment

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

Add link to documentation in 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.

done

mayankbansal018
mayankbansal018 reviewed Apr 5, 2018
View reviewed changes
src/Microsoft.TestPlatform.Common/Utilities/PEReaderHelper.cs
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

namespace Microsoft.VisualStudio.TestPlatform.Common.Utilities
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this class Singleton?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not required. Removed.

mayankbansal018
mayankbansal018 approved these changes Apr 6, 2018
View reviewed changes
Copy link
Contributor

@mayankbansal018 mayankbansal018 left a comment

Choose a reason for hiding this comment

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

:shipit:

smadala
smadala reviewed Apr 9, 2018
View reviewed changes
src/Microsoft.TestPlatform.Common/Utilities/PEReaderHelper.cs Outdated
/// Initializes a new instance of the <see cref="PEReaderHelper"/> class.
/// </summary>
/// <param name="fileHelper">File helper.</param>
public PEReaderHelper(FileHelper fileHelper)
Copy link
Contributor

Choose a reason for hiding this comment

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

Use type IFileHelper.

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

src/Microsoft.TestPlatform.Common/Utilities/PEReaderHelper.cs Outdated

try
{
using (var fileStream = fileHelper.GetStream(filePath, FileMode.Open, FileAccess.Read, FileShare.ReadWrite))
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Add this.

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

src/Microsoft.TestPlatform.Common/Utilities/PEReaderHelper.cs Outdated
}
catch (Exception ex)
{
EqtTrace.Warning("GetAssemblyTypeFromAssemblyMetadata: failed to determine assembly type: {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.

Nit: Keep message prefix to Classname.Method name.

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

src/Microsoft.TestPlatform.Common/Utilities/PEReaderHelper.cs Outdated

if (EqtTrace.IsInfoEnabled)
{
EqtTrace.Info("AssemblyMetadataProvider.GetAssemblyType: Determined assemblyType:'{0}' for source: '{1}'", assemblyType, filePath);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: change the class name.

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

...crosoft.TestPlatform.Common.PlatformTests/Microsoft.TestPlatform.Common.PlatformTests.csproj Outdated
<Import Project="$(TestPlatformRoot)scripts/build/TestPlatform.Settings.targets" />
<PropertyGroup>
<TargetFrameworks>netcoreapp1.0;net451</TargetFrameworks>
<OutputType Condition=" '$(TargetFramework)' == 'netcoreapp2.0' ">Exe</OutputType>
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: this line is not required.

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

...crosoft.TestPlatform.Common.PlatformTests/Microsoft.TestPlatform.Common.PlatformTests.csproj
<ItemGroup>
<ProjectReference Include="..\Microsoft.TestPlatform.TestUtilities\Microsoft.TestPlatform.TestUtilities.csproj" />
<PackageReference Include="Microsoft.TestPlatform.TestAsset.NativeCPP">
<Version>2.0.0</Version>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please never upload nupkg with same version after changing the content. It will leads to different behavior based on user nuget cache.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not doing in this as we are only using it this nuget. Will update if happens in future changes

@abhishkk abhishkk merged commit 534026f into microsoft:master Apr 9, 2018
acesiddhu
acesiddhu reviewed Apr 9, 2018
View reviewed changes
src/Microsoft.TestPlatform.Common/Utilities/PEReaderHelper.cs

try
{
using (var fileStream = this.fileHelper.GetStream(filePath, FileMode.Open, FileAccess.Read, FileShare.ReadWrite))
Copy link
Contributor

Choose a reason for hiding this comment

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

why ReadWrite

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its share permission for other threads

src/Microsoft.TestPlatform.Common/Utilities/PEReaderHelper.cs

if (EqtTrace.IsInfoEnabled)
{
EqtTrace.Info("PEReaderHelper.GetAssemblyType: Determined assemblyType:'{0}' for source: '{1}'", assemblyType, filePath);
Copy link
Contributor

Choose a reason for hiding this comment

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

this should go in resources

Copy link
Contributor Author

Choose a reason for hiding this comment

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

these are logs only.

src/Microsoft.TestPlatform.Common/Utilities/PEReaderHelper.cs
var corHeader = peHeaders.CorHeader;
var corHeaderStartOffset = peHeaders.CorHeaderStartOffset;

assemblyType = (corHeader != null && corHeaderStartOffset >= 0) ?
Copy link
Contributor

Choose a reason for hiding this comment

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

can you point the documentation for how to determine if the assembly is managed or native

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Documentation links added in above lines

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@acesiddhu acesiddhu acesiddhu left review comments

@smadala smadala smadala approved these changes

@mayankbansal018 mayankbansal018 mayankbansal018 approved these changes

@cltshivash cltshivash Awaiting requested review from cltshivash

@singhsarab singhsarab Awaiting requested review from singhsarab

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@abhishkk @acesiddhu @smadala @mayankbansal018