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

AnyCPU tests to choose default architecture based on process #2206

Merged
merged 7 commits into from Oct 18, 2019

Conversation

vagisha-nidhi
Copy link
Contributor

@vagisha-nidhi vagisha-nidhi commented Oct 1, 2019

Description

Default architecture to be decided whether the process is vstest.console or dotnet(32bit) or dotnet(64bit).
Also, getting testhost.exe path from nuget folder.

Changing the copying mechanism for testhost exes. We need to only copy test host exe in case of publish scenario (since already finding in nuget folders for build scenario)

Default architecture is set as follows:

  1. If process is vstest.console, default architecture is x64
  2. If process is dotnet =>
    a. if dotnet 32 bit : default architecture is x86
    b. if dotnet 64 bit : default architecture is x64

The logic to find native executable testhost.exe is also modified.

  1. Find testhost.exe/testhost.x86.exe in source folder.
  2. If not found, find testhost.exe/testhost.x86.exe in nuget folder based on the architecture.
  3. Else fallback on dotnet.

For x64 and x86 dlls, the corresponding testhost/testhost.x86.exe is copied to the source folder.
For AnyCPU, there willbe no copying of testhost exe.

dotnet test --runtime win7-x86 and dotnet test --runtime win7-x64 generates corresponding x86 and x64 dlls

Related Issue

#2189

@@ -12,7 +12,7 @@
<Visible>False</Visible>
</Content>
</ItemGroup>
<ItemGroup Condition=" '$(Platform)' != 'x86' AND '$(OS)' == 'Windows_NT'" >
<ItemGroup Condition=" ('$(Platform)' == 'x64' OR '$(PlatformTarget)' == 'x64') AND '$(OS)' == 'Windows_NT'" >
Copy link
Contributor

Choose a reason for hiding this comment

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

OS [](start = 89, length = 2)

what happens when we give mulitple PlatformTargets

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With multiple PlatformTargets, dotnet build generates AnyCPU dll (then picks testhost from nuget path depending upon default architecture)

@vagisha-nidhi vagisha-nidhi changed the title [Draft] AnyCPU tests to choose default architecture based on process AnyCPU tests to choose default architecture based on process Oct 4, 2019
[TestMethod]
public void GetTestHostProcessStartInfoShouldUseTestHostX86ExeFromNugetIfNotFoundInSourceLocation()
{
var testhostExePath = "testhost.x86.exe";
Copy link
Contributor

Choose a reason for hiding this comment

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

See if you can reduce this test by mocking only the relevant call, I am not sure if your change is related to this.

@0xd4d
Copy link

0xd4d commented Oct 10, 2019

This PR will probably also fix #2215

@singhsarab
Copy link
Contributor

@vagisha-nidhi Are we done with all the changes here ?

@vagisha-nidhi
Copy link
Contributor Author

@vagisha-nidhi Are we done with all the changes here ?

Yes. Please review this.

@@ -0,0 +1,9 @@
<?xml version="1.0" encoding="utf-8"?>
<Project ToolsVersion="12.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
Copy link
Contributor

Choose a reason for hiding this comment

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

Did we try CopyToPublishDirectory in the props itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried. It works. Updated the PR

Copy link
Contributor

@singhsarab singhsarab left a comment

Choose a reason for hiding this comment

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

:shipit:

@vagisha-nidhi vagisha-nidhi merged commit 89d102d into microsoft:master Oct 18, 2019
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.

None yet

3 participants