Add support for running helix tests against built shared fx #16828
Conversation
HaoK
added
the
area-infrastructure
github-actions
bot
added
area-mvc
HaoK
changed the title
|
@aspnet/build this looks ready for review now too, this PR now builds the shared fx/runtime and includes them by default for all helix payloads, and installs the shared fx before running the tests. This change shouldn't really change anything (other than improving 'correctness' of testing against coherent bits), but is needed to enable templates tests to be helix-ified |
|
@HaoK should we merge this? |
|
Sure @dotnet/aspnet-build do you want to review? |
| # This is going to actually build x86 native assets. See https://github.com/aspnet/AspNetCore/issues/7196 | ||
| - script: ./build.cmd | ||
| -ci | ||
| -arch x86 |
There was a problem hiding this comment.
Shouldn't this be 'x64' for 64-bit testing?
There was a problem hiding this comment.
I only saw an x86 shared framework in the ci.yml, so I assume it doesn't matter?
There was a problem hiding this comment.
The Windows x64 / x86 build step's comments are a bit confusing. The first ./build.cmd invocation creates the x64 shared runtime. The second (commented) one adds just the x86-specific bits.
I'm surprised the tests succeeded with the x86 shared runtime. Does anything actually test using it?
There was a problem hiding this comment.
Yes this is on by default so all the shared framework bits are getting copied onto the helix machines for every test, so it doesn't seem to be causing any issues.
There was a problem hiding this comment.
My point is the created x86 shared framework here may not be used at all because it's not the bitness the tests need. Not causing issues isn't the same thing as enabling the testing we need.
There was a problem hiding this comment.
Sure what's the fix just specify arch x64?
There was a problem hiding this comment.
I suggest adding a test that won't run unless it executes against the expected shared framework. Maybe something that looks at the location of a loaded SharedFX assembly. That'll help show what's working (or not) here.
Put another way, I'd rather not see this new infrastructure need further changes as soon as we try to use it.
There was a problem hiding this comment.
This is prerequisite work for template testing, I don't think we need to add a new test for that, the only reason we need this is because its a requirement for templates which will fail if they aren't running against latest shared framework bits.
There was a problem hiding this comment.
Rewording what I said: This doesn't look like it should work. Need to show that it is working w/ Win x64 test.
But, sure, one way to do that would be to enable template tests in the same PR.
| # This is going to actually build x86 native assets. See https://github.com/aspnet/AspNetCore/issues/7196 | ||
| - script: ./build.cmd | ||
| -ci | ||
| -arch x86 |
There was a problem hiding this comment.
The Windows x64 / x86 build step's comments are a bit confusing. The first ./build.cmd invocation creates the x64 shared runtime. The second (commented) one adds just the x86-specific bits.
I'm surprised the tests succeeded with the x86 shared runtime. Does anything actually test using it?
|
@dougbu are you good merging this now? |
No but mostly because 'master' has moved on. |
|
@jkotalik when you get a chance can you take a look at the IIS functional failures that have started to show up after copying over the shared framework bits onto the helix machines, any ideas what might cause all the win 10 failures? https://dev.azure.com/dnceng/public/_build/results?buildId=526231&view=ms.vss-test-web.build-test-results-tab&runId=16660578&paneView=asg-cloudtest.asg-cloudtest-attachments.asg-cloudtest.asg-cloudtest-attachments&resultId=109141 |
|
@HaoK I need some context on this change. Is the goal of this change to reduce the number of dlls brought over in each helix payload? My guess is that with this change, somehow the prebuilt IIS sites aren't getting published into the right location, or that dotnet isn't being found for some reason. I'll start investigating once I have more context on this change and the goals of it. |
|
@jkotalik well the idea is to allow any tests to run against the build shared fx bits (templates), maybe we want to enable this by default for all the tests. I wasn't expecting there to be any changes to the IIS tests at this time, so I'm not sure what affected them. The changes in this PR did tweak the build steps to build the shared fx, so maybe that messed up the prebuilt steps, hopefully that gives you a pointer in what might have broken things. |
|
Replacing with #19177 |
amcasey
added
area-networking
No description provided.