Add implicit PackageTargetFallback for net461 for netcoreapp and netstandard 2.0 #1133

livarcocc · 2017-04-19T21:25:10Z

@dotnet/dotnet-cli

…t fallback.

gulbanana · 2017-04-20T05:46:55Z

So if i want to build netstandard 2.0 libraries which don't risk accidentally referencing netfx packages, would it be correct to set property DisableImplicitPackageTargetFallback to True?

… It returns not zero when tests are skipped as well. Instead, let's rely on Jenkins validating the xml file. Fixing the test that I implicit set to fail to test Jenkins xml test parsing. Also changing the name of the test projects to avoid conflict when using the files.

livarcocc · 2017-04-20T17:04:40Z

@gulbanana yes. also, unless you explicitly turned it off, when ptf is used, you will get a warning.

livarcocc · 2017-04-20T21:40:09Z

@dotnet-bot Test Windows_NT_FullFramework Debug

livarcocc · 2017-04-20T21:55:25Z

@dsplaisted @nguerrera PTAL

nguerrera · 2017-04-20T22:56:49Z

build/build.proj

-    </Exec>
-
-    <Error Text="There were test failures, for full log see %(XmlTestFile.FullPath)" Condition="$(ExitCode) != 0" />
-


Why is this removed?

Removed it because xunit returns non-zero even when we have skipped tests. Which now that we have skip attributes happened and caused the build to fail. We don't need this anymore because we are parsing the testResult.xml file and jenkins will fail if there are any failed tests in there.

nguerrera · 2017-04-20T22:58:25Z

test/Microsoft.NET.Build.Tests/DeleteNuGetArtifactsFixture.cs

+            }
+            catch
+            {
+                // No-Op; as this is a precaution - do not throw an exception.


I know you just moved this, but it's sloppy to catch all exceptions. We can introduce any bug at all in the try block and never know. Why is it just a precaution to remove these artifacts?

@johnbeisner can you help clarify this?

nguerrera · 2017-04-20T22:59:16Z

test/Microsoft.NET.Build.Tests/GivenThatWeWantToVerifyNuGetReferenceCompat.cs

            }
        }

+        [WindowsOnlyTheoryAttribute]


Nit: drop the Attribute suffix.

Oops. Copy/paste. Will do.

nguerrera · 2017-04-20T23:03:03Z

test/Microsoft.NET.Build.Tests/GivenThatWeWantToVerifyNuGetReferenceCompat.cs

+        [InlineData("netcoreapp2.0")]
+        public void Net461_is_implicit_for_Netstandard_and_Netcore_20(string targetFramework)
+        {
+            if (UsingFullFrameworkMSBuild)


While we're getting better at xunit, should we have some sort of attribute to indicate core msbuild only?

nguerrera · 2017-04-20T23:10:30Z

test/Microsoft.NET.TestFramework/ProjectConstruction/TestProject.cs


+            foreach (var additionalProperty in AdditionalProperties)
+            {
+                propertyGroup.Add(new XElement($"{ns}{additionalProperty.Key}", additionalProperty.Value));


This should be ns + additionalProperty.Key like the others above.

You want https://msdn.microsoft.com/en-us/library/system.xml.linq.xnamespace.op_addition(v=vs.110).aspx, not string concatenation. I suspect this is only working now because the projects under test have no namespace declared.

dsplaisted · 2017-04-21T01:15:59Z

test/Microsoft.NET.Build.Tests/GivenThatWeWantToVerifyNuGetReferenceCompat.cs

+        [WindowsOnlyTheoryAttribute]
+        [InlineData("netstandard2.0")]
+        [InlineData("netcoreapp2.0")]
+        public void Net461_is_implicit_for_Netstandard_and_Netcore_20(string targetFramework)


I wouldn't include the values of the theory parameters in the name of the test. I'd call it something like Net461_is_implicit_package_target_fallback.

dsplaisted · 2017-04-21T01:22:36Z

test/Microsoft.NET.Build.Tests/GivenThatWeWantToVerifyNuGetReferenceCompat.cs

+            string targetFramework,
+            Dictionary<string, string> additionalProperties = null)
+        {
+            _net461PackageReference = CreateNet461Package();


It feels messy to use a field of the class to pass this object around. I think I'm used to tests not storing any instance state.

dsplaisted · 2017-04-21T01:23:43Z

test/Microsoft.NET.Build.Tests/GivenThatWeWantToVerifyNuGetReferenceCompat.cs

+            var net461Project = 
+                new TestProject
+                {
+                    Name = $"net461_pkg",


Nit: no need for $ in front of string here.

dsplaisted · 2017-04-21T01:32:59Z

test/Microsoft.NET.Build.Tests/GivenThatWeWantToVerifyNuGetReferenceCompat.cs

+        }
+
+        [WindowsOnlyFact]
+        public void It_is_possible_to_disabled_net461_implicit_package_target_fallback()


Nit: change disabled to disable in the method name.

…s can be more xunity.

nguerrera · 2017-04-21T17:51:33Z

test/Microsoft.NET.TestFramework/CoreMSBuildAndWindowsOnlyTheoryAttribute.cs

+
+namespace Microsoft.NET.TestFramework
+{
+    public class CoreMSBuildAndWindowsOnlyTheoryAttribute : TheoryAttribute


Roslyn has a nice pattern for combining common skip conditions: https://github.com/dotnet/roslyn/blob/master/src/Test/Utilities/Portable/Assert/ConditionalFactAttribute.cs

I think we should steal it as it allows composing things like WindowsOnly and CoreMSBuildOnly and also has the benefit of consistency with other dotnet repos.

Can we do that in a separate PR though? I would like to merge this one and then we can bring that over.

I agree that's much better.

livarcocc and others added 2 commits April 19, 2017 11:44

Adding a first test and net461 as a implicit package target fallback.

3972b68

Adding more tests and an option to disable the implicit package targe…

27d0dc7

…t fallback.

dnfclas added the cla-already-signed label Apr 19, 2017

livarcocc force-pushed the ptf_461 branch from 627b482 to dc3aefb Compare April 20, 2017 00:47

livarcocc force-pushed the ptf_461 branch from dc3aefb to 94b99b8 Compare April 20, 2017 06:08

livarcocc force-pushed the ptf_461 branch from 94b99b8 to 5d4c9cd Compare April 20, 2017 07:00

livarcocc force-pushed the ptf_461 branch from c68b237 to 54c0e44 Compare April 20, 2017 19:46

Reducing the path length of the net 461 package project.

aa416d0

livarcocc force-pushed the ptf_461 branch from 54c0e44 to aa416d0 Compare April 20, 2017 20:44

nguerrera approved these changes Apr 20, 2017

View reviewed changes

dsplaisted approved these changes Apr 21, 2017

View reviewed changes

Addressing review feedback and adding new attributes so that our test…

dc11535

…s can be more xunity.

nguerrera reviewed Apr 21, 2017

View reviewed changes

livarcocc merged commit af6530c into dotnet:master Apr 21, 2017

livarcocc deleted the ptf_461 branch April 21, 2017 18:31

This was referenced May 1, 2017

Restore on .NET Core 2.0 project imports .NET Framework 3.5 package targets NuGet/Home#5149

Closed

.NET Core 2.0 restore imports .NET Framework targets from packages #1166

Closed

JunTaoLuo mentioned this pull request May 19, 2017

Exclude build assets from sqlitepclraw and dependencies aspnet/MetaPackages#119

Merged

		</Exec>

		<Error Text="There were test failures, for full log see %(XmlTestFile.FullPath)" Condition="$(ExitCode) != 0" />

Add implicit PackageTargetFallback for net461 for netcoreapp and netstandard 2.0 #1133

Add implicit PackageTargetFallback for net461 for netcoreapp and netstandard 2.0 #1133

Uh oh!

Conversation

livarcocc commented Apr 19, 2017

Uh oh!

gulbanana commented Apr 20, 2017

Uh oh!

livarcocc commented Apr 20, 2017

Uh oh!

livarcocc commented Apr 20, 2017

Uh oh!

livarcocc commented Apr 20, 2017

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants