[Blazor][Templates] Replaces selenium with playwright for our E2E Tests #29873

javiercn · 2021-02-03T12:45:53Z

Supports multiple browsers.
Supports multiple OS.
Individual configuration per browser via configuration files.
Separate configuration for CI environments.
Separate configuration per Operating System.
Separate configuration for debugging.
Removes the requirement to have a browser installed.
Removes the requirement to have JAVA installed.
Removes the requirement to have nodejs installed.
Records detailed information on each test run
- Network traces from the browser.
- Video of the test execution in the browser.
- Browser logs.
- Network errors on the browser.
- Page errors on the browser.

Addresses #27610

wtgodbe · 2021-02-04T17:43:18Z

Nice!

src/ProjectTemplates/BlazorTemplates.Tests/BlazorTemplates.Tests.csproj

src/Shared/BrowserTesting/src/BrowserManager.cs

src/Shared/BrowserTesting/src/BrowserManagerConfiguration.cs

src/Shared/BrowserTesting/src/PublicAPI.Unshipped.txt

AspNetCore.sln

src/Shared/BrowserTesting/src/PublicAPI.Shipped.txt

src/ProjectTemplates/BlazorTemplates.Tests/BlazorServerTemplateTest.cs

src/ProjectTemplates/BlazorTemplates.Tests/BlazorWasmTemplateTest.cs

src/ProjectTemplates/BlazorTemplates.Tests/PlaywrightFixture.cs

src/ProjectTemplates/BlazorTemplates.Tests/playwrightSettings.debug.json

src/Shared/BrowserTesting/src/BrowserManagerConfiguration.cs

src/Shared/BrowserTesting/src/ContextInformation.cs

javiercn

Addressed feedback

AspNetCore.sln

src/ProjectTemplates/BlazorTemplates.Tests/BlazorServerTemplateTest.cs

src/ProjectTemplates/BlazorTemplates.Tests/PlaywrightFixture.cs

src/ProjectTemplates/BlazorTemplates.Tests/playwrightSettings.debug.json

src/Shared/BrowserTesting/src/BrowserManagerConfiguration.cs

src/Shared/BrowserTesting/src/PublicAPI.Shipped.txt

src/Shared/BrowserTesting/src/PublicAPI.Unshipped.txt

dougbu

I also find all of the JSON files and related configuration complexity odd. Would like to hear more.

dougbu · 2021-02-05T20:51:51Z

AspNetCore.sln

+Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "BrowserTesting", "BrowserTesting", "{8F33439F-5532-45D6-8A44-20EF9104AA9D}"
+EndProject
+Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Microsoft.AspNetCore.BrowserTesting", "src\Shared\BrowserTesting\src\Microsoft.AspNetCore.BrowserTesting.csproj", "{B739074E-6652-4F5B-B37E-775DC2245FEC}"
+EndProject


😃 looks much better

Minor but FAE04EC0-301F-11D3-BF4B-00C04F79EFBC is the old C# GUID. Should be 9A19103F-16F7-4668-BE54-9A1E7A4F7556

javiercn · 2021-02-09T14:37:25Z

I also find all of the JSON files and related configuration complexity odd. Would like to hear more.

There are three main axis for config:

Local build vs CI
General/OS specific config
Running tests or actively debugging them.

We want to be able to pivot on all three of them. Some samples:

CI is not setup to run tests in a given platform, but we still want to run them locally as part of dev builds.
We want to run test headless when we are not debugging, but we want to see the browser window and the interactions when we are debugging.
We don't want to have a 30 seconds timeout on assertions when we are debugging since it forces you to restart when you stop to debug and investigate something.
We don't want to run tests for a specific browser on a platform in a CI instance.
A browser needs different command-line arguments in a different OS.

The goal of this config system is to enable all those things through config without having to continuously tweak those things in code.

That's how the old system behaved and how we ended up with different config systems/mechanism throughout the codebase in different areas of the product. The purpose of this config system is to have a unified mechanism that can satisfy the needs of different config requirements across different areas of the product without having to resort to custom config mechanisms per area.

Once we start adopting this through other areas (components) we won't have to wonder how these test are configured there or how to change the configuration if it needs to be updated. In addition to that, we will be able to share as many parts as the configuration we want, while keeping the ability to override things if necessary.

The current config system is incomplete as it is, and that's by design, since we plan to complete it as we start adopting this new approach through other areas of the product.

pranavkm

Looks great

pranavkm · 2021-02-09T14:31:08Z

src/Shared/BrowserTesting/src/PageInformation.cs

+            {
+                _logger.LogError(e.FailureText);
+            }
+            catch


Why does logging fail?

I've seen it fail when I wasn't doing proper cleanup of things, and I just want to make sure I'm conservative and don't hide a failure in a different place.

So will this eventually be removed once you resolve the other issues?

src/ProjectTemplates/BlazorTemplates.Tests/BlazorServerTemplateTest.cs

pranavkm · 2021-02-09T15:07:11Z

src/ProjectTemplates/BlazorTemplates.Tests/BlazorServerTemplateTest.cs

-            Browser.Click(By.PartialLinkText("Counter"));
-            Browser.Contains("counter", () => Browser.Url);
-            Browser.Equal("Counter", () => Browser.FindElement(By.TagName("h1")).Text);
+            await Task.WhenAll(


Doesn't this change the expectation from before? For e.g. if the initial page rendered a Counter (because we have a bug in our template or framework), this would now succeed whereas it would not have previously?

Also, it's not clear what the concurrent execution is meant to solve? In the ordinary case, these should generally evaluate serially just because that's when items would be ready no?

No, the await page.WaitForSelectorAsync("h1 >> text=Hello, world!"); above makes sure that's not the case.

In this case it can be done sequentially, but by doing it this way has the benefit that we make sure that we don't miss any change if it happens fast after the interaction.

any change if it happens fast after the interaction.

Can you elaborate on this? Do we have a concurrency issue with our tests this is trying to fix?

Perhaps there are certain kinds of changes that can be missed if they happen too soon (for example, a state that exists only briefly), but it doesn't seem like it applies to the assertions we're doing here, since the assertions are checking that the document reaches a desired state before we mutate the document further.

In general people think about test scripts as a sequence of actions and observations performed serially. It will be easier to reason about if we stick closer to that pattern rather than having higher-level constructs like parallelism.

src/ProjectTemplates/BlazorTemplates.Tests/PlaywrightFixture.cs

src/Shared/BrowserTesting/src/BrowserManager.cs

src/Shared/BrowserTesting/src/Directory.Build.props

dougbu

Still struck by the complications in this PR but 🆗

Please fix #29873 (comment)

dougbu · 2021-02-09T19:26:30Z

AspNetCore.sln

+Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "BrowserTesting", "BrowserTesting", "{8F33439F-5532-45D6-8A44-20EF9104AA9D}"
+EndProject
+Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Microsoft.AspNetCore.BrowserTesting", "src\Shared\BrowserTesting\src\Microsoft.AspNetCore.BrowserTesting.csproj", "{B739074E-6652-4F5B-B37E-775DC2245FEC}"
+EndProject


Minor but FAE04EC0-301F-11D3-BF4B-00C04F79EFBC is the old C# GUID. Should be 9A19103F-16F7-4668-BE54-9A1E7A4F7556

javiercn · 2021-02-09T19:40:53Z

Minor but FAE04EC0-301F-11D3-BF4B-00C04F79EFBC is the old C# GUID. Should be 9A19103F-16F7-4668-BE54-9A1E7A4F7556

I feel like we are fighting the tooling. I added the project with dotnet sln add, I don't think we should be editing the solution file manually. If there are no drawbacks/functional differences I propose we don't change anything or that we file an issue in the SDK to make sure the new guid is used but avoid manually editing files

dougbu · 2021-02-09T19:48:04Z

I feel like we are fighting the tooling

Not really. Visual Studio will update the sln in some random PR as soon as the next person edits the file.

The problem is weird issues in dotnet sln add.

captainsafia

Good stuff! Left some questions/comments inline.

eng/Versions.props

src/ProjectTemplates/BlazorTemplates.Tests/BlazorServerTemplateTest.cs

src/ProjectTemplates/BlazorTemplates.Tests/playwrightSettings.ci.win.json

src/ProjectTemplates/Shared/AspNetProcess.cs

src/Shared/BrowserTesting/src/BrowserKind.cs

src/Shared/E2ETesting/E2ETesting.props

* Supports multiple browsers. * Supports multiple OS. * Individual configuration per browser via configuration files. * Separate configuration for CI environments. * Separate configuration per Operating System. * Separate configuration for debugging. * Removes the requirement to have a browser installed. * Removes the requirement to have JAVA installed. * Removes the requirement to have nodejs installed. * Records detailed information on each test run * Network traces from the browser. * Video of the test execution in the browser. * Browser logs. * Network errors on the browser. * Page errors on the browser.

javiercn · 2021-02-10T20:47:14Z

/AzurePipelines run AspNetCore-ci

azure-pipelines · 2021-02-10T20:47:24Z

Azure Pipelines successfully started running 1 pipeline(s).

BrennanConroy · 2021-02-10T22:42:33Z

src/ProjectTemplates/BlazorTemplates.Tests/BlazorServerTemplateTest.cs


+            // Wait until SignalR sends a ping and we answer


lol, I'm seeing the socket getting disconnected and I suspect is because we click a link before blazor is "fully" ready.

I'm not able to repro it locally (if you see, I'm running the automation 100 times in a loop) so this is my attempt to wait for "things to settle" before we start the test.

