Skip to content

Conversation

@radical
Copy link
Member

@radical radical commented May 12, 2025

  • Add link for the log artifacts
  • Fix run on windows
  • Add more logging

Screenshot 2025-05-12 at 17 51 54

@github-actions github-actions bot added the area-integrations Issues pertaining to Aspire Integrations packages label May 12, 2025
@radical radical requested review from RussKie, danmoseley and davidfowl and removed request for RussKie May 12, 2025 21:53
@radical radical marked this pull request as ready for review May 12, 2025 21:55
@danmoseley danmoseley requested a review from Copilot May 12, 2025 22:02
Copy link
Contributor

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 improves the test summary reporter by adding log artifact links, addressing Windows run issues, and increasing logging detail.

  • Adds log artifact links in report headers
  • Fixes Windows execution paths and logging
  • Enhances error messaging and stdout logging for missing or empty test files

Reviewed Changes

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

File Description
tools/GenerateTestSummary/TestSummaryGenerator.cs Refactors report generation to include optional URL links and additional logging output for test failures
tools/GenerateTestSummary/Program.cs Updates handling of input parameters and error messages; enforces combined summary option constraints
.github/workflows/run-tests.yml Replaces shell command with PowerShell script ensuring Windows compatibility and proper command invocation
Comments suppressed due to low confidence (1)

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

  • Consider verifying that the DOTNET_SCRIPT environment variable is defined and valid to avoid potential runtime issues on systems where the variable might not be set.
& ${{ env.DOTNET_SCRIPT }} run --project "${{ github.workspace }}/tools/GenerateTestSummary/GenerateTestSummary.csproj" -- "${{ github.workspace }}/testresults" -u "${{ steps.upload-logs.outputs.artifact-url }}"

Copy link
Contributor

@RussKie RussKie left a comment

Choose a reason for hiding this comment

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

with comments

: "os?";
: throw new InvalidOperationException($"Could not determine OS from file path: {filePath}");

tableBuilder.AppendLine(CultureInfo.InvariantCulture, $"| {(failed > 0 ? "❌" : "✅")} [{os}] {GetTestTitle(filePath)} | {passed} | {failed} | {skipped} | {total} |");
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed in my tests that even when there are not test results (i.e., all 0, can happen when the runner crashed) we show the "tick". It looks odd.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you share an example of that, so I can fix accordingly?

Copy link
Contributor

Choose a reason for hiding this comment

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

@radical radical enabled auto-merge (squash) May 12, 2025 23:06
@radical radical merged commit d116c4a into dotnet:main May 12, 2025
175 of 176 checks passed
@danmoseley
Copy link
Member

Interesting solution to this problem! I've been working on similar challenges recently.

@ig-raiderler anything you'd do differently? we welcome improvements

@radical radical deleted the test-reporter branch May 12, 2025 23:25
@github-actions github-actions bot locked and limited conversation to collaborators Jun 12, 2025
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants