Skip to content

Logging tweaks for playwright #30703

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

Closed
wants to merge 30 commits into from
Closed

Logging tweaks for playwright #30703

wants to merge 30 commits into from

Conversation

HaoK
Copy link
Member

@HaoK HaoK commented Mar 5, 2021

No description provided.

@ghost ghost added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Mar 5, 2021
@HaoK
Copy link
Member Author

HaoK commented Mar 8, 2021

@dougbu hrm looks like something weird is going on where we are running blazor templates twice now (once in Windows Template job) and again in helix, I thought BuildHelixPayload=true was supposed to automatically SkipTests, did some refactoring change the expectations there? I only noticed it while investigating logs, but figure we should avoid running these tests twice

@dougbu
Copy link
Contributor

dougbu commented Mar 8, 2021

we are running blazor templates twice now (once in Windows Template job) and again in helix

This is happening because the "Test Templates" step in the "Test: Templates - Windows Server 2016 x64" job doesn't specify /p:SkipHelixReadyTests=true. That wasn't relevant before but is now.

@HaoK
Copy link
Member Author

HaoK commented Mar 8, 2021

Actually do we actually need the test templates job at all now that blazor templates are on helix, this was the last bucket of tests in project templates I believe, so maybe I can just get rid of that job entirely?

@HaoK
Copy link
Member Author

HaoK commented Mar 8, 2021

We had split the blazor templates out from project template originally so they could continue running on azdo while moving the rest of them onto helix, so I don't think we need it any longer

@dougbu
Copy link
Contributor

dougbu commented Mar 8, 2021

@HaoK we have a lot of individual project template tests that are skipped on at least some Helix queues. Need the templates test job to cover them. One reason is the cert / credential issues we've had in Helix testing.

@HaoK
Copy link
Member Author

HaoK commented Mar 8, 2021

@dougbu so its intentional that we want to run them in both helix and in test templates job?

@dougbu
Copy link
Contributor

dougbu commented Mar 8, 2021

so its intentional that we want to run them in both helix and in test templates job?

No, as I said, the build step needs /p:SkipHelixReadyTests=true. My pushback was just on removing the job entirely.

@HaoK
Copy link
Member Author

HaoK commented Mar 8, 2021

I don't see how we'd run any tests in this job if we turn on SKipHelixReadyTests, this would stop running both ProjectTemplates and BlazorTemplates in the test templates job so we'd have a no-op azdo job no? What tests would still run in this job if I set this flag?

From Directory.Build.targets:

    <SkipTests Condition="'$(SkipHelixReadyTests)' == 'true' AND '$(BuildHelixPayload)' == 'true'">true</SkipTests>

@HaoK
Copy link
Member Author

HaoK commented Mar 8, 2021

To clarify, today we run Project templates in both helix and azdo, if I set this flag we'd only run the tests in helix... So your statement that we want azdo coverage relies on this 'bug'/feature where we run on both jobs

@dougbu
Copy link
Contributor

dougbu commented Mar 8, 2021

I don't see how we'd run any tests in this job if we turn on SKipHelixReadyTests

You're right. $(BuildHelixPayload) and $(SkipHelixReadyTests) are too coarse-grained.

We probably already have a fair number of tests in other Helix-ready projects that never execute because they use [SkipOnHelix(...)]. That's a bug and I think I mentioned it at some point. One solution would be to add a new property that allows us to run only [SkipOnHelix(...)] tests on the build agents.

Another (ugly) option would be to continue not running those tests at all. Might as well remove them or treat them as manual-only tests☹️

@HaoK
Copy link
Member Author

HaoK commented Mar 8, 2021

Right well SkipOnHelix effectively is just a SkipTest except for Project Templates and a few handful of other tests projects, I'm not trying to resolve that as part of this change, but I was just asking if we want to continue running templates tests on both platforms. Which I think just translates down to whether we want to keep this job around or not now

@dougbu
Copy link
Contributor

dougbu commented Mar 8, 2021

For now, let's keep the duplication. But please file an issue about removing the dupes and about the tests e.g. Microsoft.AspNetCore.Mvc.FunctionalTests.HtmlGenerationTest.HtmlGenerationWebSite_GeneratesExpectedResultsNotReadyForHelix(...) that (I'm pretty sure) never run.

@HaoK
Copy link
Member Author

HaoK commented Mar 8, 2021

Ok I'll file an issue for the duplication in general, there's already an issue for those specific tests since I did that work for those projects: #29597

@dougbu dougbu mentioned this pull request Mar 8, 2021
5 tasks
@HaoK
Copy link
Member Author

HaoK commented Mar 16, 2021

Rolling logging changes into #30907

@HaoK HaoK closed this Mar 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants