Upgrade to VS 2017 #7542

bricelam · 2017-02-02T23:56:27Z

Resolves #7538

Providers/contributors:

Specification tests now target net452 (xUnit.net dropped net451)
EntityFramework.sln now requires Visual Studio 2017 RC. For running tests, TestDriven.NET still works

bricelam · 2017-02-03T00:04:06Z

I'm still verifying this, but it's getting close enough to start the review.

I can't get the VS Test runner to work (even after installing Microsoft.NETCore.App version 1.2.0), but TestDriven.NET works (so long as net451 is listed first).

VS 2017 RC.3 chokes during solution load and package restore, but d15rel works.

Looks like there are still issues with tests that compile:

InvalidOperationException: Assembly 'System.Runtime' not found.

Will investigate.

@natemcmaster Any tricks to PreserveCompilationContext?

natemcmaster

Minor formatting suggestions, but looks good

natemcmaster · 2017-02-02T23:59:15Z

src/Microsoft.EntityFrameworkCore.Design/Microsoft.EntityFrameworkCore.Design.csproj

+    <TargetFrameworks>net451;netstandard1.3</TargetFrameworks>
+    <NoWarn>CS1591</NoWarn>
+    <GenerateDocumentationFile>true</GenerateDocumentationFile>
+    <Serviceable>true</Serviceable>


Will be set automatically for Release builds by Internal.AspNetCore.Sdk

natemcmaster · 2017-02-03T00:00:35Z

src/Microsoft.EntityFrameworkCore.Design/Microsoft.EntityFrameworkCore.Design.csproj

-    <Compile Include="Scaffolding\Internal\ScaffoldingServiceCollectionExtensions.cs" />
-    <Compile Include="Scaffolding\Internal\ScaffoldingUtilities.cs" />
-    <Compile Include="Scaffolding\Internal\StringBuilderCodeWriter.cs" />
+    <ProjectReference Include="..\Microsoft.EntityFrameworkCore.Relational.Design\Microsoft.EntityFrameworkCore.Relational.Design.csproj" />
  </ItemGroup>
  <ItemGroup>


Nit: i've been condensing itemgroups so there is one block of package + project references. But up to your team to decide if you like separate blocks better.

I've observed that VS likes to create different groups for different item types. Following suit tends to keep edits via VS cleaner.

natemcmaster · 2017-02-03T00:01:48Z

src/Microsoft.EntityFrameworkCore.Relational/Microsoft.EntityFrameworkCore.Relational.csproj

-    <NoWarn>1591</NoWarn>
+    <Description>Shared Entity Framework Core components for relational database providers.</Description>
+    <TargetFrameworks>net451;netstandard1.3</TargetFrameworks>
+    <NoWarn>CS1591</NoWarn>


Nit: concat nowarns.

YAGNI, but we might need it, so... 😉

natemcmaster · 2017-02-03T00:03:15Z

src/Microsoft.EntityFrameworkCore.Relational/Microsoft.EntityFrameworkCore.Relational.csproj

-    </ProjectReference>
+  <ItemGroup Condition="'$(TargetFramework)' == 'net451'">
+    <!-- TODO: Make this a <frameworkAssembly> in the *.nuspec file. -->
+    <Reference Include="System.Transactions" />


Is there a nuget package for this? cref https://github.com/aspnet/Coherence-Signed/issues/389

Not until 2.0. Will remove ASAP.

(But it will no longer be propagated to the nuspec)

natemcmaster · 2017-02-03T00:03:31Z

...tyFrameworkCore.Specification.Tests/Microsoft.EntityFrameworkCore.Specification.Tests.csproj

-    <NoWarn>1591</NoWarn>
+    <Description>Shared test suite for Entity Framework Core database providers.</Description>
+    <TargetFrameworks>net451;netstandard1.3</TargetFrameworks>
+    <NoWarn>CS1591</NoWarn>


Nit: concat nowarn

natemcmaster · 2017-02-03T00:04:08Z

...tyFrameworkCore.Specification.Tests/Microsoft.EntityFrameworkCore.Specification.Tests.csproj

-      <Name>Microsoft.EntityFrameworkCore</Name>
-    </ProjectReference>
+    <PackageReference Include="Microsoft.CSharp" Version="4.4.0-*" />
+    <PackageReference Include="Microsoft.Extensions.RuntimeEnvironment.Sources" Version="1.2.0-*">


Nit: PrivateAssets can be an attribute instead of a nested element.

Feels more idiomatic as an element to me, but I might just be living in the past...

