-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Add sharedfx and targeting pack tests (#23045) #23070
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
* Add test for assembly versions * Add test for framework list * Add some hardcoded lists for sharedfx and targeting pack content
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.
Please test the targeting pack part of this offline before merging, if you haven't already. Not building them right now.
And, to confirm: The hard-coded lists of expected assemblies isn't quite identical to what we're looking for in 'master', correct❔
Yea I'll be tweaking a few of the details before merging. |
src/Framework/test/SharedFxTests.cs
Outdated
|
||
if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) | ||
{ | ||
listedSharedFxAssemblies.Remove("aspnetcorev2_inprocess"); |
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.
Hmm now I'm wondering why this test didn't fail in master without adding this.
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.
Ah it's probably because it's testing the targeting pack that was built on Windows, given that we run the test on Helix. It should be benign then since we only ship the package built on Windows anyway.
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.
This is testing the shared framework and we ship that for every platform.
- Should not muck with the
ListedSharedFxAssemblies
value in a single test. This means other tests may fail depending on test execution order. TheTestData
value itself should conditionally exclude the native assembly. - It's not clear how this test is passing on non-Windows platforms in 'master'. Please figure that out.
Separately, this won't work unless you also exclude the ANCM assembly on Windows ARM and ARM64. Probably better to copy the $(VerifyAncmBinary)
property and associated TestData
from 'master' to handle future testing on those platforms.
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.
Good points. I'll address 1. As for 2, the reason is that we are running the tests on Helix and we use the test payload that was built on Windows even if we run the tests on non-Windows. @HaoK do we have infrastructure to build test payloads on non-Windows OS's. I don't remember having anything like that so maybe we should be running the tests on AzDO until we can do that?
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.
We don't have infrastructure to build on non windows yet, so maybe this test should have SkipTests explicitly set to false so it runs on azdo as well, since the behavior is different.
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.
These seem like a good candidate for tests that we just never run on helix too, if they are testing installer/platform specific things?
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'll make sure the tests run when I merge this forward to master.
Tested locally to ensure the tests would pass when run. Made a few changes that warrant another look. |
src/Framework/test/SharedFxTests.cs
Outdated
|
||
if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) | ||
{ | ||
listedSharedFxAssemblies.Remove("aspnetcorev2_inprocess"); |
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.
This is testing the shared framework and we ship that for every platform.
- Should not muck with the
ListedSharedFxAssemblies
value in a single test. This means other tests may fail depending on test execution order. TheTestData
value itself should conditionally exclude the native assembly. - It's not clear how this test is passing on non-Windows platforms in 'master'. Please figure that out.
Separately, this won't work unless you also exclude the ANCM assembly on Windows ARM and ARM64. Probably better to copy the $(VerifyAncmBinary)
property and associated TestData
from 'master' to handle future testing on those platforms.
🆙📅 |
@wtgodbe can go into release/3.1 right now? I just want to double check it wouldn't interfere with any of your 3.1 work. |
@JunTaoLuo please wait for next month's release, the build for 3.1.6 is already pretty much done |
Addresses #19151 and #14832. Backport of #23045