Skip consistently failing Components.E2ETests.ServerTransportsTests

[halter73]

I filed #35481 to unskip these.

[SteveSandersonMS]

Rather than skipping these unconditionally, can they be marked to skip on CI only, or just quarantined?

The reason is that they pass locally, and represent real functionality we don’t want to regress.

While CI is unable to run the tests reliably, the tests themselves work well locally, so for now running locally is the best we’ve got. Skipping unconditionally would prevent that too.

[BrennanConroy]

Moved to quarantine

[SteveSandersonMS]

Thanks

[BrennanConroy]

Huh, actually we have a [SkipOnCI] attribute, if the point is to get the check green then we should use that instead

[HaoK]

If the goal is just to get the CI check green, maybe we should just change the pipeline for components e2e tests to always return success regardless of what the test results are, similar to quarantined-pr?

[SteveSandersonMS]

If the goal is just to get the CI check green, maybe we should just change the pipeline for components e2e tests to always return success regardless of what the test results are, similar to quarantined-pr?

Is there any way to make the overall result simply ignore the outcome from one particular job, without faking the result of that job?

[BrennanConroy]

Is there any way to make the overall result simply ignore the outcome from one particular job, without faking the result of that job?

That's what we're doing today, the components leg isn't required so we ignore it. But it still will show red which we want to avoid.

[SteveSandersonMS]

I mean ignore it for the purposes of deciding on the red/green label.

[BrennanConroy]

Isn't that what Hao was suggesting:

maybe we should just change the pipeline for components e2e tests to always return success regardless of what the test results are, similar to quarantined-pr?

[SteveSandersonMS]

No, because then the test job will show as green when it isn’t.

Perhaps it’s a minor distinction, but if we show it as green regardless, I wonder what benefit there is in running it at all. People may be misled into thinking it proves they didn’t break anything even if they have.

I was asking if we can report the result of this job accurately, but without it affecting the overall red/green.

[HaoK]

Well today functionally we have what we want, and its correct, meaning it shows red because a pr check failed, and components being red does not block the PR (because its not a required check). It sounds like we want the whole PR check to show green ignoring the fact that there are failures, which isn't possible I don't think. So if we want a green checkbox for the PR, we need to force the components check to be green. The agreement I thought was that the blazor team would be manually looking at this pipeline (so it still has value even if its always green). That said, its mostly a psychological/cosmetic difference if it still can be merged right?

[SteveSandersonMS]

Sure, if it can’t be modelled in such a way that one job is ignored when computing the aggregate status then yes, faking it to be green would be an OK compromise until we can get it to be realistic.

[BrennanConroy]

We could also remove the quarantined test run

aspnetcore/.azure/pipelines/components-e2e-tests.yml

Line 45 in 8f871c4

/p:RunQuarantinedTests=true

, our other pipelines don't run quarantined tests on PR checks. This would mean only known good tests would run and if they failed, then you know you either caused a test failure, or they are flaky and should get quarantined.

[halter73]

I was hoping to the the dotnet.aspnetcore-components check green with this PR, but there are still 23 test failures which all seem to have the same problem initializing. For some reason, the "Tests" tab on AzDO shows no failures, but if you fish out Microsoft.AspNetCore.Components.E2ETests_net6.0_x64.html from the artifacts you see a bunch of failures related to /subdir returning a 404 response.

OpenQA.Selenium.BrowserAssertFailedException : Xunit.Sdk.NotEmptyException: Assert.NotEmpty() Failure\r\n
   at Xunit.Assert.NotEmpty(IEnumerable collection) in C:\\Dev\\xunit\\xunit\\src\\xunit.assert\\Asserts\\CollectionAsserts.cs:line 331\r\n
   at Microsoft.AspNetCore.E2ETesting.WaitAssert.<>c__DisplayClass15_0.<Exists>b__0() in /_/src/Shared/E2ETesting/WaitAssert.cs:line 72\r\n
   at Microsoft.AspNetCore.E2ETesting.WaitAssert.<>c__DisplayClass18_0`1.<WaitAssertCore>b__0(IWebDriver _) in /_/src/Shared/E2ETesting/WaitAssert.cs:line 109\r\n
Screen shot captured at 'D:\\workspace\\_work\\1\\s\\artifacts\\TestResults\\Release\\Quarantined\\Microsoft.AspNetCore.Components.E2ETests\\73cbc1d1746b41948ffc410cbe4561dd.png'\r\n
Encountered browser errors\r\n
[2021-08-19T18:30:25Z] [Severe]
http://127.0.0.1:2415/subdir - Failed to load resource: the server responded with a status of 404 (Not Found)Page content:\r\n
<head></head><body></body>\r\n
\r\n
---- Assert.NotEmpty() Failure
   at Microsoft.AspNetCore.E2ETesting.WaitAssert.WaitAssertCore[TResult](IWebDriver driver, Func`1 assertion, TimeSpan timeout) in /_/src/Shared/E2ETesting/WaitAssert.cs:line 129
   at Microsoft.AspNetCore.E2ETesting.WaitAssert.Exists(IWebDriver driver, By finder, TimeSpan timeout) in /_/src/Shared/E2ETesting/WaitAssert.cs:line 67
   at Microsoft.AspNetCore.E2ETesting.WaitAssert.Exists(IWebDriver driver, By finder) in /_/src/Shared/E2ETesting/WaitAssert.cs:line 54
   at Microsoft.AspNetCore.Components.E2ETest.BasicTestAppWebDriverExtensions.WaitUntilTestSelectorReady(IWebDriver browser) in /_/src/Components/test/E2ETest/Infrastructure/WebDriverExtensions/BasicTestAppWebDriverExtensions.cs:line 27
   at Microsoft.AspNetCore.Components.E2ETest.BasicTestAppWebDriverExtensions.MountTestComponent[TComponent](IWebDriver browser) in /_/src/Components/test/E2ETest/Infrastructure/WebDriverExtensions/BasicTestAppWebDriverExtensions.cs:line 18
   at Microsoft.AspNetCore.Components.E2ETest.Tests.BinaryHttpClientTest.InitializeAsyncCore() in /_/src/Components/test/E2ETest/Tests/BinaryHttpClientTest.cs:line 43
   at Microsoft.AspNetCore.E2ETesting.BrowserTestBase.InitializeAsync(String isolationContext) in /_/src/Shared/E2ETesting/BrowserTestBase.cs:line 70