natemcmaster · 2017-02-03T00:08:37Z

Does this assembly resolution depend on current working directory? cref microsoft/vstest#311

Also, is this just .NET Framework or .NET core too?

natemcmaster · 2017-02-03T00:09:18Z

One more thing: add os: Visual Studio 2017 RC to appveyor.yml

bricelam · 2017-02-03T16:32:47Z

Also, is this just .NET Framework or .NET core too?

Just .NET Core

natemcmaster · 2017-02-03T16:43:19Z

🤔 hmm, not sure. Lmk if you figure out what's going on. I have similarly weird test failures on Entropy

bricelam · 2017-02-03T18:13:22Z

Looks like DependencyContext.Load() was returning null. Updating to DependencyContext.Default works.

bricelam · 2017-02-03T21:04:22Z

Getting closer. It looks like AppVeyor is getting an older build of Roslyn on net451.

bricelam · 2017-02-03T23:32:48Z

Tests pass locally.

natemcmaster · 2017-02-03T23:37:56Z

🎉

natemcmaster · 2017-02-04T01:40:36Z

Curious, how does test time compare? We've seen a huge test slowdown in Kestrel and other projects. cref microsoft/vstest#349 (comment)

natemcmaster · 2017-02-09T22:57:13Z

@bricelam I just pushed an update to KoreBuild which should make building big repos much faster. It builds solutions instead of individual csprojs. Can you verify it works with your changes?

dougbu · 2017-02-14T03:13:01Z

...ft.EntityFrameworkCore.Relational.Design.Specification.Tests/TestUtilities/BuildReference.cs

@@ -6,19 +6,14 @@
 using System.Reflection;
 using Microsoft.CodeAnalysis;

-#if NETSTANDARD1_7
+#if !NET451


This will conflict with #7607 changes. Will need to rebase and deal with the fallout, sorry.

dougbu · 2017-02-14T03:16:42Z

...e.CrossStore.FunctionalTests/Microsoft.EntityFrameworkCore.CrossStore.FunctionalTests.csproj

-    <FileAlignment>512</FileAlignment>
-    <NoWarn>1591</NoWarn>
+    <TargetFrameworks>net451;netcoreapp1.1</TargetFrameworks>
+    <RuntimeIdentifier Condition="'$(TargetFramework)' != 'netcoreapp1.1'">win7-x64</RuntimeIdentifier>


<TargetFrameworks>net452;netcoreapp1.1</TargetFrameworks> to match Bump test projects up to .NET 4.5.2 #7607

