-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Reenable framework unit tests on helix #19975
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
Conversation
@dougbu @dotnet/aspnet-build does anyone know where the .version file in the sharedruntime comes from? I see a |
aspnetcore/src/Framework/src/Microsoft.AspNetCore.App.Runtime.csproj Lines 254 to 270 in ce0886c
And that target should run unconditionally. |
@dougbu does that mean it should be in the app runtime nupkgs of our artifacts? https://dev.azure.com/dnceng/public/_build/results?buildId=567706&view=artifacts&type=publishedArtifacts |
Yes. I'm not sure why Microsoft.AspNetCore.App.Runtime.linux-arm.5.0.0-ci.nupkg and 5.0.0-preview.3.20170.3 (from an official build and in the netcore5 feed) don't include the file. Maybe it's not supposed to ship? @wtgodbe and @JunTaoLuo shouldn't there be a .versions file in our runtime packages? Or, is that one of the outstanding issues on the Build plate? |
Looks like there's no ThirdPartyNotices.txt file in there either - something isn't working right in that project, I'm guessing. I can take a look on Monday |
Well I guess this is actually an argument in favor of running these framework tests on helix (which runs against the shared framework built artifacts) since the versions file does exist in today's runs of these tests...so there's a difference somewhere...? |
Marking as ready for review, I've skipped the failing .versions file for now and filed #20086 to track resolving that. I left the tests running on both azdo and helix for now until we are able to resolve the .Versions check |
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 suspect we need to do more to prepare for servicing. But, I defer to @JunTaoLuo on whether this migration will make such work more difficult. @JunTaoLuo please review.
@HaoK after https://github.com/dotnet/aspnetcore/pull/20250/files you should be able to un-disable |
Awesome thanks @wtgodbe! @JunTaoLuo are you ok with these tests being enabled as they are for now? Do you want me to file an issue for any servicing concerns about these tests so you can revisit this in the future? |
bb131ac
to
a2c7182
Compare
Update Microsoft.AspNetCore.App.UnitTests.csproj Update TargetingPackTests.cs Update SharedFxTests.cs Update TargetingPackTests.cs Update InstallAspNetRef.ps1 Update Helix.targets Create installaspnetref.sh Update SharedFxTests.cs Update TargetingPackTests.cs Update SharedFxTests.cs Fix runtimeVersion Update runtests.cmd Update runtests.cmd Update runtests.sh Update runtests.sh Update runtests.cmd Update SharedFxTests.cs Update runtests.cmd Update runtests.cmd Update InstallAppRuntime.ps1 Update runtests.sh Skip versions file check on helix for now Update Microsoft.AspNetCore.App.UnitTests.csproj Run unit tests on azdo too Update SharedFxTests.cs Update SharedFxTests.cs Update Microsoft.AspNetCore.App.UnitTests.csproj
Ok finally the framework tests are all green again, @dougbu @JunTaoLuo can you take a final look |
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.
One nit
@@ -0,0 +1,15 @@ | |||
#!/usr/bin/env bash |
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.
Can we migrate these to C#
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.
Yeah i'll file an issue for this and the other one (shared runtime)
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.
No description provided.