----- Inner Stack Trace -----
   at Microsoft.AspNetCore.E2ETesting.WaitAssert.<>c__DisplayClass15_0.<Exists>b__0() in /_/src/Shared/E2ETesting/WaitAssert.cs:line 72
   at Microsoft.AspNetCore.E2ETesting.WaitAssert.<>c__DisplayClass18_0`1.<WaitAssertCore>b__0(IWebDriver _) in /_/src/Shared/E2ETesting/WaitAssert.cs:line 109

And in the last two runs, all the ServerTransportsTests seemed to have passed. I'm not sure what changed, because they seem to always fail when Test failures do get reported to the "Tests" tab on AzDO.

[BrennanConroy]

all the ServerTransportsTests seemed to have passed

I actually think they haven't run.
Edit: They might be having resource issues, so when run in the quarantined leg there is less contention?

there are still 23 test failures which all seem to have the same problem initializing

These are quarantined tests. Looks like Steve changed the pipeline to run quarantined tests, which also means the non-quarantined tests do not run. I don't know how much of that was intended.

[SteveSandersonMS]

Looks like Steve changed the pipeline to run quarantined tests, which also means the non-quarantined tests do not run

Wow, that is not what I expected. That’s very different to how the other flags work.

@halter73 Would it be ok to flip that flag back off in this PR? I can add the extra commit to do it if you want.

[SteveSandersonMS]

@halter73 Would it be ok to flip that flag back off in this PR? I can add the extra commit to do it if you want.

Well, I just did it. Hope that was OK.

[BrennanConroy]

301 Components test failures, did something break or are there some major infra issues?

Edit: Maybe caused by #35318?

[BrennanConroy]

Ok, I now think it's #35414 that caused the test failures. You can see the last commit before switching to only quarantined tests there were 300 failures https://github.com/dotnet/aspnetcore/runs/3360365134

[SteveSandersonMS]

Looks like this is becoming quite a distraction. Do you think it might be time to just disable the job until I can find a solid solution for CI?

For context, since you're basically involved now, my plan of attack for this whole area has been:

1. Ensure each test (including quarantined) usually passes, to have good confidence we don't have real product regressions in RC1
2. Get the whole test suite to pass every time when run in a controlled environment (an Azure VM)
3. Get the same reliability in CI

I'm at phase 2 now. Last night the test set ran 44 times in my VM, and with the changes in #35505, every test passed every time with the exception of CanFocusDuringOnAfterRenderAsyncWithFocusInEvent which failed twice. I'll find a way to resolve that, and then I think it's fair to say the tests are working well in the right environment.

As for phase 3, I've not begun looking into that yet and haven't attempted to do anything for it specifically. I don't know why #35414 might have made more tests fail in CI (but at least fail more consistently) when it clearly improved matters locally. It's the next phase to look into.

@halter73 @BrennanConroy I'm meant to be off work today, so for now, do you want to disable the dotnet.aspnetcore-components job entirely? Alternatively feel free to just ignore it until Monday, when I'll probably disable that job unless I have some definite breakthrough in understanding the CI situation on that day.

[HaoK]

@SteveSandersonMS I can take a quick look today to see if there's a simple tweak to force the components pipeline to report green, that seems like it'd be better for you as you'd continue to get tons of runs to continue making progress on improving the tests on the ci, and without all the distractions of every PR appearing red

[wtgodbe]

You could just add ContinueOnError to the failing step:

aspnetcore/eng/common/templates/steps/publish-logs.yml

Line 13 in 52eff90

continueOnError: true

[HaoK]

Thanks Will! #35559

[HaoK]

Merged the PR so components should always be green excluding build errors now, also filed #35565 to track undoing this at some point

[halter73]

@HaoK should we close this now that #35559 is merged?