Skip to content

Move Blazor Server template tests to new project #21345

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

Merged
merged 1 commit into from
May 1, 2020

Conversation

captainsafia
Copy link
Member

@captainsafia captainsafia commented Apr 29, 2020

Changes in this PR

  • Creates new BlazorTemplates.Test project with Blazor Server tests
  • Removes skip test on Helix from ProjectTemplate config
  • Move E2E helpers to shared asets

Everything in the new test project is copied from the original, the key differences are in:

  • src/ProjectTemplates/test/ProjectTemplates.Tests.csproj
  • src/ProjectTemplates/test/BlazorServerTemplateTest.cs → src/ProjectTemplates/BlazorTemplates.Tests/BlazorServerTemplateTest.cs
  • src/ProjectTemplates/BlazorTemplates.Tests/BlazorTemplates.Tests.csproj

Addresses #21293

@ghost ghost added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Apr 29, 2020
@HaoK
Copy link
Member

HaoK commented Apr 29, 2020

Maybe want to consider a comment/note that mentions how we have a copy of the utility files between BlazorTemplates/ProjectTemplates, or consider just moving them to shared sources, otherwise there's a consistency issue if we update one set of files and not the other.

@captainsafia captainsafia force-pushed the safia/move-template-tests branch 2 times, most recently from 323f05b to c20ad8c Compare April 29, 2020 23:13
@captainsafia
Copy link
Member Author

captainsafia commented Apr 29, 2020

Maybe want to consider a comment/note that mentions how we have a copy of the utility files between BlazorTemplates/ProjectTemplates, or consider just moving them to shared sources, otherwise there's a consistency issue if we update one set of files and not the other.

I moved these into Shared sources.

@captainsafia captainsafia force-pushed the safia/move-template-tests branch 2 times, most recently from f40fc41 to 359e7ed Compare April 30, 2020 00:47
@captainsafia captainsafia reopened this Apr 30, 2020
@captainsafia captainsafia marked this pull request as ready for review April 30, 2020 16:12
@captainsafia captainsafia added area-blazor Includes: Blazor, Razor Components and removed area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels Apr 30, 2020
@captainsafia captainsafia force-pushed the safia/move-template-tests branch 2 times, most recently from 12e77e3 to 9abbb11 Compare April 30, 2020 19:48
@captainsafia captainsafia requested review from HaoK and a team April 30, 2020 21:34
Copy link
Member

@HaoK HaoK left a comment

Choose a reason for hiding this comment

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

Looks good I think, looks like the project template tests are skipped in the output, only thing that looks a bit sketchy is that it only takes 9 seconds to run the blazor tests? Might be worth double checking that things actually ran

https://dev.azure.com/dnceng/public/_build/results?buildId=625519&view=logs&j=80f594f0-dab9-53e4-691e-a6c036ec4279&t=08be69de-580a-5f94-b421-8e6f5d337215

@captainsafia captainsafia force-pushed the safia/move-template-tests branch from 9abbb11 to 296fcac Compare April 30, 2020 23:15
@captainsafia captainsafia force-pushed the safia/move-template-tests branch 3 times, most recently from 707cf03 to dc3b8d4 Compare May 1, 2020 00:50
@captainsafia captainsafia force-pushed the safia/move-template-tests branch from dc3b8d4 to 374cd29 Compare May 1, 2020 02:54
@@ -0,0 +1,1282 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

A lot of the things in this file don't appear to be specific to Blazor. Should this be trimmed to just the Blazor tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

It can be. I was trying to avoid making too many changes considering the temporary nature of this PR.

@SteveSandersonMS
Copy link
Member

SteveSandersonMS commented May 1, 2020

Thanks for taking this on, @captainsafia!

I'm a little concerned about the amount of stuff in the following files that appears to be generic infrastructure boilerplate:

  • BlazorTemplates.Tests.csproj
  • Infrastructure/GenerateTestProps.targets
  • Infrastructure/TemplateTests.props.in

My concern is that I don't know how we'll maintain this. It doesn't appear specific to Blazor and I don't even know what a lot of it is for (example). Some of that seems to be about EF :)

Having generic infrastructure boilerplate stuff is kind of OK as long as it's really owned by a sufficiently large part of the whole ASP.NET org. However having a separate Blazor-specific copy of it is likely to mean, in practice, that the Blazor team is on the hook for figuring out how to keep it updated to match the rest of the CI infrastructure. I don't know that we can realistically do that.

Do we have agreements and systems to ensure that anyone who updates the original test project will take care of updating BlazorTemplates.Tests too? Unless we do I'd be reluctant to even have a separate project, as it will mean duplicated effort and more costs to coordinate.

@HaoK
Copy link
Member

HaoK commented May 1, 2020

I think the short/medium term goal is to have selenium working on helix, so we can delete this project and move it back into the project templates. The reason we want to move the blazor server tests out immediately, is we have no way easy way to run a few tests on azdo and a few on helix, and since template tests are pretty flaky, we really want to run them only in one place (helix or azdo not both). You don't need to worry about the blazor team owning this long term. The idea agreed to in DOI this week is for the larger team as a whole to collectively own the templates tests, and this new test project would also fall under that same umbrella...

@SteveSandersonMS
Copy link
Member

Thanks for responding, @HaoK!

If this is broadly agreed to be technical debt, and we have a plan to fix it in the short/medium term, and during that time we're treating the infrastructure in this new project as jointly owned by the whole ASP.NET group, then that does allay my concerns.

I'm still interested in whether some of those large stretches of infrastructure boilerplate (that I don't recognize, like the EF stuff) have ways of being factored out into more shared locations to reduce the opportunities for things diverging by mistake. However I trust you and @captainsafia to make good choices about what is/isn't reasonable for that 😄

@HaoK
Copy link
Member

HaoK commented May 1, 2020

I took a closer look at the shared infrastructure stuff most of that stuff is involved in getting dotnet new/dotnet net ef to work in the template tests, since those require a bunch of configuration so the directories and commands work. That said, this setup is mostly for the azdo side, as helix does its own things via environment and in the testrunner bootstrapping. Unfortunately I'm not that familiar with all of that complexity either, so I think for now, we should just duplicate those rather than investing in teasing that logic apart for this PR. There probably are a bunch of settings that aren't needed by the blazor tests, but Safia would have basically have to do several iterations of nuking all the settings and adding them back piece by piece, which I don't think is worth it

@captainsafia
Copy link
Member Author

captainsafia commented May 1, 2020

Ditto @HaoK's point. This is a temporary stopgap until we get Selenium working on Helix. Once that is done, we can refactor our test setup on the assumption that we are only targeting one environment. Hopefully, we'll be able to get Selenium working on Helix before the blazor-wasm branch is merged into master so that our Selenium-based tests can run alongside everything there.

Looks good I think, looks like the project template tests are skipped in the output, only thing that looks a bit sketchy is that it only takes 9 seconds to run the blazor tests?

Yes, it looks like the test project is detected by the actual the cases are not running, I will take a look into this.

This is actually running. I had the wrong filter in the AzDO UI.

@captainsafia
Copy link
Member Author

I'll merge this for now and focus on getting #21280 squared away so that we have a consistent environment to start making improvements on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants