-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Upgrade to VS 2017 #7542
Upgrade to VS 2017 #7542
Conversation
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 VS 2017 RC.3 chokes during solution load and package restore, but d15rel works. Looks like there are still issues with tests that compile:
Will investigate. @natemcmaster Any tricks to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor formatting suggestions, but looks good
<TargetFrameworks>net451;netstandard1.3</TargetFrameworks> | ||
<NoWarn>CS1591</NoWarn> | ||
<GenerateDocumentationFile>true</GenerateDocumentationFile> | ||
<Serviceable>true</Serviceable> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will be set automatically for Release builds by Internal.AspNetCore.Sdk
<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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've observed that VS likes to create different groups for different item types. Following suit tends to keep edits via VS cleaner.
<NoWarn>1591</NoWarn> | ||
<Description>Shared Entity Framework Core components for relational database providers.</Description> | ||
<TargetFrameworks>net451;netstandard1.3</TargetFrameworks> | ||
<NoWarn>CS1591</NoWarn> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: concat nowarns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
YAGNI, but we might need it, so... 😉
</ProjectReference> | ||
<ItemGroup Condition="'$(TargetFramework)' == 'net451'"> | ||
<!-- TODO: Make this a <frameworkAssembly> in the *.nuspec file. --> | ||
<Reference Include="System.Transactions" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a nuget package for this? cref https://github.com/aspnet/Coherence-Signed/issues/389
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not until 2.0. Will remove ASAP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(But it will no longer be propagated to the nuspec)
<NoWarn>1591</NoWarn> | ||
<Description>Shared test suite for Entity Framework Core database providers.</Description> | ||
<TargetFrameworks>net451;netstandard1.3</TargetFrameworks> | ||
<NoWarn>CS1591</NoWarn> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: concat nowarn
<Name>Microsoft.EntityFrameworkCore</Name> | ||
</ProjectReference> | ||
<PackageReference Include="Microsoft.CSharp" Version="4.4.0-*" /> | ||
<PackageReference Include="Microsoft.Extensions.RuntimeEnvironment.Sources" Version="1.2.0-*"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: PrivateAssets can be an attribute instead of a nested element.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels more idiomatic as an element to me, but I might just be living in the past...
Does this assembly resolution depend on current working directory? cref microsoft/vstest#311 Also, is this just .NET Framework or .NET core too? |
One more thing: add |
Just .NET Core |
🤔 hmm, not sure. Lmk if you figure out what's going on. I have similarly weird test failures on Entropy |
Looks like |
Getting closer. It looks like AppVeyor is getting an older build of Roslyn on net451. |
b5d9762
to
c2729b7
Compare
Tests pass locally. |
🎉 |
Curious, how does test time compare? We've seen a huge test slowdown in Kestrel and other projects. cref microsoft/vstest#349 (comment) |
@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? |
@@ -6,19 +6,14 @@ | |||
using System.Reflection; | |||
using Microsoft.CodeAnalysis; | |||
|
|||
#if NETSTANDARD1_7 | |||
#if !NET451 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will conflict with #7607 changes. Will need to rebase and deal with the fallout, sorry.
<FileAlignment>512</FileAlignment> | ||
<NoWarn>1591</NoWarn> | ||
<TargetFrameworks>net451;netcoreapp1.1</TargetFrameworks> | ||
<RuntimeIdentifier Condition="'$(TargetFramework)' != 'netcoreapp1.1'">win7-x64</RuntimeIdentifier> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<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>
<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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow @bricelam you are old school 😺 Haven't seen a <link>
in forever.
<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-*" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
0787416
to
5493d0a
Compare
5493d0a
to
33bf245
Compare
@bricelam, the travis build failed with:
(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 ( |
@natemcmaster Any way to make make the test runner shut up about passed tests? |
@bricelam not yet. See microsoft/vstest#301 (and add a vote for default output=minimal). |
Resolves #7538
Providers/contributors:
net452
(xUnit.net droppednet451
)EntityFramework.sln
now requires Visual Studio 2017 RC. For running tests, TestDriven.NET still workscc @ErikEJ @roji @tuespetre