Skip to content
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

Swap compiler test target frameworks to put netcoreapp first #40859

Merged
merged 3 commits into from Jan 13, 2020

Conversation

@agocke
Copy link
Contributor

agocke commented Jan 9, 2020

This will cause local Visual Studio and VS Code test runners to run under
CoreCLR by default. This will only affect tests which are multi-targeted,
which today only includes the compiler unit tests.

@agocke agocke force-pushed the agocke:swap-tfs branch from 9d1c34a to 568d8d4 Jan 9, 2020
@sharwell

This comment has been minimized.

Copy link
Member

sharwell commented Jan 9, 2020

Doesn't the new test explorer window run both according to which test you have selected to run?

@agocke agocke force-pushed the agocke:swap-tfs branch from 568d8d4 to c9a9140 Jan 9, 2020
This will cause local Visual Studio and VS Code test runners to run under
CoreCLR by default. This will only affect tests which are multi-targeted,
which today only includes the compiler unit tests.
@agocke agocke force-pushed the agocke:swap-tfs branch from c9a9140 to a8c7e3f Jan 10, 2020
@agocke

This comment has been minimized.

Copy link
Contributor Author

agocke commented Jan 10, 2020

@sharwell As far as I can tell, no. If you just right click in the editor and hit "run test" it only runs the test in the framework that is first in the list of target frameworks.

@agocke agocke marked this pull request as ready for review Jan 10, 2020
@agocke agocke requested review from dotnet/roslyn-compiler as code owners Jan 10, 2020
@agocke agocke requested a review from jaredpar Jan 10, 2020
@@ -18,6 +18,7 @@
<ToolsetPackagesDir>$(RepoRoot)build\ToolsetPackages\</ToolsetPackagesDir>

<RoslynPortableTargetFrameworks>net472;netcoreapp2.1</RoslynPortableTargetFrameworks>

This comment has been minimized.

Copy link
@jasonmalinowski

jasonmalinowski Jan 10, 2020

Member

Why not just reverse this variable instead? (I assume I'm going to regret asking this question.)

This comment has been minimized.

Copy link
@agocke

agocke Jan 11, 2020

Author Contributor

Doesn't affect just the tests. I'm not opposed to it, just not what this change was intended to do.

This comment has been minimized.

Copy link
@jasonmalinowski

jasonmalinowski Jan 11, 2020

Member

What else cares about it besides the test tooling?

This comment has been minimized.

Copy link
@agocke

agocke Jan 11, 2020

Author Contributor

The target frameworks themselves may be different for executables and tests. For instance, we may want to move the tests to target netcoreapp50 and keep the executables on 3.1

This comment has been minimized.

Copy link
@agocke

agocke Jan 11, 2020

Author Contributor

That's not now, obviously, but I wanted to plan for future changes.

This comment has been minimized.

Copy link
@jaredpar

jaredpar Jan 13, 2020

Member

The target frameworks themselves may be different for executables and tests

This is true however the way we generally handle it is by special casing that test project and having a custom <TargetFrameworks>. These exceptions exist today because of netcoreapp3.0 and DIM. I would expect them to exist again in future releases.

I'm not opposed to it, just not what this change was intended to do.

I 100% think we should just be swapping out the order here vs. adding a new property. There is no reason to have a different order here for test and non-test projects. The only real impact the order has here is changing the default tests that are run which is the entire point of this PR.

Copy link
Member

jaredpar left a comment

Really think we should just be flipping the order here vs. introducing a new property.

@@ -18,6 +18,7 @@
<ToolsetPackagesDir>$(RepoRoot)build\ToolsetPackages\</ToolsetPackagesDir>

<RoslynPortableTargetFrameworks>net472;netcoreapp2.1</RoslynPortableTargetFrameworks>

This comment has been minimized.

Copy link
@jaredpar

jaredpar Jan 13, 2020

Member

The target frameworks themselves may be different for executables and tests

This is true however the way we generally handle it is by special casing that test project and having a custom <TargetFrameworks>. These exceptions exist today because of netcoreapp3.0 and DIM. I would expect them to exist again in future releases.

I'm not opposed to it, just not what this change was intended to do.

I 100% think we should just be swapping out the order here vs. adding a new property. There is no reason to have a different order here for test and non-test projects. The only real impact the order has here is changing the default tests that are run which is the entire point of this PR.

@333fred

This comment has been minimized.

Copy link
Member

333fred commented Jan 13, 2020

Agreed with Jared. This seems too complicated.

@agocke

This comment has been minimized.

Copy link
Contributor Author

agocke commented Jan 13, 2020

Fine with me :)

@agocke agocke merged commit 1c04026 into dotnet:master Jan 13, 2020
18 checks passed
18 checks passed
WIP Ready for review
Details
license/cla All CLA requirements met.
Details
roslyn-CI Build #20200113.24 succeeded
Details
roslyn-CI (Linux_Test coreclr) Linux_Test coreclr succeeded
Details
roslyn-CI (SourceBuild_Test) SourceBuild_Test succeeded
Details
roslyn-CI (Windows_CoreClr_Unit_Tests debug) Windows_CoreClr_Unit_Tests debug succeeded
Details
roslyn-CI (Windows_CoreClr_Unit_Tests release) Windows_CoreClr_Unit_Tests release succeeded
Details
roslyn-CI (Windows_Correctness_Test) Windows_Correctness_Test succeeded
Details
roslyn-CI (Windows_Desktop_Spanish_Unit_Tests) Windows_Desktop_Spanish_Unit_Tests succeeded
Details
roslyn-CI (Windows_Desktop_Unit_Tests debug_32) Windows_Desktop_Unit_Tests debug_32 succeeded
Details
roslyn-CI (Windows_Desktop_Unit_Tests debug_64) Windows_Desktop_Unit_Tests debug_64 succeeded
Details
roslyn-CI (Windows_Desktop_Unit_Tests release_32) Windows_Desktop_Unit_Tests release_32 succeeded
Details
roslyn-CI (Windows_Desktop_Unit_Tests release_64) Windows_Desktop_Unit_Tests release_64 succeeded
Details
roslyn-CI (Windows_Determinism_Test) Windows_Determinism_Test succeeded
Details
roslyn-CI (macOS_Test) macOS_Test succeeded
Details
roslyn-integration-CI Build #20200113.24 succeeded
Details
roslyn-integration-CI (VS_Integration debug_async) VS_Integration debug_async succeeded
Details
roslyn-integration-CI (VS_Integration release_async) VS_Integration release_async succeeded
Details
@agocke agocke deleted the agocke:swap-tfs branch Jan 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.