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

Added OutputType to Microsoft.NET.Test.Sdk.target #310

Merged
merged 8 commits into from
Jan 6, 2017
Merged

Added OutputType to Microsoft.NET.Test.Sdk.target #310

merged 8 commits into from
Jan 6, 2017

Conversation

harshjain2
Copy link
Contributor

  • Defined OutputType for test project in Microsoft.NET.Test.Sdk.targets.
  • OutputType defined in this file will override the same property defined in .csproj

Issue Link : #287

Verification done:

  1. Verified by patching the Test.targets file and ran all the Unit Tests.
  2. Verified by patching the Test.targets file and ran Acceptance tests.
  3. Manually ran tests from vstest.console.unittests
  4. Verified by patching Test.Targets file and executed test from Visual Studio for dotnetcore test project.

@@ -12,6 +12,13 @@

<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">

<PropertyGroup >
<!-- Output type for full clr test project shoudl be library-->
<OutputType Condition=" !$(TargetFramework.ToLower().StartsWith('netcoreapp')) ">Library</OutputType>
Copy link
Contributor

Choose a reason for hiding this comment

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

How can user override the OutputType if she wishes to?

<!-- Output type for full clr test project shoudl be library-->
<OutputType Condition=" !$(TargetFramework.ToLower().StartsWith('netcoreapp')) ">Library</OutputType>
<!-- Output type for dotnet core test project should be exe. -->
<OutputType Condition=" $(TargetFramework.ToLower().StartsWith('netcoreapp')) ">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.

Why do we require to have !netcoreapp? Isn't Library the default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is, by default, Library.
The intention behind adding this is to explicitly override the OutputType.
Following scenario can lead to unpredictable behavior if this is not specified.
a. User creates a new netcoreapp1.0 project with current template.
b. User targets the project to net46 as well.
c. User upgraded to latest Microsoft.Net.Test.Sdk package.
c. Since is by default Exe and if we don't override this, net46 tests might not work.

@@ -12,6 +12,13 @@

<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">

<PropertyGroup >
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: extra space.

@@ -12,6 +12,13 @@

<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">

<PropertyGroup >
<!-- Output type for full clr test project shoudl be library-->
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: s/shoudl/should

<file src="Microsoft.NET.Test.Sdk.targets" target="build\netcoreapp1.0\" />
<file src="Microsoft.NET.Test.Sdk.targets" target="build\net45\" />
<file src="Microsoft.NET.Test.Sdk.props" target="build\netcoreapp1.0\" />
<file src="Microsoft.NET.Test.Sdk.targets" target="build\netcoreapp1.0\" />
Copy link
Contributor

Choose a reason for hiding this comment

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

It will break the user with net46 with outputType exe????

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have reverted it for now, let's have a detailed discussion on this and then do this change later.

@harshjain2 harshjain2 merged commit 3743280 into microsoft:master Jan 6, 2017
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