Add sample dashboard playwright test#4386
Conversation
ghost
added
the
|
related to #958 |
adamint
changed the title
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Could I get a review on this? |
…run-oop # Conflicts: # src/Aspire.Dashboard/DashboardWebApplication.cs
|
Build failing with: |
Sorry, @radical. That property name was seemingly changed and I did not notice after merge. It builds now 😄 |
|
|
||
| public async Task InitializeAsync() | ||
| { | ||
| var installExitCode = Microsoft.Playwright.Program.Main(new[] {"install"}); |
There was a problem hiding this comment.
This will run once for every class using this fixture.
EDIT: doing this once would be better.
| throw new PlaywrightException($"Playwright exited with code {installExitCode}"); | ||
| } | ||
|
|
||
| var installDepsExitCode = Microsoft.Playwright.Program.Main(new[] {"install-deps"}); |
There was a problem hiding this comment.
On the linux (ubuntu) build machine install-deps will install a whole bunch of packages with apt-get. Because this is a shared machine, we want to avoid installing packages on it.
The existing infrastructure for installing playwright available is partially broken right now due to package installation issues (#4623). And these tests are likely to run into the same issue randomly.
So, I would suggest not running the Playwright based tests on linux for now. You can add this to your test - [ActiveIssue("https://github.com/dotnet/aspire/issues/4623", TestPlatforms.Linux)].
Going forward:
- [tests] Extract playwright bits to Playwright.targets, and separate class #4689 - this will make the existing playwright targets reusable
- Once Install some playwright dependencies on Ubuntu.2204.Amd64* helix agents dotnet/dnceng#3122 is resolved, we can drop even this code.
Neither of these need to block your PR though.
There was a problem hiding this comment.
Also, could you please open an issue to track that the code here needs to be adjusted once #4689 is merged, and dotnet/dnceng#3122 is fixed?
There was a problem hiding this comment.
Usually we strongly recommend doing install and --with-deps from the outside. Since these require sudo and modify the system. These should not delay the test execution and skipping it on Linux seems like showing the drawbacks for it.
lets call it from the CI yaml instead? This follows what Playwright users are used to.
dotnet-policy-service
Bot
added
the
needs-author-action
| throw new PlaywrightException($"Playwright exited with code {installExitCode}"); | ||
| } | ||
|
|
||
| var installDepsExitCode = Microsoft.Playwright.Program.Main(new[] {"install-deps"}); |
There was a problem hiding this comment.
Usually we strongly recommend doing install and --with-deps from the outside. Since these require sudo and modify the system. These should not delay the test execution and skipping it on Linux seems like showing the drawbacks for it.
lets call it from the CI yaml instead? This follows what Playwright users are used to.
|
|
||
| public async Task GoToHomeAndWaitForDataGridLoad(IPage page) | ||
| { | ||
| await page.GotoAsync("/"); |
There was a problem hiding this comment.
I recommend page -> Page like we have in our base classes.
| await page.GotoAsync("/"); | |
| await Page.GotoAsync("/"); |
There was a problem hiding this comment.
As a method parameter, that does not follow naming conventions. If page were a property, I would agree
There was a problem hiding this comment.
I see, thought its a property on the class where page gets created in TestInitialize etc. Feel free to ignore then.
This code was removed. |
|
Thanks @adamint . Does any of my feedback from your original PR apply here? (IDK) |
| public async Task AppBar_Change_Theme() | ||
| { | ||
| // Arrange | ||
| await RunTestAsync(async page => |
There was a problem hiding this comment.
nit: this can return RunTestAsync(..) directly.
| public async Task InitializeAsync() | ||
| { | ||
| PlaywrightProvider.DetectAndSetInstalledPlaywrightDependenciesPath(); | ||
| Browser = await PlaywrightProvider.CreateBrowserAsync(); |
There was a problem hiding this comment.
This would create the browser instance, and launch the browser for every test class using this fixture. For workload tests we use:
https://github.com/adamint/aspire/blob/64ea4ab15976fbf903c9663aa6982205be8b1e2f/tests/Aspire.Workload.Tests/WorkloadTestsBase.cs#L22-L34
and private static Lazy<IBrowser> Browser => new(CreateBrowser);
And the test was skipped on linux. |
Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=724697&view=codecoverage-tab |
…t-dashboard-run-oop # Conflicts: # tests/Aspire.Workload.Tests/EmptyTemplateRunTests.cs # tests/Aspire.Workload.Tests/StarterTemplateRunTestsBase.cs # tests/Shared/Playwright/PlaywrightProvider.cs
|
@adamint I merged the base playwright changes in a separate PR, and update this one to fix the merge conflicts. |
Is anything else needed here? @radical @danmoseley not that I'm aware of. The recommendation is to avoid assuming the structure of the page, so locating elements by text instead of css classes is preferred |
7 participants
Runs dashboard with a mocked dashboard client, adds example playwright test, as we will be adding many more dashboard playwright tests using the same infrastructure in the future.
Microsoft Reviewers: Open in CodeFlow