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

Add ASP.NET Core runtime version test #7227

Merged
merged 11 commits into from
Apr 15, 2021

Conversation

jonfortescue
Copy link
Contributor

@jonfortescue jonfortescue commented Apr 12, 2021

Closes #7022.

Adds a test to verify that the hardcoded version of the ASP.NET Core runtime matches the .NET SDK version.

Example console logs (from UnitTests.proj): Passing
Example console logs (from testing proj): Passing, Failing due to mismatch, Failing due to invalid version number

To double check:

@jonfortescue
Copy link
Contributor Author

Forgot to make it only run on Windows lol. One sec.

Copy link
Member

@MattGal MattGal left a comment

Choose a reason for hiding this comment

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

LGTM, was going to make the windows comment but you beat me to it.

tests/UnitTests.proj Outdated Show resolved Hide resolved
tests/ASPNETVersionCheck/aspnet-versioncheck.ps1 Outdated Show resolved Hide resolved
@jonfortescue jonfortescue self-assigned this Apr 12, 2021
Copy link
Member

@premun premun left a comment

Choose a reason for hiding this comment

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

Looks good!

I can see one possible way this could fail - if someone went and changed the the type of .NET in the unit tests from SDK to ASP.NET Runtime. Then we would be testing ASPNET vs ASPNET and it would always succeed. But I guess other parts of the unit tests would fail and this won't really happen.

@@ -1,10 +1,11 @@
<Project>
<PropertyGroup>
<IncludeDotNetCli Condition=" '$(IncludeDotNetCli)' != 'true' ">false</IncludeDotNetCli>
<AspNetCoreRuntimeVersion>6.0.0-preview.2.21154.6</AspNetCoreRuntimeVersion>
Copy link
Member

Choose a reason for hiding this comment

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

Can you also please use this property inside XHarnessRunner.targets?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@premun Check out the latest push and see if you think this will work.

Copy link
Member

Choose a reason for hiding this comment

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

@jonfortescue it didn't work out of the box? without pulling the property in its own file?

I think the whole DotNetCli is included for Helix SDK already

Copy link
Member

Choose a reason for hiding this comment

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

I am meaning to say - did you just try using the property without any other changes?

@premun
Copy link
Member

premun commented Apr 13, 2021

@jonfortescue seems like Microsoft.DotNet.Helix.Sdk.targets includes all props on line 5:

  <Import Project="$(MSBuildThisFileDirectory)*/*.props"/>

image

If you went few commits back and didn't pull the property in its own new file, and just define it in DotNetCli.props, things should work still and the property should be visible.

I might be wrong but can you verify that please?

@jonfortescue
Copy link
Contributor Author

@premun oooh neat. let's test it.

Copy link
Member

@premun premun left a comment

Choose a reason for hiding this comment

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

Nice, should work well!

Also we should probably move to .NET 6.0 preview 3 in Arcade

@jonfortescue jonfortescue merged commit 89ebfff into dotnet:main Apr 15, 2021
@premun premun mentioned this pull request Mar 28, 2022
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix setting of the DotNetCliVersion for ASP.NET Core runtime
3 participants