Upgrade to VS 2017 #925
Upgrade to VS 2017 #925
Conversation
|
Those baselines should be moved from Microsoft.AspNetCore.Server.Testing to Microsoft.AspNetCore.Server.IntegrationTesting |
|
That project isn't an automated test project. It's a test app. |
|
Regarding baselines, if the project got renamed, there's no point renaming the baseline, because the whole thing is a breaking change. Is this a package we publish? Should we even be checking for breaking changes in it? |
|
I'm not sure the baseline files were ever used. They were added in 911da31, but even then, there was no project named Microsoft.AspNetCore.Server.Testing. See https://github.com/aspnet/Hosting/tree/911da31476615a5ce4bfe2c9a59cffeddb8c2422/src/Microsoft.AspNetCore.Server.Testing |
| <PropertyGroup> | ||
| <Description>ASP.NET Core hosting server abstractions for web applications.</Description> | ||
| <TargetFrameworks>net451;netstandard1.3</TargetFrameworks> | ||
| <NoWarn>$(NoWarn);CS1591</NoWarn> |
There was a problem hiding this comment.
Should this go into common.props?
There was a problem hiding this comment.
I think not. It's nice to keep this a little more visible as a reminder that we are missing XML docs in some API
There was a problem hiding this comment.
If we used the internal keyword more I would agree. The fact that we have this in every single project is why I think it should go into the common props files.
There was a problem hiding this comment.
I still prefer it in the project, right next to the GenerateDocumentationFile flag, and for the sake of consistency about what common.props is really for... but I don't care a whole lot either way.
There was a problem hiding this comment.
...and I'd guess most devs won't care either.
There was a problem hiding this comment.
I'm with @natemcmaster here, we should leave it per-project. We should ideally do what we did in EF and put explicit docs for .Internal types saying they're not supported. But, until then...
| <Project Sdk="Microsoft.NET.Sdk"> | ||
|
|
||
| <PropertyGroup> | ||
| <TargetFrameworks>net451;netcoreapp1.1</TargetFrameworks> |
There was a problem hiding this comment.
Need a RuntimeIdentifier here?
There was a problem hiding this comment.
No. This is a class library. RID is only required for exe projects, and as a workaround for dotnet/sdk#396
There was a problem hiding this comment.
Looking closer its neither, not supposed to be run, just showing examples of how startups work
Remove project.json and upgrade to MSBuild.
Other notable changes:
Removed unused baseline.json files. There were baseline files for "Microsoft.AspNetCore.Server.Testing" package, even though there was no project by that name. cc @javiercn @Eilon