(channelling @natemcmaster because I couldn't fit this into Bump test projects up to .NET 4.5.2 #7607) add the following to automatically disable full desktop testing on non-Windows
<TargetFrameworks Condition=" '$(OS)' != 'Windows_NT' ">netcoreapp1.1</TargetFrameworks>

dougbu · 2017-02-14T03:18:17Z

...e.CrossStore.FunctionalTests/Microsoft.EntityFrameworkCore.CrossStore.FunctionalTests.csproj

-    <Compile Include="SqlServerCrossStoreFixture.cs" />
-    <Compile Include="TestModels\CrossStoreContext.cs" />
-    <Compile Include="TestModels\SimpleEntity.cs" />
-  </ItemGroup>
  <ItemGroup>
    <Content Include="..\Microsoft.EntityFrameworkCore.SqlServer.FunctionalTests\Northwind.sql">
      <Link>Northwind.sql</Link>
      <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
    </Content>


Wow @bricelam you are old school 😺 Haven't seen a <link> in forever.

dougbu · 2017-02-14T03:21:53Z

...e.CrossStore.FunctionalTests/Microsoft.EntityFrameworkCore.CrossStore.FunctionalTests.csproj

+    <PackageReference Include="Microsoft.NET.Test.Sdk" Version="15.0.0-*" />
+    <PackageReference Include="xunit" Version="2.2.0-*" />
+    <PackageReference Include="xunit.runner.visualstudio" Version="2.2.0-*" />


Lots of these test projects depend on each other or on Microsoft.EntityFrameworkCore.Specification.Tests. Should be bringing at least the xUnit package in transitively. Leave transitive dependencies out.

Hmm... By default, I don't think build scripts of packages are brought in transitively. I think I have to leave these since both xunit & xunit.runner.visualstudio have build scripts.

am11 · 2017-02-15T18:07:34Z

@bricelam, the travis build failed with:

The log length has exceeded the limit of 4 MB (this usually means that the test suite is raising the same exception over and over).

The job has been terminated

(https://api.travis-ci.org/jobs/201944010/log.txt?deansi=true)

Seems like this limitation is not configurable yet: travis-ci/travis-ci#3865.

Can the msbuild test target be configured from Korebuild so it only outputs errors and warnings from xunit execution, but not the passing tests (1>/dev/null for test runs only)?

bricelam · 2017-02-15T18:08:43Z

@natemcmaster Any way to make make the test runner shut up about passed tests?

natemcmaster · 2017-02-15T18:39:28Z

@bricelam not yet. See microsoft/vstest#301 (and add a vote for default output=minimal).

bricelam added the needs-review label Feb 2, 2017

bricelam requested a review from natemcmaster February 2, 2017 23:56

dnfclas added the cla-already-signed label Feb 2, 2017

natemcmaster approved these changes Feb 3, 2017

View reviewed changes

natemcmaster removed the needs-review label Feb 3, 2017

bricelam force-pushed the feature/msbuild branch from b5d9762 to c2729b7 Compare February 3, 2017 22:16

This was referenced Feb 13, 2017

Infrastructure: Upgrade to VS 2017 #7538

Closed

Bump test projects up to .NET 4.5.2 #7607

Closed

dougbu requested changes Feb 14, 2017

View reviewed changes

bricelam force-pushed the feature/msbuild branch 5 times, most recently from 0787416 to 5493d0a Compare February 15, 2017 17:45

bricelam added 2 commits February 15, 2017 10:04

dotnet migrate

ff2fecd

Skip code-compiling tests

33bf245

bricelam force-pushed the feature/msbuild branch from 5493d0a to 33bf245 Compare February 15, 2017 18:04

bricelam added the providers-beware label Feb 15, 2017

am11 mentioned this pull request Feb 15, 2017

Option to minimize console output on vstest.console microsoft/vstest#301

Closed

bricelam merged commit 33bf245 into dev Feb 15, 2017

bricelam deleted the feature/msbuild branch February 15, 2017 19:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to VS 2017 #7542

Upgrade to VS 2017 #7542

bricelam commented Feb 2, 2017 •

edited

bricelam commented Feb 3, 2017 •

edited

natemcmaster left a comment

natemcmaster Feb 2, 2017

natemcmaster Feb 3, 2017

bricelam Feb 3, 2017

natemcmaster Feb 3, 2017

bricelam Feb 3, 2017

natemcmaster Feb 3, 2017

bricelam Feb 3, 2017 •

edited

bricelam Feb 3, 2017

natemcmaster Feb 3, 2017

natemcmaster Feb 3, 2017

bricelam Feb 3, 2017

natemcmaster commented Feb 3, 2017 •

edited

natemcmaster commented Feb 3, 2017

bricelam commented Feb 3, 2017

natemcmaster commented Feb 3, 2017

bricelam commented Feb 3, 2017

bricelam commented Feb 3, 2017 •

edited

bricelam commented Feb 3, 2017

natemcmaster commented Feb 3, 2017

natemcmaster commented Feb 4, 2017

natemcmaster commented Feb 9, 2017

dougbu Feb 14, 2017

dougbu Feb 14, 2017

dougbu Feb 14, 2017

dougbu Feb 14, 2017

bricelam Feb 14, 2017

am11 commented Feb 15, 2017

bricelam commented Feb 15, 2017

natemcmaster commented Feb 15, 2017

Upgrade to VS 2017 #7542

Upgrade to VS 2017 #7542

Conversation

bricelam commented Feb 2, 2017 • edited

bricelam commented Feb 3, 2017 • edited

natemcmaster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bricelam Feb 3, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natemcmaster commented Feb 3, 2017 • edited

natemcmaster commented Feb 3, 2017

bricelam commented Feb 3, 2017

natemcmaster commented Feb 3, 2017

bricelam commented Feb 3, 2017

bricelam commented Feb 3, 2017 • edited

bricelam commented Feb 3, 2017

natemcmaster commented Feb 3, 2017

natemcmaster commented Feb 4, 2017

natemcmaster commented Feb 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

am11 commented Feb 15, 2017

bricelam commented Feb 15, 2017

natemcmaster commented Feb 15, 2017

bricelam commented Feb 2, 2017 •

edited

bricelam commented Feb 3, 2017 •

edited

bricelam Feb 3, 2017 •

edited

natemcmaster commented Feb 3, 2017 •

edited

bricelam commented Feb 3, 2017 •

edited