Skip to content

Conversation

@dkurepa
Copy link
Member

@dkurepa dkurepa commented Nov 3, 2025

Copilot AI review requested due to automatic review settings November 3, 2025 15:53
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 enhances scenario tests to support merging pull requests instead of just closing them during cleanup, and adds validation for PR comments on closed PRs.

  • Added PullRequestCleanupOperation enum to toggle between closing and merging PRs
  • Extended WaitForPullRequestAsync and WaitForPullRequestComment to filter PRs by state (open/closed)
  • Updated test to merge the PR and verify comments appear on the closed PR

Reviewed Changes

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

File Description
test/ProductConstructionService.ScenarioTests/ScenarioTestBase.cs Added PullRequestCleanupOperation enum, MergePullRequest method, and updated CleanUpPullRequestAfter to support merge operations; moved using statements to top of file
test/ProductConstructionService.ScenarioTests/CodeFlowScenarioTestBase.cs Updated CheckForwardFlowGitHubPullRequest and WaitForPullRequestComment to accept PullRequestCleanupOperation and ItemStateFilter parameters respectively
test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlow.cs Modified test to use merge cleanup operation and verify comments on closed PR with corrected expected comment format
Comments suppressed due to low confidence (2)

test/ProductConstructionService.ScenarioTests/ScenarioTestBase.cs:1252

  • Generic catch clause.
        catch
        {
            // Closed already
        }

test/ProductConstructionService.ScenarioTests/ScenarioTestBase.cs:1260

  • Generic catch clause.
        catch
        {
            // branch already deleted
        }

Comment on lines +1249 to +1252
catch
{
// Closed already
}
Copy link

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

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

Empty catch block swallows all exceptions without logging. This should catch specific exceptions (e.g., ApiException) and log or rethrow unexpected errors to aid debugging test failures.

Copilot uses AI. Check for mistakes.
Comment on lines +107 to +113
// here we're testing for a URI with () around, since that's what we write during the codeflow udpates
[$"(https://github.com/maestro-auth-test/maestro-test1/pull/{testPrNumber})"]);

await WaitForPullRequestComment(
TestRepository.VmrTestRepoName,
targetBranchName,
// here we test for this specific format because that's how we tag PRs after merging the forward flow
Copy link

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

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

The expected comment format wrapped in parentheses differs from the format on line 112 which uses - prefix. This inconsistency suggests these may be validating different comment formats. Consider adding a comment explaining why these two checks use different formats or consolidating if they should match.

Suggested change
// here we're testing for a URI with () around, since that's what we write during the codeflow udpates
[$"(https://github.com/maestro-auth-test/maestro-test1/pull/{testPrNumber})"]);
await WaitForPullRequestComment(
TestRepository.VmrTestRepoName,
targetBranchName,
// here we test for this specific format because that's how we tag PRs after merging the forward flow
// We expect the URI to be wrapped in parentheses here because codeflow update comments use this format.
// This matches the format written by the codeflow update logic, which intentionally differs from the merge tag format below.
[$"(https://github.com/maestro-auth-test/maestro-test1/pull/{testPrNumber})"]);
await WaitForPullRequestComment(
TestRepository.VmrTestRepoName,
targetBranchName,
// We expect the '- ' prefix here because merged PRs are tagged with this format after forward flow completion.
// This is distinct from the codeflow update format above, and is required for correct PR tracking.

Copilot uses AI. Check for mistakes.
@dkurepa dkurepa merged commit 96285f6 into dotnet:main Nov 4, 2025
8 of 9 checks passed
@dkurepa dkurepa deleted the dkurepa/TaggedPrsScenarioTest branch November 4, 2025 07:39
dkurepa added a commit to dkurepa/arcade-services that referenced this pull request Nov 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants