-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Test with trimming enabled in CI and in release builds. #30822
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
@@ -32,8 +31,7 @@ protected override string StartAndGetRootUri() | |||
{ | |||
if (_buildWebHostMethod == null) | |||
{ | |||
// Use Blazor's dev host server | |||
var underlying = new DevHostServerFixture<TClientProgram>(); |
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.
Why do we need to remove the devhost server fixture?
We want in general to know that things work well with our devhost server
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.
Is this a rename that it's not picking up correctly?
<_Parameter1>Microsoft.AspNetCore.E2ETesting.TestTrimmedApps</_Parameter1> | ||
<_Parameter2>$(TestTrimmedApps)</_Parameter2> | ||
</AssemblyAttribute> | ||
</ItemGroup> |
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 ought to stop doing this, it's end up adding up and becoming painful. I'm ok with it for now, but we should move to a json file or similar on the folder that is much easier to change, update, etc. without having to deal with assembly attributes
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.
Only minor comments, but looks good otherwise.
@@ -9,6 +9,13 @@ | |||
<GenerateResxSource>false</GenerateResxSource> | |||
</PropertyGroup> | |||
|
|||
<PropertyGroup Condition="'$(TestTrimmedApps)' == 'true'"> | |||
<StaticWebAssetBasePath>/subdir</StaticWebAssetBasePath> |
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.
Why does this only apply to trimmed apps?
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 weirds out our in-place tests if we configure the StaticWebAssetBasePath. It's a result of how we run the test - by specifying a base path to the host. I tried changing it but it seemed like a messy ball to undo.
Btw, I'm holding off on merging this PR because it conflicts with my hot reload work. We're expressly removing a lot of the hot reload APIs during trimming which poses a problem with the approach here.
9b222ec
to
4b379dc
Compare
4b379dc
to
8c1cff1
Compare
8c1cff1
to
b5771ec
Compare
No description provided.