For reference, some of the logs I see

dbug: Microsoft.AspNetCore.Components.Server.Circuits.CircuitHost[206] The JS interop call with callback id '2' succeeded. info: [Browser][https://localhost:52046/] [2021-02-10T21:20:50.897Z] Information: Normalizing '_blazor' to 'https://localhost:52046/_blazor'. (https://localhost:52046/_framework/blazor.server.js:0:5693) error: [Browser]net::ERR_CONNECTION_REFUSED error: [Browser][https://localhost:52046/] Failed to load resource: net::ERR_CONNECTION_REFUSED (https://localhost:52046/_blazor/negotiate?negotiateVersion=1:0:0) warn: [Browser][https://localhost:52046/] [2021-02-10T21:20:53.219Z] Warning: Error from HTTP request. TypeError: Failed to fetch. (https://localhost:52046/_framework/blazor.server.js:0:5593) error: [Browser][https://localhost:52046/] [2021-02-10T21:20:53.219Z] Error: Failed to complete negotiation with the server: TypeError: Failed to fetch (https://localhost:52046/_framework/blazor.server.js:0:5496) error: [Browser][https://localhost:52046/] [2021-02-10T21:20:53.219Z] Error: Failed to start the connection: TypeError: Failed to fetch (https://localhost:52046/_framework/blazor.server.js:0:5496) error: [Browser][https://localhost:52046/] [2021-02-10T21:20:53.220Z] Error: TypeError: Failed to fetch (https://localhost:52046/_framework/blazor.server.js:18:24519) error: [Browser][https://localhost:52046/] [2021-02-10T21:20:53.220Z] Error: Error: Cannot send data if the connection is not in the 'Connected' State. (https://localhost:52046/_framework/blazor.server.js:18:24519) info: [Browser][https://localhost:52046/] [2021-02-10T21:21:13.222Z] Information: Normalizing '_blazor' to 'https://localhost:52046/_blazor'. (https://localhost:52046/_framework/blazor.server.js:0:5693) error: [Browser]net::ERR_CONNECTION_REFUSED error: [Browser][https://localhost:52046/] Failed to load resource: net::ERR_CONNECTION_REFUSED (https://localhost:52046/_blazor/negotiate?negotiateVersion=1:0:0) warn: [Browser][https://localhost:52046/] [2021-02-10T21:21:15.587Z] Warning: Error from HTTP request. TypeError: Failed to fetch. (https://localhost:52046/_framework/blazor.server.js:0:5593) error: [Browser][https://localhost:52046/] [2021-02-10T21:21:15.587Z] Error: Failed to complete negotiation with the server: TypeError: Failed to fetch (https://localhost:52046/_framework/blazor.server.js:0:5496) error: [Browser][https://localhost:52046/] [2021-02-10T21:21:15.587Z] Error: Failed to start the connection: TypeError: Failed to fetch (https://localhost:52046/_framework/blazor.server.js:0:5496) error: [Browser][https://localhost:52046/] [2021-02-10T21:21:15.587Z] Error: TypeError: Failed to fetch (https://localhost:52046/_framework/blazor.server.js:18:24519) error: [Browser][https://localhost:52046/] [2021-02-10T21:21:15.588Z] Error: Error: Cannot send data if the connection is not in the 'Connected' State. (https://localhost:52046/_framework/blazor.server.js:18:24519) error: [Browser][https://localhost:52309/] [2021-02-10T21:21:20.356Z] Error: Connection disconnected with error 'Error: WebSocket closed with status code: 1006 ().'. (https://localhost:52309/_framework/blazor.server.js:0:5496)

Looks like these logs are failing on negotiate which is before the websocket

@BrennanConroy Yes, but I think that happens because we click on the counter button too fast and that causes a full page navigation to happen instead of a client-side navigation

If you see the first line, the socket is already connected on the initial page

I don't have access to the network trace on the PR but I've seen a similar issue when I was running locally and wasn't waiting long enough

Is it possible we need a more explicit signal that the Blazor Server client-side code is wired up and ready to go?

Reacting to the WebSockets traffic is an indicator that things have started happening, but I'm not sure it guarantees that blazor.server.js has got as far as activating the link click interception. There might still be some time gap where it can go wrong.

We are a bit limited in what we can do when testing the templates, as we can't add any test-specific logic to the application being tested. If we're keen to solve this, one option would be something like getting PlayWright to wait until the DOM contains some artifacts that are specific to interactive rendering, such as the comment marker tags we put around component outputs, or even explicitly looking for the delegating event handler registered on the root element.

If you think this is sufficiently unlikely to make a difference then I'm fine with leaving it until we see reliability issues. Might be good to clarify the comment in the test sources though to at least state what kind of test reliability issue this logic is meant to mitigate.

@SteveSandersonMS at first I thought the reliability issues we were having were because we were not waiting enough, but after doing all this, what I'm seeing is that the connection to the server is lost in some cases.

I have not been able to reproduce this locally (I executed the whole E2E interaction 200 times in a loop several times with 100% success).

I think the amount of waiting that we do here is correct (and arguably we don't need to wait for the SignalR ping).

The reason for is:

When the circuit starts (first frame sent) an initial render happens.

That initial render renders the router which in turn triggers the interop wire up.

The browser always receives the render batch before the interop wire up and always acks the first batch.

Then the browser receives the interop call and acks the js interop invocation.

At the point where we receive the second js interop invocation we are ready to go.

I think overall, waiting for frames is much more reliable than checking other secondary DOM or JS state that we can't clearly evaluate.

If we wanted to go that far, we could even crack open the payloads of the websockets to validate they were exactly what we were expecting.

src/ProjectTemplates/BlazorTemplates.Tests/BlazorServerTemplateTest.cs

Pilchie added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Feb 3, 2021

javiercn marked this pull request as ready for review February 4, 2021 17:29

javiercn requested review from dougbu and a team as code owners February 4, 2021 17:29

javiercn requested review from HaoK and a team February 4, 2021 17:29

wtgodbe reviewed Feb 4, 2021

View reviewed changes

src/ProjectTemplates/BlazorTemplates.Tests/BlazorTemplates.Tests.csproj Show resolved Hide resolved

wtgodbe approved these changes Feb 4, 2021

View reviewed changes

HaoK reviewed Feb 4, 2021

View reviewed changes

src/Shared/BrowserTesting/src/BrowserManager.cs Show resolved Hide resolved

HaoK reviewed Feb 4, 2021

View reviewed changes

src/Shared/BrowserTesting/src/BrowserManagerConfiguration.cs Show resolved Hide resolved

HaoK reviewed Feb 4, 2021

View reviewed changes

src/Shared/BrowserTesting/src/PublicAPI.Unshipped.txt Outdated Show resolved Hide resolved

dougbu suggested changes Feb 4, 2021

View reviewed changes

AspNetCore.sln Outdated Show resolved Hide resolved

src/Shared/BrowserTesting/src/PublicAPI.Shipped.txt Outdated Show resolved Hide resolved