Skip to content

[Blazor] Fix template tests #54606

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 35 commits into from
Jul 9, 2024
Merged

[Blazor] Fix template tests #54606

merged 35 commits into from
Jul 9, 2024

Conversation

MackinnonBuck
Copy link
Member

Opening initially as a draft to resolve CI issues

@ghost ghost added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Mar 18, 2024
@MackinnonBuck MackinnonBuck added area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework and removed area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels Mar 18, 2024
@MackinnonBuck
Copy link
Member Author

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@MackinnonBuck
Copy link
Member Author

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@MackinnonBuck MackinnonBuck force-pushed the mbuck/fix-template-tests branch from 5085388 to af4c2dd Compare March 21, 2024 22:46
@MackinnonBuck
Copy link
Member Author

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@MackinnonBuck MackinnonBuck force-pushed the mbuck/fix-template-tests branch from b0ab194 to 70ea4d6 Compare March 25, 2024 20:43
@build-analysis build-analysis bot mentioned this pull request Mar 26, 2024
Open
3 tasks
@MackinnonBuck MackinnonBuck force-pushed the mbuck/fix-template-tests branch 2 times, most recently from 7c14145 to 408c832 Compare June 20, 2024 01:12
@build-analysis build-analysis bot mentioned this pull request Jun 24, 2024
Open
3 tasks
@MackinnonBuck
Copy link
Member Author

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@build-analysis build-analysis bot mentioned this pull request Jun 25, 2024
Open
3 tasks
MackinnonBuck
MackinnonBuck commented Jun 25, 2024
View reviewed changes
eng/Npm.Workspace.nodeproj
@@ -44,7 +44,6 @@
<Message Text="Building NPM packages..." Importance="high" />

<Exec
Condition="$(ContinuousIntegrationBuild) == 'true'"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change was required in order to build locally with the -pack option. But unfortunately it seems to increase the build time quite a bit. Might be worth considering somehow moving this step to the "Pack" target.

src/ProjectTemplates/TestInfrastructure/PrepareForTest.targets Outdated
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes in this file make it so that we use an alternate .NET host when running template projects. We copy select contents of the .dotnet folder and then overlay the just-built shared framework on top of it. We also add an AssemblyAttribute that can get picked up by the DotNetMuxer so it knows to use the alternate host to run the template projects.

I decided to put this logic in PrepareForTest.targets because this requirement is pretty specific to testing project templates, but I'm happy to consider putting it somewhere else if there's a compelling reason to do so.

@MackinnonBuck MackinnonBuck marked this pull request as ready for review June 25, 2024 22:10
@MackinnonBuck MackinnonBuck requested review from a team and wtgodbe as code owners June 25, 2024 22:10
javiercn
javiercn reviewed Jul 1, 2024
View reviewed changes
Copy link
Member

@javiercn javiercn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good.

It would be great if we can document how the tests are expected to work and what setup is needed as well as the workflow for developers to run things locally.

Specifically all the changes to Directory.Build.targets.in, and similar files.

javiercn
javiercn approved these changes Jul 3, 2024
View reviewed changes
Copy link
Member

@javiercn javiercn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, as we've discussed the remaining things offline, giving you the sign-off now and letting you address those as you see fit.

@MackinnonBuck MackinnonBuck force-pushed the mbuck/fix-template-tests branch from ae01e4e to bb41807 Compare July 5, 2024 23:57
@MackinnonBuck
Revert last two commits
02d1354
@MackinnonBuck
Try undoing build ordering changes
304f577 
These shouldn't have an impact for now since we're only running in
Helix.
@MackinnonBuck
Add comments describing changes
185862c
@MackinnonBuck MackinnonBuck merged commit 8ae09e8 into main Jul 9, 2024
26 checks passed
@MackinnonBuck MackinnonBuck deleted the mbuck/fix-template-tests branch July 9, 2024 16:05
@dotnet-policy-service dotnet-policy-service bot added this to the 9.0-preview7 milestone Jul 9, 2024
@MackinnonBuck MackinnonBuck mentioned this pull request Jul 9, 2024
Closed
4 tasks
@MackinnonBuck MackinnonBuck mentioned this pull request Aug 15, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@javiercn javiercn javiercn approved these changes

@wtgodbe wtgodbe Awaiting requested review from wtgodbe wtgodbe is a code owner

Assignees
No one assigned
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework
Projects
None yet
Milestone
9.0-preview7
Development

Successfully merging this pull request may close these issues.
2 participants
@MackinnonBuck @javiercn