Changing test projects to default to netcoreapp1.1 instead of netcoreapp1.0 by joperezr · Pull Request #12241 · dotnet/corefx

joperezr · 2016-09-30T17:38:27Z

Fixes #12129

This change will make it such that netcoreapp1.1 will be the new default TestTFM for all projects, and it will also now only run the netcoreapp1.1 tests as part of the regular CI. One more thing I did with this change, is to make all of the test .csprojs to also default their nuget target moniker to netstandard1.7 if they were testing API that is specific to that framework, and I did this so that now all three (tests, refs, and impls) will have the same behavior and default to netstandard1.7 and x-compile only to older frameworks.

cc: @weshaggard @ericstj @danmosemsft @stephentoub @karajas

ericstj · 2016-09-30T17:54:31Z

Can we also delete https://github.com/dotnet/corefx/search?utf8=%E2%9C%93&q=TestNugetTargetMoniker?

ericstj · 2016-09-30T17:51:06Z

    <RootNamespace>System.Collections.Concurrent.Tests</RootNamespace>
-    <DefineConstants Condition="'$(TargetGroup)'=='netstandard1.7'">$(DefineConstants);netstandard17</DefineConstants>
-    <NugetTargetMoniker Condition="'$(NugetTargetMoniker)'==''">.NETStandard,Version=v1.3</NugetTargetMoniker>
+    <DefineConstants Condition="'$(TargetGroup)'==''">$(DefineConstants);netstandard17</DefineConstants>


I wonder if we should make netstandard13 be the ifdef. Maybe separate issue.

Yeah, I guess that would be better but it is not super straight forward to make this with a script so it would have to be done manually. I can do that as a separate cleanup PR.

I agree we should make the non-default configuration the one that should be #ifdef'ed, and I also agree we can do that as a follow-up item.

ericstj · 2016-09-30T17:53:17Z

@@ -1,4 +1,4 @@
-<?xml version="1.0" encoding="utf-8"?>
+<?xml version="1.0" encoding="utf-8"?>


Undo this file, and others. Normally I wouldn't care about these changes but since nothing else changed in the file should be easy to delete it.

joperezr · 2016-09-30T18:17:35Z

Can we also delete https://github.com/dotnet/corefx/search?utf8=%E2%9C%93&q=TestNugetTargetMoniker?

Will do.

ericeil · 2016-09-30T18:47:19Z

@@ -152,7 +151,7 @@
    <ItemGroup>
      <Project>
        <!-- default to netcoreapp1.0.0 if TestTFMs aren't set on the project -->


Should probably also change the comment

good catch, will do.

joperezr · 2016-09-30T20:59:35Z

@weshaggard I've removed the TestTFMSupportSet with my latest commit. PTAL to dir.traversal.targets change in f4b9864

weshaggard · 2016-09-30T22:31:38Z

-    <TestTFM Condition="'$(TestTFM)'==''">netcoreapp1.0</TestTFM>
+    <TestTFM Condition="'$(TestTFM)'==''">netcoreapp1.1</TestTFM>
    <!-- we default FilterToTestTFM to netcoreapp1.1 if it is not explicity defined -->
    <FilterToTestTFM Condition="'$(FilterToTestTFM)'==''">netcoreapp1.1</FilterToTestTFM>


Just to connect the dots can you set FiltertoTestTFM to TestTFM.

I'll do this

weshaggard · 2016-09-30T22:32:09Z

-        <!-- default to netcoreapp1.0.0 if TestTFMs aren't set on the project -->
-        <TestTFMs Condition="'%(Project.TestTFMs)'==''">netcoreapp1.0</TestTFMs>
+        <!-- default to netcoreapp1.1 if TestTFMs aren't set on the project -->
+        <TestTFMs Condition="'%(Project.TestTFMs)'==''">netcoreapp1.1</TestTFMs>


Similarly to connect the dots can you use TestTFM here.

Actually we might want to think about this one a little more (especially in cases where people actually set it at the commandline and it isn't the default) but I do still think it might make since to share a common property for the default. Perhaps we should create an independent "DefaultTestTFM" property and use that as the default in all these places.

mm I don't think that we should set it to TestTFM. I think it is good to keep TestTFM property only related when building a single project or an individual .csproj, and TestTFMs for the traversal world and .builds files. Your DefaultTestTFM idea I think is better, but it might need a bit more logic to it. Do you want me to do this now or should we design it and do it in a follow up PR?

Talked offline, I'll do this as well

weshaggard · 2016-09-30T22:36:39Z

-        "opensuse.42.1-x64"
-      ]
-    }
+    "coreFx.Test.netcoreapp1.1": {}


Can you please also include "coreFx.Test.net463" as it is needed for the other test configuration.

@stephentoub in order to run these on desktop you will need this support clause. After you do that you have too possible ways to run:

msbuild System.CodeDom.Tests.csproj /p:OSGroup=Windows_NT /p:TestTFM=net463 or msbuild System.CodeDom.Tests.builds /p:FilterToTestTFM=net463

For better or worse when building the builds file we build all combinations but only test the what is set in FilterToTestTFM which you can pass directory or it defaults to https://github.com/dotnet/corefx/pull/12241/files#diff-0b192804a6349e8c26d2b027afbd89a2R504.

I thought net463 was not being tested yet, I'll add this in here.

weshaggard · 2016-09-30T22:37:03Z

+      <TargetGroup>netstandard1.3</TargetGroup>
+      <TestTFMs>netcoreapp1.0</TestTFMs>
    </Project>
+    <Project Include="System.Collections.Concurrent.Tests.csproj" />


Lets please try to keep the default configuration at the top of the files.

This comment applies to all the .builds files.

This won't be super straight forward to do, the reason why they got out of order like this is because I used a tool to parse and edit all of these files, but it edits them in place, so this one for example use to be the netstandard1.7 and netcoreapp1.1 build, but it is now the default since those are the defaults now. Since it doesn't really add any value I believe if we watne dto clean this up we could do it later, but I can totally do it now if you feel strongly about it.

Nvm, I found a way to automate this easily so I'll do it also

weshaggard · 2016-09-30T22:39:31Z

-    <DefineConstants Condition="'$(TargetGroup)'=='netstandard1.7'">$(DefineConstants);netstandard17</DefineConstants>
-    <NugetTargetMoniker Condition="'$(NugetTargetMoniker)'==''">.NETStandard,Version=v1.3</NugetTargetMoniker>
+    <DefineConstants Condition="'$(TargetGroup)'==''">$(DefineConstants);netstandard17</DefineConstants>
+    <NugetTargetMoniker Condition="'$(NugetTargetMoniker)'==''">.NETStandard,Version=v1.7</NugetTargetMoniker>


nit: As part of your clean-up can you change these conditions to make them consistent and use '$(TargetGroup)==''`? I want to try and keep the conditions in the projects limited to OSGroup/Targets and TargetGroup to try and cut down on the different things folks need to know about in these conditions.

Sounds good, I'll change this.

weshaggard · 2016-09-30T22:40:40Z

    <Compile Include="ArrayListTests.cs" />
    <Compile Include="ArrayList\ArrayList.IList.Tests.cs" />
-    <Compile Include="CaseInsensitiveHashCodeProviderTests.cs" Condition="'$(TargetGroup)'=='netstandard1.7'" />
+    <Compile Include="CaseInsensitiveHashCodeProviderTests.cs" Condition="'$(TargetGroup)'==''" />


Can we move this to the item group with the same condition below?

weshaggard · 2016-09-30T22:43:51Z

    <RootNamespace>System.Drawing.Primitives.Tests</RootNamespace>
    <AssemblyName>System.Drawing.Primitives.Tests</AssemblyName>
    <ProjectGuid>{297A9116-1005-499D-A895-2063D03E4C94}</ProjectGuid>
+    <NugetTargetMoniker>.NETStandard,Version=v1.7</NugetTargetMoniker>


Does this need a condition?

Even if it isn't necessary today I would suggest adding the condition to help prevent issues tomorrow when we add another configuration.

weshaggard · 2016-09-30T22:46:00Z

+      <TestTFMs>netcoreapp1.0</TestTFMs>
    </Project>
    <Project Include="System.Globalization.Extensions.Tests.csproj">
-      <TestTFMs>net463</TestTFMs>


This should still be OSGroup=Windows_NT and it looks like we also need to include a Unix default build.

Actually looks like we might not even need the Unix build for netstandard1.3 as it doesn't look like it is being used in the test project. We at least need to add the net463 configuration with Windows_NT as they will not work or run on Unix.

the .csproj inside is setting Windows_NT by default as the OSGroup, which is why it is not needed here. If I keep it as it was it would cause a binclash betweein this build and the default one for netcoreapp1.1, where one has OSGroup set as a global property and the other one is set in the .csproj. That is why I merged them into only one build, that has two testTFMs

…app1.0

weshaggard · 2016-10-01T21:03:34Z

 <Project ToolsVersion="12.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
  <ItemGroup>
+    <Project Include="Performance\System.IO.FileSystem.Performance.Tests.csproj" />


nit: This .builds file has interesting ordering because it contains multiple build projects. If I were to sort this one I would still group the individual test projects. I realize you generated this so I know how it turned out like this I just wanted to point it out to you.

weshaggard · 2016-10-01T21:06:11Z

+      <TargetGroup>netstandard1.3</TargetGroup>
+      <TestTFMs>netcoreapp1.0</TestTFMs>
+    </Project>
+    <Project Include="System.Globalization.Tests.csproj" />


This is the configuration I would expect to be the first in the list.

weshaggard · 2016-10-01T21:07:49Z

+      <TargetGroup>netstandard1.3</TargetGroup>
    </Project>
    <Project Include="System.IO.FileSystem.Watcher.Tests.csproj">
-      <TargetGroup>netstandard1.7</TargetGroup>


It seems like it would have been better to just swap ns17 with the older builds so the diff would be logically ordered with the default builds at the top.

weshaggard · 2016-10-01T21:10:55Z

LGTM thanks for doing this @joperezr I added a few comments where the order is a little funky but I think they are technically correct. Perhaps we should just write a simple program to order all the .builds files to make them consistent.

joperezr · 2016-10-02T05:33:01Z

Sounds good, I'll write up a quick program and run it through the repo soon

…app11 Changing test projects to default to netcoreapp1.1 instead of netcoreapp1.0 Commit migrated from dotnet/corefx@76b46ea

dnfclas added the cla-already-signed label Sep 30, 2016

ericstj approved these changes Sep 30, 2016

View reviewed changes

ericeil reviewed Sep 30, 2016

View reviewed changes

joperezr force-pushed the DefaultToNetcoreapp11 branch from 0c68ed5 to f4b9864 Compare September 30, 2016 20:58

joperezr force-pushed the DefaultToNetcoreapp11 branch from 4542e8e to 7fe9193 Compare September 30, 2016 22:24

weshaggard reviewed Sep 30, 2016

View reviewed changes

weshaggard mentioned this pull request Oct 1, 2016

Add ExcludeFromCodeCoverageAttribute #11986

Merged

Changing test projects to default to netcoreapp1.1 instead of netcore…

d56046b

…app1.0

weshaggard mentioned this pull request Oct 1, 2016

Fix test publishing #12280

Merged

joperezr force-pushed the DefaultToNetcoreapp11 branch from 7fe9193 to d56046b Compare October 1, 2016 18:43

joperezr merged commit 76b46ea into dotnet:master Oct 1, 2016

weshaggard reviewed Oct 1, 2016

View reviewed changes

joperezr deleted the DefaultToNetcoreapp11 branch October 3, 2016 15:49

weshaggard mentioned this pull request Oct 3, 2016

Move S.IO.FileAccess to corelib. #12206

Merged

joperezr mentioned this pull request Oct 3, 2016

Fix ordering of test builds files #12312

Merged

hseok-oh mentioned this pull request Oct 7, 2016

Add '--testTFM' option and set default to netcoreapp1.1 in run-test.sh #12440

Merged

karelz modified the milestone: 1.2.0 Dec 3, 2016

		@@ -1,4 +1,4 @@
		<?xml version="1.0" encoding="utf-8"?>
		<?xml version="1.0" encoding="utf-8"?>

Conversation

joperezr commented Sep 30, 2016

Uh oh!

ericstj commented Sep 30, 2016

Uh oh!

ericstj Sep 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericstj Sep 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joperezr commented Sep 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joperezr commented Sep 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weshaggard commented Oct 1, 2016

Uh oh!

joperezr commented Oct 2, 2016

Uh oh!

Reviewers

ericstj Sep 30, 2016 •

edited

Loading

ericstj Sep 30, 2016 •

edited

Loading