Skip to content

[tests] Extract playwright bits to Playwright.targets, and separate class#4689

Merged
radical merged 6 commits intomicrosoft:mainfrom
radical:playwright-targets
Jun 27, 2024
Merged

[tests] Extract playwright bits to Playwright.targets, and separate class#4689
radical merged 6 commits intomicrosoft:mainfrom
radical:playwright-targets

Conversation

@radical
Copy link
Copy Markdown
Member

@radical radical commented Jun 26, 2024

... to allow re-use outside the workload tests.

  • Extracts the Playwright specific msbuild bits to tests/Shared/Playwright/
  • A test project can import tests/Shared/Playwright/Playwright.targets to get support for installing the dependencies on build.

Prompted by #4386 .

Microsoft Reviewers: Open in CodeFlow

@ghost ghost added the area-integrations Issues pertaining to Aspire Integrations packages label Jun 26, 2024
@radical radical force-pushed the playwright-targets branch 2 times, most recently from a6a6fbe to c138703 Compare June 27, 2024 00:57
…lass to allow re-use outside the workload tests
@radical radical force-pushed the playwright-targets branch from c138703 to a6648e8 Compare June 27, 2024 01:32
Comment thread tests/Aspire.Workload.Tests/Aspire.Workload.Tests.csproj Outdated
@radical radical requested a review from joperezr June 27, 2024 03:21
@radical radical marked this pull request as ready for review June 27, 2024 03:21
@radical radical requested a review from eerhardt as a code owner June 27, 2024 03:21
# Conflicts:
#	tests/Shared/WorkloadTesting/EnvironmentVariables.cs
Comment thread tests/Shared/Playwright/Playwright.targets Outdated
Comment thread tests/Shared/Playwright/PlaywrightProvider.cs Outdated
return;
}

if (repoRoot is null)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this case ever happen? If repoRoot is null its because we are in Helix, right?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is if the caller didn't pass in a pre-computed repoRoot then we do the detection here. In BuildEnvironment.cs we already have repoRoot computed so this would avoid it being repeated. But when using it outside the workload tests, the caller might not have it, and thus pass null here.

But now that you mention it, I'm repeating this code in BuildEnvironment.cs . This should become a separate method .. somewhere!

Copy link
Copy Markdown
Member Author

@radical radical Jun 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extracted that to a TestUtils.FindRepoRoot.

@dotnet-bot
Copy link
Copy Markdown
Contributor

‼️ Found issues ‼️

Project Coverage Type Expected Actual
Microsoft.Extensions.ServiceDiscovery Branch 81 79.31 🔻
Microsoft.Extensions.ServiceDiscovery Line 81 79.01 🔻

Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=723056&view=codecoverage-tab

@radical radical requested a review from eerhardt June 27, 2024 22:26
@radical radical merged commit 38f987e into microsoft:main Jun 27, 2024
@radical radical deleted the playwright-targets branch June 27, 2024 22:27
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-integrations Issues pertaining to Aspire Integrations packages testing ☑️

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants