Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Replace netstandard test project tfms with implementations #37972

Merged
merged 6 commits into from
May 30, 2019
Merged

Replace netstandard test project tfms with implementations #37972

merged 6 commits into from
May 30, 2019

Conversation

ViktorHofer
Copy link
Member

@ViktorHofer ViktorHofer commented May 27, 2019 •
edited
Loading

Netstandard test projects are broken by design and don't support the
Microsoft.Net.Test.SDK. Replacing the netstandard tfm with the concrete
implementation tfms. This helps enabling project restore for test projects
and brings us closer to support VS Test Explorer. This is one of the goals outlined
in my infra spec that I sent around last week.

Fixes https://github.com/dotnet/corefx/issues/35088

Changes (only in test projects):

  • Replace netstandard tfms with concrete frameworks
  • Remove uapaot leftovers
  • Remove netfx tfm, NetFramework SkipOnTargetFramework and IsFullFramework conditions where we are not live-building or shipping netfx assets. I left some netfx comments in cases where context is helpful.
  • Minor code cleanup (removing a few Service items that VS added)
  • Fixed a project which had wrong configurations set and was missing a Configurations.props file

@danmosemsft @safern I know this is a huge diff but the changes are very scoped. Do you think you could review the PR?

cc @ericstj

@ViktorHofer ViktorHofer self-assigned this May 27, 2019
@ViktorHofer ViktorHofer changed the title Replace netstandard test projects with tfms Replace netstandard test projects with implementation tfms May 27, 2019
@davidsh
Copy link
Contributor

davidsh commented May 27, 2019

Remove netfx tfm, NetFramework SkipOnTargetFramework and IsFullFramework conditions where we are not live-building or shipping netfx assets.

Why do this? We usually add SkipOnTargetFramework for netfx because the test will fail on netfx (i.e. a behavior or bug fix isn't present on .NET Framework). So, removing the SkipOnTargetFramework will cause the test to fail. Are we no longer running tests against .NET Framework at all?

@ViktorHofer
Copy link
Member Author

The top post should contain all the necessary information:

Netstandard test projects are broken by design and don't support the
Microsoft.Net.Test.SDK. Replacing the netstandard tfm with the concrete
implementation tfms. This helps enabling project restore for test projects
and brings us closer to support VS Test Explorer.

Are we no longer running tests against .NET Framework at all?

With this PR we are still running tests against netfx but only selectively for assemblies which we ship as a package. For more details see the linked issue.

@ViktorHofer ViktorHofer changed the title Replace netstandard test projects with implementation tfms Replace netstandard test project tfms with implementations May 28, 2019
@ViktorHofer ViktorHofer force-pushed the NSTestRemoval branch 17 times, most recently from 974a95c to cce075b Compare May 30, 2019 11:02
@ViktorHofer ViktorHofer requested a review from danmoseley May 30, 2019 11:14
@ViktorHofer ViktorHofer force-pushed the NSTestRemoval branch 4 times, most recently from 89d30b4 to cfc07e0 Compare May 30, 2019 13:11
@ViktorHofer
Copy link
Member Author

This is now ready to go in, tested multiple times in CI, no related failures. I expect merge conflicts but would like to avoid them thus it would be great if somebody could review this 😊

I reviewed my changes myself couple of times, I'm confident that they are safe.

safern
safern reviewed May 30, 2019
View reviewed changes
src/System.Linq.Expressions/tests/System.Linq.Expressions.Tests.csproj
</PropertyGroup>
<PropertyGroup>
<ProjectGuid>{4B4AA59B-89F9-4A34-B3C3-C97EF531EE00}</ProjectGuid>
<IsInterpreting Condition="'$(TargetGroup)' == 'uapaot'">true</IsInterpreting>
<IsInterpreting>false</IsInterpreting>
<DefineConstants Condition=" '$(IsInterpreting)' != 'true' ">$(DefineConstants);FEATURE_COMPILE</DefineConstants>
<DefineConstants Condition=" '$(FeatureInterpret)' == 'true' ">$(DefineConstants);FEATURE_INTERPRET</DefineConstants>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like FeatureInterpret is uapaot feature? Maybe this can be removed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this can be removed but I left it there as it might be interesting for some Mono bring-up. Not a deal breaker atm.

safern
safern reviewed May 30, 2019
View reviewed changes
src/System.Linq/tests/Configurations.props
@@ -2,8 +2,6 @@
<PropertyGroup>
<BuildConfigurations>
netcoreapp;
netstandard;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No uap configuration?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll follow-up on this one.

safern
safern approved these changes May 30, 2019
View reviewed changes
Copy link
Member

@safern safern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good to me (of course there are a lot of files so something could've slipped).

Left some comments, though

@ViktorHofer
Copy link
Member Author

@safern thanks a lot for reviewing this!

@ViktorHofer
Copy link
Member Author

Merging as CI was "green" in the last round and to avoid conflicts again.

@ViktorHofer ViktorHofer merged commit 30ca411 into dotnet:master May 30, 2019
@ViktorHofer ViktorHofer deleted the NSTestRemoval branch May 30, 2019 22:26
@jkotas jkotas mentioned this pull request Jun 23, 2019
Closed
@davidsh davidsh mentioned this pull request Jun 24, 2019
Merged
@karelz karelz added this to the 3.0 milestone Jul 16, 2019
@filipnavara filipnavara mentioned this pull request Mar 17, 2020
Merged
macrogreg pushed a commit to open-telemetry/opentelemetry-dotnet-instrumentation that referenced this pull request Sep 24, 2020
@ViktorHofer
Merge pull request dotnet/corefx#37972 from ViktorHofer/NSTestRemoval
944af3d 
Replace netstandard test project tfms with implementations

Commit migrated from dotnet/corefx@30ca411
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
@ViktorHofer
Merge pull request dotnet/corefx#37972 from ViktorHofer/NSTestRemoval
f3f516a 
Replace netstandard test project tfms with implementations

Commit migrated from dotnet/corefx@30ca411
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@safern safern safern approved these changes

@danmoseley danmoseley Awaiting requested review from danmoseley

Assignees

@ViktorHofer ViktorHofer

Labels
area-Infrastructure-libraries
Projects
None yet
Milestone
3.0
Development

Successfully merging this pull request may close these issues.

Delete tests specific to .NET Framework?
5 participants
@ViktorHofer @davidsh @danmoseley @safern @karelz