Framework.Tests: support running against Linux build artifacts. #46600
Conversation
ghost
added
area-infrastructure
|
Thanks for your PR, @tmds. Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
| // System.Diagnostics.EventLog.Messages is only present in the Windows build. | ||
| if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) | ||
| { | ||
| ListedSharedFxAssemblies.Remove("System.Diagnostics.EventLog.Messages"); |
There was a problem hiding this comment.
This won't work on Helix since we always run tests against the windows assets there (we don't intend to change that) - you could add !SkipOnHelixAttribute.OnHelix() to this condition to account for that
There was a problem hiding this comment.
I'll make the change.
I'm curious, is there a reason you only run these tests against the Windows assets?
There was a problem hiding this comment.
We try to run every test we can on Helix because it saves resources, and helix works by building assets once & sending them off to multiple test queues. So the answer is essentially "because we can" - but, I might be swayed by an argument that these tests should not be run on Helix because they're testing if the assets were produced correctly on the target OS, not that they run correctly there. @dougbu what do you think?
There was a problem hiding this comment.
My point is that these tests check "Was the sharedFx built correctly?". Since we run them in helix, we're only testing "Was the sharedFx built on windows built correctly?" - we don't run this test against a SharedFx built on Linux or Mac (in CI).
There was a problem hiding this comment.
My point is that these tests check "Was the sharedFx built correctly?". Since we run them in helix, we're only testing "Was the sharedFx built on windows built correctly?" - we don't run this test against a SharedFx built on Linux or Mac (in CI).
One way to add such testing would be to move the tests off Helix. Unfortunately, that goes against our preference to test everything we can in Helix and avoid duplicate testing.
This also pretty normal because all of our assemblies are built on Windows and tested on the Helix agents. The main unusual thing is that we actually ship shared Fx packages built on non-Windows agents.
Until and unless we centralize crossgen2 execution to the Windows build, let's ignore this. Thoughts❔
There was a problem hiding this comment.
The condition now looks like it'll work fine when run on Helix agents. Am I missing something❔
The tests failed on non-windows on helix in the first iteration because of this test, because of this condition (they ran on non-windows, but tested against a SharedFx built on windows)
Are we 🆗 from your perspective here now❔
There was a problem hiding this comment.
One way to add such testing would be to move the tests off Helix. Unfortunately, that goes against our preference to test everything we can in Helix and avoid duplicate testing. This also pretty normal because all of our assemblies are built on Windows and tested on the Helix agents. The main unusual thing is that we actually ship shared Fx packages built on non-Windows agents.
There are some exceptions to that rule though, and I think this should be one of them - these tests are different from others in that they're not testing "do these bits run on this platform", they're testing "Did we produce the correct set of bits for this platform". As it is today, there's no point in running this test on the Windows, Linux, and MacOS Helix queues - all three are testing "Did we build the correct set of bits for windows", and none are testing "Did we build the correct set of bits for Mac/Linux"
There was a problem hiding this comment.
There are some exceptions to that rule though, and I think this should be one of them
The exceptions are all bugs and I would prefer we don't pile on more technical debt. But I also don't disagree our tests of the shared Fx packages is a touch of tunnel vision.
My thought was also about urgency because we haven't hit significant platform-specific problems w/ the shared Fx packages (ignoring those that show up when building in the CI) more than a couple of times (if that). I also plan to try centralizing crossgen2 sooner rather than later.
There was a problem hiding this comment.
I don't think we need to block this PR on it - I opened #46620 to track fixing it in the future.
What changes do you mean? |
| @(ExternalAspNetCoreAppReference); | ||
| @(_TransitiveExternalAspNetCoreAppReference); | ||
| System.Diagnostics.EventLog.Messages" /> | ||
| <_ExpectedSharedFrameworkBinaries Condition="'$(TargetOsName)' == 'win'" Include="System.Diagnostics.EventLog.Messages" /> |
There was a problem hiding this comment.
Is there some Helix check needed here too?
There was a problem hiding this comment.
No, the project will be built only in test jobs (on build agents) or locally. We just upload the test assemblies to Helix agents.
I was looking for the feedback you are providing. |
| @(ExternalAspNetCoreAppReference); | ||
| @(_TransitiveExternalAspNetCoreAppReference); | ||
| System.Diagnostics.EventLog.Messages" /> | ||
| <_ExpectedSharedFrameworkBinaries Condition="'$(TargetOsName)' == 'win'" Include="System.Diagnostics.EventLog.Messages" /> |
There was a problem hiding this comment.
No, the project will be built only in test jobs (on build agents) or locally. We just upload the test assemblies to Helix agents.
| // System.Diagnostics.EventLog.Messages is only present in the Windows build. | ||
| if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) | ||
| { | ||
| ListedSharedFxAssemblies.Remove("System.Diagnostics.EventLog.Messages"); |
|
CI is happy. @dougbu can you merge this please? |
Fixes #46524.
@dougbu ptal.
These are the changes that make the tests pass for a local build.
Can we make it so that the aspnetcore CI when running on X will consume the binaries built for X?
Or will CI keep on running these tests against Windows build artifacts only?
In what files should I make the changes for
HELIX_WORKITEM_ROOT?cc @omajid