Skip to content

Conversation

Youssef1313
Copy link
Member

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes # (issue)

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No
  • Does the change require an update in our Aspire docs?

@Youssef1313
Copy link
Member Author

Youssef1313 commented Apr 17, 2025

@captainsafia Could you test this please in VS Code? Provide me with logs if it fails please.

@github-actions github-actions bot added the area-engineering-systems infrastructure helix infra engineering repo stuff label Apr 17, 2025
@Youssef1313
Copy link
Member Author

Youssef1313 commented Apr 17, 2025

@radical
Copy link
Member

radical commented Apr 17, 2025

Let's try to make sure these are correct before merging:

cc @RussKie @captainsafia @danmoseley

@Youssef1313
Copy link
Member Author

@mitchdenny Can you check the CodeSpaces part please from this branch?

@mitchdenny
Copy link
Member

Whatever is in this PR seems to work.

  • Can debug a test
  • Tests show up after building test project (not sure if its 100% or not, but the ones I tested worked)
  • Play button next to test in source is visible
  • "Go to test" menu item shows up and works

This looks good to me!

@Youssef1313
Copy link
Member Author

Youssef1313 commented Apr 17, 2025

Thanks for confirming @mitchdenny

If you can also confirm whether current main works or not please? Because I'm suspecting that reverting MTP PR may still not work.

@Youssef1313 Youssef1313 marked this pull request as ready for review April 17, 2025 09:38
@Youssef1313
Copy link
Member Author

@radical @RussKie Can we get this one merged soon please?

@Youssef1313
Copy link
Member Author

Ping @danmoseley @captainsafia @radical. If you can confirm if this is working and get it merged please. Thanks!

@captainsafia
Copy link
Member

Ping @danmoseley @captainsafia @radical. If you can confirm if this is working and get it merged please. Thanks!

I haven't success with test discovery on the branch locally. Attempting to refresh in the test explorer produces the following logs and discovers no tests:

Created Test Controller
Log verbosity: diagnostic
[diag] C# Dev Kit version: 1.19.20
[diag] Test Explorer version: 14
[diag] platform: darwin
[diag] arch: arm64
[diag] Calling dotnet.findPath with context: {"acquireContext":{"version":"0.0","requestingExtensionId":"ms-dotnettools.csdevkit","architecture":"arm64","mode":"sdk","installType":"global"},"versionSpecRequirement":"greater_than_or_equal","rejectPreviews":false}
[diag] Found dotnet.exe at: /Users/captainsafia/git/aspire/mtp-fix/.dotnet/dotnet
Checking environment variable 'DOTNET_HOST_PATH': Current value = '<null>', Proposed value = '/Users/captainsafia/git/aspire/mtp-fix/.dotnet/dotnet'
Initialized Test Explorer Server [2327]
Updated environment variable 'DOTNET_HOST_PATH' to '/Users/captainsafia/git/aspire/mtp-fix/.dotnet/dotnet' (was '<null>', invalid or missing)
Found global.json at /Users/captainsafia/git/aspire/mtp-fix/global.json
Test Store Folder: /Users/captainsafia/Library/Application Support/Code/User/workspaceStorage/0f0ea6e3a73431b9294637d01d229b07/ms-dotnettools.csdevkit
Using vstest from dotnet sdk in [/Users/captainsafia/git/aspire/mtp-fix/.dotnet/sdk/9.0.200].
Test data store opened in 0.005 sec.
[diag] Test Store loaded from persistance: No
Test controller initialized.
Initialized project system provider.
Waiting for projects to be detected by Project System...
No runSettingsPath configured. Falling back to default settings.
79 projects added, 0 changed, 0 removed.
No runSettingsPath configured. Falling back to default settings.

@captainsafia
Copy link
Member

Ping @danmoseley @captainsafia @radical. If you can confirm if this is working and get it merged please. Thanks!

I haven't success with test discovery on the branch locally. Attempting to refresh in the test explorer produces the following logs and discovers no tests:

Created Test Controller
Log verbosity: diagnostic
[diag] C# Dev Kit version: 1.19.20
[diag] Test Explorer version: 14
[diag] platform: darwin
[diag] arch: arm64
[diag] Calling dotnet.findPath with context: {"acquireContext":{"version":"0.0","requestingExtensionId":"ms-dotnettools.csdevkit","architecture":"arm64","mode":"sdk","installType":"global"},"versionSpecRequirement":"greater_than_or_equal","rejectPreviews":false}
[diag] Found dotnet.exe at: /Users/captainsafia/git/aspire/mtp-fix/.dotnet/dotnet
Checking environment variable 'DOTNET_HOST_PATH': Current value = '<null>', Proposed value = '/Users/captainsafia/git/aspire/mtp-fix/.dotnet/dotnet'
Initialized Test Explorer Server [2327]
Updated environment variable 'DOTNET_HOST_PATH' to '/Users/captainsafia/git/aspire/mtp-fix/.dotnet/dotnet' (was '<null>', invalid or missing)
Found global.json at /Users/captainsafia/git/aspire/mtp-fix/global.json
Test Store Folder: /Users/captainsafia/Library/Application Support/Code/User/workspaceStorage/0f0ea6e3a73431b9294637d01d229b07/ms-dotnettools.csdevkit
Using vstest from dotnet sdk in [/Users/captainsafia/git/aspire/mtp-fix/.dotnet/sdk/9.0.200].
Test data store opened in 0.005 sec.
[diag] Test Store loaded from persistance: No
Test controller initialized.
Initialized project system provider.
Waiting for projects to be detected by Project System...
No runSettingsPath configured. Falling back to default settings.
79 projects added, 0 changed, 0 removed.
No runSettingsPath configured. Falling back to default settings.

Some debugging reveals that the issue might've been the useTestingPlatformProtocol setting being set to true in our workspace definition. Setting it to false unblocks the build/test/debug loop for me.

We should validate that things work in VS Code after changing this setting and do one last sanity check on VS to ensure the dev inner loop on the repo is healthy.

@radical
Copy link
Member

radical commented Apr 17, 2025

@Youssef1313
Copy link
Member Author

I kicked a new AzDO build after the last commit (the last commit shouldn't have any effect on AzDO though as it just changes a VS Code setting). Hopefully now that it's working for @captainsafia it should be ready to merge. I confirmed VS works for me as well.

@radical
Copy link
Member

radical commented Apr 17, 2025

When I run a test project locally with ./dotnet.sh test <project>.. no .trx files are generated.

Looking at just the log it is hard to figure out which test failed. With the existing setup I can grep for [FAIL] in the log. What would be the equivalent with mtp?


Test run summary: Failed! - /Users/ankj/dev/a3/artifacts/bin/Aspire.Hosting.Testing.Tests/Release/net8.0/Aspire.Hosting.Testing.Tests.dll (net8.0|arm64)
  total: 100
  failed: 1
  succeeded: 99
  skipped: 0
  duration: 7m 40s 604ms

=== COMMAND LINE ===
/Users/ankj/dev/a3/artifacts/bin/Aspire.Hosting.Testing.Tests/Release/net8.0/Aspire.Hosting.Testing.Tests --internal-msbuild-node /var/folders/dj/z2lsb9l9087356sgyzrlklvr0000gn/T/d14f9875594f4d12a72952dd545f6029/.p --filter-not-trait "category=failing"

@Youssef1313
Copy link
Member Author

Try with /tl:off or -v:detailed

@radical
Copy link
Member

radical commented Apr 18, 2025

Try with /tl:off or -v:detailed

But shouldn't the .trx be generated here?

@Youssef1313
Copy link
Member Author

Try with /tl:off or -v:detailed

But shouldn't the .trx be generated here?

No. Same with VSTest.
In VSTest you'd need dotnet test --logger trx

In MTP, you need dotnet test -- --report-trx (extra -- is intentional and is required. But will no longer be required in .NET 10)

@radical
Copy link
Member

radical commented Apr 18, 2025

Try with /tl:off or -v:detailed

But shouldn't the .trx be generated here?

No. Same with VSTest. In VSTest you'd need dotnet test --logger trx

In MTP, you need dotnet test -- --report-trx (extra -- is intentional and is required. But will no longer be required in .NET 10)

We should add that. It would be fine to do that in a follow up too.

Copy link
Member

@radical radical left a comment

Choose a reason for hiding this comment

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

LGTM! @captainsafia @danmoseley does this look good to merge?

@danmoseley
Copy link
Member

I would like @captainsafia @mitchdenny to confirm that they're unblocked before merging

@Youssef1313
Copy link
Member Author

@danmoseley From offline chat with @captainsafia this works for her.

@mitchdenny confirmed this is working in #8833 (comment), but I pushed a change after. So might be worth to have a second confirmation it still works.

Copy link
Member

@captainsafia captainsafia left a comment

Choose a reason for hiding this comment

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

Green check for VS Code on macOS build/test/debug loop.

@mitchdenny
Copy link
Member

Doesn't seem to be working on codespaces :(

image

I just build tests/Aspire.Cli.Tests and test explorer hasn't shown up any tests at all. I would have expected tests for Aspire.Cli.Tests and Aspire.Hosting.Tests.

@mitchdenny
Copy link
Member

Interesting if I did a full refresh on the tests in test explorer they started showing up - but not the test project that I want.

@Youssef1313
Copy link
Member Author

Interesting if I did a full refresh on the tests in test explorer they started showing up - but not the test project that I want.

Can you double check if anything here is different from main? I wouldn't expect so.

@mitchdenny
Copy link
Member

Nothing specific to that test project.

@Youssef1313
Copy link
Member Author

Nothing specific to that test project.

I'm not really following. Is there something that works on main but broken with this PR?

@mitchdenny
Copy link
Member

Unreliability of loading tests in test explorer is not new. I just created a fresh codespace and it worked this time. I have no idea why it works sometimes and not others. Are you relying of file change notifications to decide whether to scan an assembly?

@Youssef1313
Copy link
Member Author

We rely on build. Once a project is built, we start discovery.

Unreliability of loading tests in test explorer is not new.

In that case, we should be fine proceeding with this PR right?

@mitchdenny
Copy link
Member

Yes, we should merge this PR. However, I believe that there is a DevKit bug with test discovery that needs to be solve. Test discovery missing assemblies is enough of a common issue that there has to be a real issue here.

@Youssef1313 Youssef1313 merged commit 0af9e78 into main Apr 19, 2025
187 of 194 checks passed
@Youssef1313 Youssef1313 deleted the dev/ygerges/mtp-back branch April 19, 2025 12:34
radical added a commit to radical/aspire that referenced this pull request May 15, 2025
 ## What broke?

Template tests run `dotnet test --list-tests` to get the list of tests so they can
be run on separate helix jobs. This run of `dotnet` failed the builds on
azdo with:

```
  You must install or update .NET to run this application.

  App: /mnt/vss/_work/1/s/artifacts/bin/Aspire.Templates.Tests/Release/net8.0/Aspire.Templates.Tests
  Architecture: x64
  Framework: 'Microsoft.NETCore.App', version '8.0.0' (x64)
  .NET location: /usr/lib/dotnet

  The following frameworks were found:
    6.0.36 at [/usr/lib/dotnet/shared/Microsoft.NETCore.App]

  Learn about framework resolution:
  https://aka.ms/dotnet/app-launch-failed

  To install missing framework, download:
  https://aka.ms/dotnet-core-applaunch?framework=Microsoft.NETCore.App&framework_version=8.0.0&arch=x64&rid=ubuntu.22.04-x64
/mnt/vss/_work/1/s/tests/Directory.Build.targets(39,5): error MSB3073: The command ""/mnt/vss/_work/1/s/artifacts/bin/Aspire.Templates.Tests/Release/net8.0/Aspire.Templates.Tests" --filter-no>
 ##[error]tests/Directory.Build.targets(39,5): error MSB3073: (NETCORE_ENGINEERING_TELEMETRY=Build) The command ""/mnt/vss/_work/1/s/artifacts/bin/Aspire.Templates.Tests/Release/net8.0/Aspire>
```

 ## Why did it break?

[1] added a change where that `dotnet test` was run with
`EnvironmentVariables="DOTNET_ROOT=$(DotNetRoot);DOTNET_ROOT_X86=$(DotNetRoot)x86"`,
so it does not use the system dotnet. This helped on Azdo builds where
we have older system dotnet (6.0x).

Then [2] removed that change so the system dotnet on github actions
could be used because a newer one was being installed. But this now
broke the Azdo builds.

To fix this set the DOTNET_ROOT on Azdo builds, so the correct dotnet is
used.

References:
1.
commit 0af9e78
Author: Youssef Victor <[email protected]>
Date:   Sat Apr 19 14:34:54 2025 +0200

    Revert to MTP and disable server capability as a workaround (dotnet#8833)

2.
```
commit 6ca2de9
Author: Ankit Jain <[email protected]>
Date:   Wed May 14 11:39:18 2025 -0400

    [CI] Add PR validation on macOS (dotnet#9287)
```
radical added a commit that referenced this pull request May 15, 2025
 ## What broke?

Template tests run `dotnet test --list-tests` to get the list of tests so they can
be run on separate helix jobs. This run of `dotnet` failed the builds on
azdo with:

```
  You must install or update .NET to run this application.

  App: /mnt/vss/_work/1/s/artifacts/bin/Aspire.Templates.Tests/Release/net8.0/Aspire.Templates.Tests
  Architecture: x64
  Framework: 'Microsoft.NETCore.App', version '8.0.0' (x64)
  .NET location: /usr/lib/dotnet

  The following frameworks were found:
    6.0.36 at [/usr/lib/dotnet/shared/Microsoft.NETCore.App]

  Learn about framework resolution:
  https://aka.ms/dotnet/app-launch-failed

  To install missing framework, download:
  https://aka.ms/dotnet-core-applaunch?framework=Microsoft.NETCore.App&framework_version=8.0.0&arch=x64&rid=ubuntu.22.04-x64
/mnt/vss/_work/1/s/tests/Directory.Build.targets(39,5): error MSB3073: The command ""/mnt/vss/_work/1/s/artifacts/bin/Aspire.Templates.Tests/Release/net8.0/Aspire.Templates.Tests" --filter-no>
 ##[error]tests/Directory.Build.targets(39,5): error MSB3073: (NETCORE_ENGINEERING_TELEMETRY=Build) The command ""/mnt/vss/_work/1/s/artifacts/bin/Aspire.Templates.Tests/Release/net8.0/Aspire>
```

 ## Why did it break?

[1] added a change where that `dotnet test` was run with
`EnvironmentVariables="DOTNET_ROOT=$(DotNetRoot);DOTNET_ROOT_X86=$(DotNetRoot)x86"`,
so it does not use the system dotnet. This helped on Azdo builds where
we have older system dotnet (6.0x).

Then [2] removed that change so the system dotnet on github actions
could be used because a newer one was being installed. But this now
broke the Azdo builds.

To fix this set the DOTNET_ROOT on Azdo builds, so the correct dotnet is
used.

References:
1.
commit 0af9e78
Author: Youssef Victor <[email protected]>
Date:   Sat Apr 19 14:34:54 2025 +0200

    Revert to MTP and disable server capability as a workaround (#8833)

2.
```
commit 6ca2de9
Author: Ankit Jain <[email protected]>
Date:   Wed May 14 11:39:18 2025 -0400

    [CI] Add PR validation on macOS (#9287)
```
radical added a commit that referenced this pull request May 15, 2025
* [CI] Fix azdo builds for `main`

 ## What broke?

Template tests run `dotnet test --list-tests` to get the list of tests so they can
be run on separate helix jobs. This run of `dotnet` failed the builds on
azdo with:

```
  You must install or update .NET to run this application.

  App: /mnt/vss/_work/1/s/artifacts/bin/Aspire.Templates.Tests/Release/net8.0/Aspire.Templates.Tests
  Architecture: x64
  Framework: 'Microsoft.NETCore.App', version '8.0.0' (x64)
  .NET location: /usr/lib/dotnet

  The following frameworks were found:
    6.0.36 at [/usr/lib/dotnet/shared/Microsoft.NETCore.App]

  Learn about framework resolution:
  https://aka.ms/dotnet/app-launch-failed

  To install missing framework, download:
  https://aka.ms/dotnet-core-applaunch?framework=Microsoft.NETCore.App&framework_version=8.0.0&arch=x64&rid=ubuntu.22.04-x64
/mnt/vss/_work/1/s/tests/Directory.Build.targets(39,5): error MSB3073: The command ""/mnt/vss/_work/1/s/artifacts/bin/Aspire.Templates.Tests/Release/net8.0/Aspire.Templates.Tests" --filter-no>
 ##[error]tests/Directory.Build.targets(39,5): error MSB3073: (NETCORE_ENGINEERING_TELEMETRY=Build) The command ""/mnt/vss/_work/1/s/artifacts/bin/Aspire.Templates.Tests/Release/net8.0/Aspire>
```

 ## Why did it break?

[1] added a change where that `dotnet test` was run with
`EnvironmentVariables="DOTNET_ROOT=$(DotNetRoot);DOTNET_ROOT_X86=$(DotNetRoot)x86"`,
so it does not use the system dotnet. This helped on Azdo builds where
we have older system dotnet (6.0x).

Then [2] removed that change so the system dotnet on github actions
could be used because a newer one was being installed. But this now
broke the Azdo builds.

To fix this set the DOTNET_ROOT on Azdo builds, so the correct dotnet is
used.

References:
1.
commit 0af9e78
Author: Youssef Victor <[email protected]>
Date:   Sat Apr 19 14:34:54 2025 +0200

    Revert to MTP and disable server capability as a workaround (#8833)

2.
```
commit 6ca2de9
Author: Ankit Jain <[email protected]>
Date:   Wed May 14 11:39:18 2025 -0400

    [CI] Add PR validation on macOS (#9287)
```

* Quarantine BrowserTokenAuthenticationTests.BrowserToken_LoginPage_Success_RedirectToResources

Issue: #9345
@github-actions github-actions bot locked and limited conversation to collaborators May 20, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-engineering-systems infrastructure helix infra engineering repo stuff
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants