Skip to content

Conversation

RussKie
Copy link
Contributor

@RussKie RussKie commented Apr 3, 2025

No description provided.

@Copilot Copilot AI review requested due to automatic review settings April 3, 2025 08:05
@github-actions github-actions bot added the area-integrations Issues pertaining to Aspire Integrations packages label Apr 3, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR revises the documentation in tests/README.md to reflect updates in variable naming for controlling test environments and project test capabilities. Key changes include updating text for clarity and consistency with renamed MSBuild properties, along with minor formatting and capitalization improvements.

  • Updated CI and Helix instructions to match variable renaming
  • Improved capitalization and consistency in documentation
Files not reviewed (13)
  • eng/Testing.props: Language not supported
  • eng/Testing.targets: Language not supported
  • tests/Aspire.Cli.Tests/Aspire.Cli.Tests.csproj: Language not supported
  • tests/Aspire.Dashboard.Tests/Aspire.Dashboard.Tests.csproj: Language not supported
  • tests/Aspire.Elastic.Clients.Elasticsearch.Tests/Aspire.Elastic.Clients.Elasticsearch.Tests.csproj: Language not supported
  • tests/Aspire.EndToEnd.Tests/Aspire.EndToEnd.Tests.csproj: Language not supported
  • tests/Aspire.Hosting.Docker.Tests/Aspire.Hosting.Docker.Tests.csproj: Language not supported
  • tests/Aspire.Hosting.Testing.Tests/Aspire.Hosting.Testing.Tests.csproj: Language not supported
  • tests/Aspire.Hosting.Tests/Aspire.Hosting.Tests.csproj: Language not supported
  • tests/Aspire.Oracle.EntityFrameworkCore.Tests/Aspire.Oracle.EntityFrameworkCore.Tests.csproj: Language not supported
  • tests/Aspire.Playground.Tests/Aspire.Playground.Tests.csproj: Language not supported
  • tests/Directory.Build.targets: Language not supported
  • tests/Shared/RepoTesting/Aspire.RepoTesting.targets: Language not supported

@RussKie RussKie force-pushed the igveliko/test_vars branch from 7f036e5 to dc32257 Compare April 3, 2025 08:05
@RussKie RussKie marked this pull request as draft April 3, 2025 08:23
@RussKie RussKie force-pushed the igveliko/test_vars branch from 53c34c0 to 9f00bb2 Compare April 3, 2025 09:22
@RussKie RussKie marked this pull request as ready for review April 3, 2025 09:37
@RussKie RussKie added area-engineering-systems infrastructure helix infra engineering repo stuff and removed area-integrations Issues pertaining to Aspire Integrations packages labels Apr 3, 2025
@RussKie RussKie self-assigned this Apr 3, 2025
@RussKie RussKie force-pushed the igveliko/test_vars branch from 9f00bb2 to bf6e745 Compare April 4, 2025 01:16
@radical
Copy link
Member

radical commented Apr 4, 2025

We should also run a Azdo build to check that nothing breaks.

@RussKie RussKie force-pushed the igveliko/test_vars branch from 164a73e to fc7b570 Compare April 8, 2025 00:05
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.

Looks good. Added some comments. Is the azdo build for this looking good too?

@RussKie
Copy link
Contributor Author

RussKie commented Apr 8, 2025

Is the azdo build for this looking good too?

Yes, both a PR and a full manual build

@RussKie RussKie force-pushed the igveliko/test_vars branch from 8d9939b to 832bf9c Compare April 8, 2025 04:27
@radical
Copy link
Member

radical commented Apr 8, 2025

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! Thank you for working on this, and patience with the review process!

Feel free to merge once the latest azdo run is green with the same test count :)

@RussKie RussKie merged commit a6af591 into main Apr 8, 2025
179 checks passed
@RussKie RussKie deleted the igveliko/test_vars branch April 8, 2025 06:36

<PropertyGroup>
<RunOnGithubActions>false</RunOnGithubActions>
<RunOnGithubActions Condition=" '$(RunOnGithubActionsWindows)' == 'true' or '$(RunOnGithubActionsLinux)' == 'true' ">true</RunOnGithubActions>
Copy link
Member

@Youssef1313 Youssef1313 Apr 9, 2025

Choose a reason for hiding this comment

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

Is this logic correct?
Shouldn't RunOnGithubActionsWindows be considered only when on Windows, and RunOnGithubActionsLinux be considered only on Linux, instead of ORing them together without any OS checks?

Currently, it seems like Playground is run on GitHub Actions on Windows (VSTest just ignores when no tests are run, but with my PR for MTP I'm getting zero tests ran for Playground)

Could you please double check @RussKie? Am I missing something here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the logic is correct.
These variables denote the "capabilities":

Project requirements:
- RunOnGithubActions: indicates whether tests should run on GitHub Actions (either Windows or Linux); computed.
- RunOnGithubActionsWindows: indicates whether tests should run on Windows in GitHub Actions; default is true; overridable.
- RunOnGithubActionsLinux: indicates whether tests should run on Linux in GitHub Actions; default is true; overridable.
- RunOnAzdoCI: indicates whether tests should run on Azure DevOps (either Windows or Linux); always false, if RunOnAzdoHelix=true; computed.
- RunOnAzdoCIWindows: indicates whether tests should run on Windows in Azure DevOps; default is true; overridable.
- RunOnAzdoCILinux: indicates whether tests should run on Linux in Azure DevOps; default is true; overridable.
- RunOnAzdoHelix: indicates whether tests should run on Helix (either Windows or Linux); computed.
- RunOnAzdoHelixWindows: indicates whether tests should run on Windows in Helix; default is true; overridable.
- RunOnAzdoHelixLinux: indicates whether tests should run on Linux in Helix; default is true; overridable.

...and used to generate runsheets, which then executed on individual build agents.

You can see how it is being used in https://github.com/dotnet/aspire/pull/8618/files#diff-ce47bb41900ee3082587fce1184e86392a2c25a05882281792f04e8e8abc51f4.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After a third look and two cups of tea later, I see the issue now. Thank you for pointing it out.
SkipTests should be OS-aware, so that we don't run a test on Windows, if the test project declared it couldn't be run on Windows. This is getting fixed in #8687.

@github-actions github-actions bot locked and limited conversation to collaborators May 10, 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.

3 participants