Add playwright test for changing theme #4154

adamint · 2024-05-10T20:23:56Z

Aspire.Hosting.Tests/Dashboard may not be the best place to put this test long-term. I made it local only for now.

I was unable to access the dashboard frontend from a browser when trying the approach in Aspire.Dashboard.Tests of just creating the dashboard frontend app - it would not connect. If anyone has an idea why that might be, I'm all ears.

I also ran into gRPC issues after orchestrating. Also seeking feedback on that, but in the meantime I injected a mock dashboard service so that there is no grpc connection.

Microsoft Reviewers: Open in CodeFlow

tlmii

Agree on the confusion of why it didn't work the way you originally tried it. It'd also be nice to get some feedback from the Hosting folks about pulling in the dashboard ref here. Just test code, so probably fine, but an ack would be nice.

tlmii · 2024-05-10T21:04:49Z

Directory.Packages.props

@@ -161,4 +162,4 @@
 <PackageVersion Include="Newtonsoft.Json.Schema" Version="3.0.15" />
 <!-- Introduced by Microsoft.Azure.Cosmos, https://github.com/Azure/azure-cosmos-dotnet-v3/issues/4302 -->
 </ItemGroup>
-</Project>


Inadvertent change?

tlmii · 2024-05-10T21:31:21Z

tests/Aspire.Hosting.Tests/Dashboard/Playwright/PlaywrightFixture.cs

+ public Task<ResourceViewModelSubscription> SubscribeResourcesAsync(CancellationToken cancellationToken) => throw new NotImplementedException();
+ }
+
+ public async Task<(string DashboardUrl, IPage Page)> SetupDashboardForPlaywrightAsync()


Not a huge fan of tuples in a public surface. But it's test code so probably OK.

tlmii · 2024-05-10T21:32:31Z

tests/Aspire.Hosting.Tests/Dashboard/Playwright/PlaywrightFixture.cs

+ catch (Exception)
+ {
+ await Task.Delay(1000, cts.Token);
+ await WaitUntilDashboardReadyAsync();


Should there be a point at which this bails out?

also probably a loop rather than recursion

oh, looks like that's the 1 minute timeout on the cts. still, feels like a loop

radical · 2024-05-10T22:37:59Z

FYI, I'm adding some playwright based tests in #3270 . But in that I'm using dotnet run to start up a real app, and then interact with it.

This waits for the dashboard to become available, and uses resiliency to handle retries - https://github.com/radical/aspire/blob/5a900f1f44018a84e6741c45c4d632a1b6e35090/tests/Shared/WorkloadTesting/AspireProject.cs#L408-L427 .

And this connects with playwright, and verifies that the expected resources show up on the dashboard.

danmoseley · 2024-05-10T22:46:09Z

tests/Aspire.Hosting.Tests/Dashboard/Playwright/PlaywrightFixture.cs

+ {
+ await page.GotoAsync(url);
+ }
+ catch (Exception)


do we want to retry on any exception type other than PlaywrightException ?

danmoseley · 2024-05-10T22:49:26Z

tests/Aspire.Hosting.Tests/Dashboard/Playwright/PlaywrightFixture.cs

+ }
+ catch (Exception)
+ {
+ await Task.Delay(1000, cts.Token);


is 1000 reasonable? say, pages typically take 1100 ms to be ready, then it's adding 900ms delay on every test, and perhaps we end with 100 tests.

sometimes in these cases you see a backoff strategy. So starting with brief delays (or just calling GotoAsync again immediately) so the happy path on a machine where it's available fast has no delay. Then backing off progressively the delay for slower machines to release resources for the pages to actually start.

danmoseley · 2024-05-10T22:56:27Z

tests/Aspire.Hosting.Tests/Dashboard/Playwright/PlaywrightFixture.cs

+
+ public async Task DisposeAsync()
+ {
+ await Browser.DisposeAsync();


Browser.CloseAsync() seems canonical in their examples

danmoseley · 2024-05-10T23:01:07Z

tests/Aspire.Hosting.Tests/Dashboard/Playwright/PlaywrightFixture.cs

+ public async Task DisposeAsync()
+ {
+ await Browser.DisposeAsync();
+ PlaywrightInstance.Dispose();


Does it need disposing? I can't find in the docs.

I think in general you don't dispose Task<T> without a reason
https://learn.microsoft.com/en-us/dotnet/api/system.threading.tasks.task.dispose?view=net-8.0#system-threading-tasks-task-dispose

danmoseley · 2024-05-10T23:02:00Z

tests/Aspire.Hosting.Tests/Dashboard/Playwright/AppbarTests.cs

+
+namespace Aspire.Hosting.Tests.Dashboard;
+
+public class AppbarTests : IClassFixture<PlaywrightFixture>


is this the Fluent AppBar? In which case it should be capitalized AppBar

danmoseley · 2024-05-10T23:02:29Z

tests/Aspire.Hosting.Tests/Dashboard/Playwright/AppbarTests.cs

+ await settingsButton.ClickAsync();
+ var darkThemeCheckbox = page.GetByRole(AriaRole.Radio).And(page.GetByText("Dark")).First;
+ await darkThemeCheckbox.ClickAsync();
+ Assert.Equal("dark", await page.Locator("html").First.GetAttributeAsync("data-theme"));


Assert it's not dark before you click the box?

and click again and assert it's light again?

also will this fail if the OS is set to dark theme up front (prefers-color-scheme: dark) ? if so it should instead test for what it is, click the box, check it changed, click the box, check it changed again.

danmoseley · 2024-05-10T23:09:28Z

tests/Aspire.Hosting.Tests/Dashboard/Playwright/AppbarTests.cs

+ var (_, page) = await _playwrightFixture.SetupDashboardForPlaywrightAsync();
+ var settingsButton = page.GetByRole(AriaRole.Button, new()
+ {
+ Name = "Launch settings"


This is matching the English string, right? Is there a way to find this button on any locale, as we want to test all localized builds. Perhaps via a class or id ?

danmoseley · 2024-05-10T23:09:42Z

tests/Aspire.Hosting.Tests/Dashboard/Playwright/AppbarTests.cs

+ Name = "Launch settings"
+ });
+ await settingsButton.ClickAsync();
+ var darkThemeCheckbox = page.GetByRole(AriaRole.Radio).And(page.GetByText("Dark")).First;


same issue with assuming English.

danmoseley · 2024-05-10T23:10:42Z

tests/Aspire.Hosting.Tests/Dashboard/Playwright/PlaywrightFixture.cs

+ public async Task<(string DashboardUrl, IPage Page)> SetupDashboardForPlaywrightAsync()
+ {
+
+ var url = "http://localhost:1234";


does it need a port at all?

danmoseley · 2024-05-10T23:12:58Z

tests/Aspire.Hosting.Tests/Dashboard/Playwright/PlaywrightFixture.cs

+ using var testProgram = TestProgram.Create<PlaywrightFixture>(
+ args,
+ includeIntegrationServices: false,
+ disableDashboard: false);


is this includeNodeApp actually?

aspire/tests/testproject/TestProject.AppHost/TestProgram.cs

Lines 151 to 157 in 612429c

public static TestProgram Create<T>(

string[]? args = null,

bool includeIntegrationServices = false,

bool includeNodeApp = false,

bool disableDashboard = true,

bool allowUnsecuredTransport = true,

bool randomizePorts = true)

hmm but if that's so how is the named param compiling? I must be missing an overload

danmoseley · 2024-05-10T23:17:37Z

tests/Aspire.Hosting.Tests/Dashboard/Playwright/PlaywrightFixture.cs

+ {
+ try
+ {
+ await page.GotoAsync(url);


I was looking to see whether there's a way to pass in the cts. But one can only pass a timeout (via PageGotoOptions) which is otherwise 30 seconds .. so I guess no reason to change that.

kvenkatrajan · 2024-05-21T18:42:08Z

Was this closed by accident?

adamint · 2024-05-21T18:53:22Z

@kvenkatrajan we are going to use Ankit’s e2e infrastructure so 90% of this pr content will be replaced :)

danmoseley · 2024-05-22T17:48:19Z

thanks for working together on this, sounds like we'll end up with a single infrastructure and it will be easier for everyone.

(when you have your new PR could you check whether any of the feedback above still applies?)

Adam Ratzman added 2 commits May 10, 2024 16:18

Add theme changing playwright test

c609522

switch appbar test to localonly

1a7dbee

adamint requested review from drewnoakes, tlmii and kvenkatrajan May 10, 2024 20:23

dotnet-issue-labeler bot added the area-components Issues pertaining to Aspire Component packages label May 10, 2024

tlmii reviewed May 10, 2024

View reviewed changes

danmoseley reviewed May 10, 2024

View reviewed changes

adamint closed this May 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add playwright test for changing theme #4154

Add playwright test for changing theme #4154

adamint commented May 10, 2024 •

edited by dotnet-policy-service bot

tlmii left a comment

tlmii May 10, 2024

tlmii May 10, 2024

tlmii May 10, 2024

danmoseley May 10, 2024

danmoseley May 10, 2024

radical commented May 10, 2024

danmoseley May 10, 2024

danmoseley May 10, 2024

danmoseley May 10, 2024

danmoseley May 10, 2024

danmoseley May 10, 2024

danmoseley May 10, 2024

danmoseley May 10, 2024

danmoseley May 10, 2024

danmoseley May 10, 2024

danmoseley May 10, 2024

danmoseley May 10, 2024

danmoseley May 10, 2024

danmoseley May 10, 2024

danmoseley May 10, 2024

kvenkatrajan commented May 21, 2024

adamint commented May 21, 2024

danmoseley commented May 22, 2024


		namespace Aspire.Hosting.Tests.Dashboard;

		public class AppbarTests : IClassFixture<PlaywrightFixture>

	public static TestProgram Create<T>(
	string[]? args = null,
	bool includeIntegrationServices = false,
	bool includeNodeApp = false,
	bool disableDashboard = true,
	bool allowUnsecuredTransport = true,
	bool randomizePorts = true)

Add playwright test for changing theme #4154

Add playwright test for changing theme #4154

Conversation

adamint commented May 10, 2024 • edited by dotnet-policy-service bot

Microsoft Reviewers: Open in CodeFlow

tlmii left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radical commented May 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kvenkatrajan commented May 21, 2024

adamint commented May 21, 2024

danmoseley commented May 22, 2024

adamint commented May 10, 2024 •

edited by dotnet-policy-service bot