Run PR validation tests on windows #7909

radical · 2025-03-05T18:51:12Z

Tests for PR validation are run for Windows, and Linux now.
Docker dependent tests are disabled on windows here too, same as for helix
Sort the list of tests

- Adjust the github workflow to handle building, and running on linux, and windows

…e nested containers are not supported

PR Overview

This PR updates the GitHub Actions workflow to run PR validation tests on both Windows and Linux and disables Docker-dependent tests on Windows.

Adds a matrix strategy to run tests on Ubuntu and Windows with OS-specific scripts.
Adjusts the test, build, and environment setup steps for Windows compatibility.

Reviewed Changes

File	Description
.github/workflows/run-tests.yml	Updated job configuration to run tests on both Ubuntu and Windows; added matrix strategy for OS-specific scripts and modified steps accordingly.
tests/Aspire.Components.Common.Tests/PlatformDetection.cs	Added support for detecting GitHub Actions environment by introducing a new property and updating the CI detection.

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

.github/workflows/run-tests.yml:52

Consider using 'matrix.os' instead of 'runner.os' for consistency with other steps in the workflow. This will ensure that the condition correctly reflects the operating system as defined in the job matrix.

if: runner.os == 'Linux' && (inputs.testShortName == 'Playground' || inputs.testShortName == 'Azure')

…with IsRunningOn

eerhardt · 2025-03-06T16:11:45Z

tests/Aspire.Hosting.Tests/SlimTestProgramTests.cs

@@ -18,6 +19,7 @@ public SlimTestProgramTests(SlimTestProgramFixture slimTestProgramFixture)
    }

    [Fact]
+    [ActiveIssue("https://github.com/dotnet/aspire/issues/7923", typeof(PlatformDetection), nameof(PlatformDetection.IsRunningOnGithubActions), nameof(PlatformDetection.IsWindows))]


I mentioned it in #7923, but maybe these should just have [RequiresDocker] attributes?

Why are these not failing on the rolling builds then?

That's a good question. Are they running there?

Yeah -

https://dev.azure.com/dnceng-public/public/_build/results?buildId=965263&view=ms.vss-test-web.build-test-results-tab

eerhardt · 2025-03-06T16:12:29Z

tests/Aspire.Hosting.Testing.Tests/TestingBuilderTests.cs

@@ -211,6 +211,7 @@ public async Task GetHttpClientBeforeStart(bool genericEntryPoint)
    [InlineData(false, true)]
    [InlineData(true, false)]
    [InlineData(true, true)]
+    [ActiveIssue("https://github.com/dotnet/aspire/issues/7930", typeof(PlatformDetection), nameof(PlatformDetection.IsRunningOnGithubActions), nameof(PlatformDetection.IsWindows))]


Same here - This test requires docker from what I can see.

eerhardt · 2025-03-06T16:13:54Z

tests/Aspire.Cli.Tests/Hosting/CliOrphanDetectorTests.cs

@@ -93,6 +94,7 @@ public async Task CliOrphanDetectorAfterTheProcessWasRunningForAWhileThenStops()
    }

    [Fact]
+    [ActiveIssue("https://github.com/dotnet/aspire/issues/7920", typeof(PlatformDetection), nameof(PlatformDetection.IsRunningOnGithubActions), nameof(PlatformDetection.IsWindows))]


I think this test requires docker.

Discussed directly. All these tests are running fine on windows on main without docker.

eerhardt · 2025-03-06T16:15:57Z

.github/workflows/tests.yml

+      - name: Get list of tests
+        env:
+          CI: false
+        run: >


for my learning, why do we sometimes use | and sometimes use >?

I was using | earlier as just continuing what was being used. | means that it keeps the newlines, so:

- run: | foo bar

.. would be foo, and bar as separate commands on the shell. And for unix we were using \ at the end of the lines to concatenate them. But that doesn't work on windows, so I started using > - which by default converts the newlines to spaces, so the lines join to become one.

https://yaml-multiline.info

eerhardt · 2025-03-06T16:16:30Z

.github/workflows/tests.yml

      - name: Generate tests matrix
        id: generate_test_matrix
+        shell: pwsh


powershell on linux... nice!

eerhardt · 2025-03-06T16:18:30Z

.github/workflows/run-tests.yml

+        if: matrix.os == 'ubuntu-latest'
+        run: ${{ matrix.dotnet_script }} dev-certs https --trust


Why don't the dev-certs need to be trusted on Windows?

I wasn't able to get that running without prompt popping up. I seem to recall that you can pass echo yes or something to it, but couldn't get something working. And .. the tests seem to be passing anyway? So, I have this disabled right now.

eerhardt · 2025-03-06T16:44:35Z

.github/workflows/run-tests.yml

+          $projectPath1 = "${{ github.workspace }}/tests/$testShortName.Tests/$testShortName.Tests.csproj"
+          $projectPath2 = "${{ github.workspace }}/tests/Aspire.$testShortName.Tests/Aspire.$testShortName.Tests.csproj"
+
+          if (Test-Path -Path $projectPath1) {


Would it be a bad idea to have both "display name" and "file name" properties on the test inputs? That way we wouldn't need to do this guessing at the file path. (not necessary here - can be a follow up)

I had it that way earlier, but any matrix inputs get serialized to the display line in PR checks. Integration / test / (Hosting, tests/Aspire.Hosting.Tests/Aspire.Hosting.Tests.csproj), which made it less readable. So, I went with this.

danmoseley · 2025-03-06T18:42:12Z

.github/workflows/run-tests.yml


      - name: Verify Docker is running
+        # nested docker containers not supported on windows


Oh just a GHA thing?

GHA, and our pipelines too - on Windows. This file is gha only though.

tests/Aspire.Playground.Tests/Aspire.Playground.Tests.csproj

Co-authored-by: Dan Moseley <[email protected]>

radical · 2025-03-06T19:59:43Z

@eerhardt I'm merging this. If you have any more feedback, then I can address those in a follow up.

eerhardt

LGTM

radical added 2 commits March 5, 2025 17:30

[CI] Enable tests to run on Windows for PR validation

dfd69bc

- Adjust the github workflow to handle building, and running on linux, and windows

Disable docker tests on windows when running on github actions becaus…

ab2768b

…e nested containers are not supported

radical force-pushed the tests-pr-windows branch from 72551ee to ab2768b Compare March 5, 2025 22:31

radical requested review from davidfowl, danmoseley, eerhardt and joperezr March 5, 2025 22:39

radical marked this pull request as ready for review March 5, 2025 22:40

Copilot bot review requested due to automatic review settings March 5, 2025 22:40

Copilot AI reviewed Mar 5, 2025

View reviewed changes

radical added 4 commits March 5, 2025 18:00

Handle tests disabled by OS also

5c56826

fix yml

bef3b18

fix yml

afcd390

fix yml

8d76359

radical marked this pull request as draft March 5, 2025 23:06

radical added 2 commits March 5, 2025 18:13

fix yml

2dba0e3

Fix conditions for linux

ee7b25f

radical marked this pull request as ready for review March 6, 2025 00:00

radical added 5 commits March 5, 2025 19:21

add comment

975ac46

Simplify os name showed in github checks

ee49cd8

address review feedback from @ eerhardt

47b7dcc

rename

369138c

fixy yml

bacb51c

This was referenced Mar 6, 2025

Failing Dashboard test - BrowserToken_LoginPage_Success_RedirectToResources #7921

Open

Failing windows tests for PR validation #7922

Open

Failing Aspire.Hosting tests on Windows for PR validation #7923

Open

radical added 4 commits March 6, 2025 08:50

Add IsWindows, IsLinux to PlatformDetection to enable combining them …

32e7327

…with IsRunningOn

Disable test AppHostExitsWhenCliProcessPidDies

e9a0103

Merge remote-tracking branch 'origin/main' into tests-pr-windows

d6b73cb

Disable failing windows tests on github actions

00ea829

Disable failing Dashboard tests on github actions

913c5ad

radical requested a review from mitchdenny as a code owner March 6, 2025 14:18

Disable failing Hosting.Testing tests on github actions

1c2fce4

radical requested a review from ReubenBond as a code owner March 6, 2025 15:20

Sort list of test classes

1e94339

eerhardt reviewed Mar 6, 2025

View reviewed changes

radical mentioned this pull request Mar 6, 2025

Testing infrastructure tasks #7837

Open

5 tasks

danmoseley approved these changes Mar 6, 2025

View reviewed changes

Update tests/Aspire.Playground.Tests/Aspire.Playground.Tests.csproj

9ad2cc2

Co-authored-by: Dan Moseley <[email protected]>

eerhardt approved these changes Mar 6, 2025

View reviewed changes

radical merged commit 0ae0f0b into dotnet:main Mar 6, 2025
137 checks passed

radical deleted the tests-pr-windows branch March 6, 2025 20:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run PR validation tests on windows #7909

Run PR validation tests on windows #7909

radical commented Mar 5, 2025 •

edited

Loading

eerhardt Mar 6, 2025

radical Mar 6, 2025

eerhardt Mar 6, 2025

radical Mar 6, 2025

eerhardt Mar 6, 2025

eerhardt Mar 6, 2025

radical Mar 6, 2025

eerhardt Mar 6, 2025

radical Mar 6, 2025

eerhardt Mar 6, 2025

eerhardt Mar 6, 2025

radical Mar 6, 2025

eerhardt Mar 6, 2025

radical Mar 6, 2025

danmoseley Mar 6, 2025

danmoseley Mar 6, 2025

radical Mar 6, 2025 •

edited

Loading

radical commented Mar 6, 2025

eerhardt left a comment

		if: matrix.os == 'ubuntu-latest'
		run: ${{ matrix.dotnet_script }} dev-certs https --trust


		- name: Verify Docker is running
		# nested docker containers not supported on windows

Run PR validation tests on windows #7909

Run PR validation tests on windows #7909

Conversation

radical commented Mar 5, 2025 • edited Loading

Choose a reason for hiding this comment

PR Overview

Reviewed Changes

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radical Mar 6, 2025 • edited Loading

Choose a reason for hiding this comment

radical commented Mar 6, 2025

eerhardt left a comment

Choose a reason for hiding this comment

radical commented Mar 5, 2025 •

edited

Loading

radical Mar 6, 2025 •

edited

Loading