Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,16 @@ protected async Task<PullRequest> CheckForwardFlowGitHubPullRequest(
string targetRepoName,
string targetBranch,
string[] testFiles,
Dictionary<string, string> testFilePatches)
Dictionary<string, string> testFilePatches,
PullRequestCleanupOperation cleanupOperation = PullRequestCleanupOperation.Close)
{
// When we expect updates from multiple repos (batchable subscriptions), we need to wait until the PR gets updated with the second repository after it is created
// Otherwise it might try to validate the contents before all updates are in place
PullRequest pullRequest = repoUpdates.Length > 1
? await WaitForUpdatedPullRequestAsync(targetRepoName, targetBranch)
: await WaitForPullRequestAsync(targetRepoName, targetBranch);

await using (CleanUpPullRequestAfter(TestParameters.GitHubTestOrg, targetRepoName, pullRequest))
await using (CleanUpPullRequestAfter(TestParameters.GitHubTestOrg, targetRepoName, pullRequest, cleanupOperation))
{
IReadOnlyList<PullRequestFile> files = await GitHubApi.PullRequest.Files(
TestParameters.GitHubTestOrg,
Expand Down Expand Up @@ -200,9 +201,10 @@ public async Task<PullRequest> WaitForPullRequestComment(
string targetRepo,
string targetBranch,
string partialComment,
ItemStateFilter prState = ItemStateFilter.Open,
int attempts = 40)
{
PullRequest pullRequest = await WaitForPullRequestAsync(targetRepo, targetBranch);
PullRequest pullRequest = await WaitForPullRequestAsync(targetRepo, targetBranch, prState);

while (attempts-- > 0)
{
Expand Down
56 changes: 51 additions & 5 deletions test/ProductConstructionService.ScenarioTests/ScenarioTestBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,12 @@
using Microsoft.DotNet.DarcLib.Models.AzureDevOps;
using Microsoft.DotNet.DarcLib.Models.Darc;
using Microsoft.DotNet.Internal.Testing.Utility;
using Microsoft.DotNet.ProductConstructionService.Client;
using Microsoft.DotNet.ProductConstructionService.Client.Models;
using Microsoft.Extensions.Logging;
using Newtonsoft.Json.Linq;
using NuGet.Configuration;
using NUnit.Framework;
using Microsoft.DotNet.ProductConstructionService.Client;
using Microsoft.DotNet.ProductConstructionService.Client.Models;
using ProductConstructionService.ScenarioTests.ObjectHelpers;

[assembly: Parallelizable(ParallelScope.Fixtures)]
Expand Down Expand Up @@ -46,7 +46,7 @@ public void BaseSetup()
_packageNameSalt = Guid.NewGuid().ToString().Substring(0, 8);
}

protected async Task<Octokit.PullRequest> WaitForPullRequestAsync(string targetRepo, string targetBranch)
protected async Task<Octokit.PullRequest> WaitForPullRequestAsync(string targetRepo, string targetBranch, Octokit.ItemStateFilter prState = Octokit.ItemStateFilter.Open)
{
Octokit.Repository repo = await GitHubApi.Repository.Get(TestParameters.GitHubTestOrg, targetRepo);

Expand All @@ -56,6 +56,7 @@ public void BaseSetup()
IReadOnlyList<Octokit.PullRequest> prs = await GitHubApi.PullRequest.GetAllForRepository(repo.Id, new Octokit.PullRequestRequest
{
Base = targetBranch,
State = prState
});

if (prs.Count == 1)
Expand Down Expand Up @@ -1189,10 +1190,21 @@ protected string GetUniqueAssetName(string packageName)
return $"{packageName}.{_packageNameSalt}";
}

protected static IAsyncDisposable CleanUpPullRequestAfter(string owner, string repo, Octokit.PullRequest pullRequest)
protected static IAsyncDisposable CleanUpPullRequestAfter(
string owner,
string repo,
Octokit.PullRequest pullRequest,
PullRequestCleanupOperation cleanupOperation = PullRequestCleanupOperation.Close)
=> AsyncDisposable.Create(async () =>
{
await ClosePullRequest(owner, repo, pullRequest);
if (cleanupOperation == PullRequestCleanupOperation.Merge)
{
await MergePullRequest(owner, repo, pullRequest);
}
else
{
await ClosePullRequest(owner, repo, pullRequest);
}
});

protected static async Task ClosePullRequest(string owner, string repo, Octokit.PullRequest pullRequest)
Expand Down Expand Up @@ -1220,6 +1232,34 @@ protected static async Task ClosePullRequest(string owner, string repo, Octokit.
}
}

protected static async Task MergePullRequest(string owner, string repo, Octokit.PullRequest pullRequest)
{
try
{
await GitHubApi.PullRequest.Merge(
owner,
repo,
pullRequest.Number,
new Octokit.MergePullRequest
{
CommitMessage = "Merge pull request",
MergeMethod = Octokit.PullRequestMergeMethod.Merge
});
}
catch
{
// Closed already
}
Comment on lines +1249 to +1252
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.
try
{
await GitHubApi.Git.Reference.Delete(owner, repo, $"heads/{pullRequest.Head.Ref}");
}
catch
{
// branch already deleted
}
}

protected static async Task CreateTargetBranchAndExecuteTest(string targetBranchName, string targetDirectory, Func<Task> test)
{
// first create a new target branch
Expand Down Expand Up @@ -1285,3 +1325,9 @@ protected static async Task WaitForNewCommitInPullRequest(string repo, Octokit.P
return pr;
}
}

internal enum PullRequestCleanupOperation
{
Close,
Merge
}
Original file line number Diff line number Diff line change
Expand Up @@ -98,12 +98,21 @@ await CreateTargetBranchAndExecuteTest(targetBranchName, vmrDirectory.Directory,
TestRepository.VmrTestRepoName,
targetBranchName,
[$"src/{TestRepository.TestRepo1Name}/{TestFile1Name}"],
TestFilePatches);
TestFilePatches,
PullRequestCleanupOperation.Merge);

await CheckIfPullRequestCommentExists(
TestRepository.VmrTestRepoName,
pr,
[$"https://github.com/maestro-auth-test/maestro-test1/pull/{testPrNumber}"]);
// 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
Comment on lines +107 to +113
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.
$"- https://github.com/maestro-auth-test/maestro-test1/pull/{testPrNumber}",
Octokit.ItemStateFilter.Closed);
}
}
}
Expand Down