diff --git a/src/Microsoft.DotNet.Darc/DarcLib/AzureDevOpsClient.cs b/src/Microsoft.DotNet.Darc/DarcLib/AzureDevOpsClient.cs index eb4efe42a9..25e32a8a36 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/AzureDevOpsClient.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/AzureDevOpsClient.cs @@ -1560,4 +1560,24 @@ private static string TruncateDescriptionIfNeeded(string str) } return str; } + + public async Task CommentPullRequestAsync(string pullRequestUri, string comment) + { + (string accountName, string _, string repoName, int id) = ParsePullRequestUri(pullRequestUri); + + using VssConnection connection = CreateVssConnection(accountName); + using GitHttpClient client = await connection.GetClientAsync(); + + var prComment = new Comment() + { + CommentType = CommentType.Text, + Content = $"{comment}{CommentMarker}" + }; + + var newCommentThread = new GitPullRequestCommentThread() + { + Comments = [prComment] + }; + await client.CreateThreadAsync(newCommentThread, repoName, id); + } } diff --git a/src/Microsoft.DotNet.Darc/DarcLib/GitHubClient.cs b/src/Microsoft.DotNet.Darc/DarcLib/GitHubClient.cs index 1e73333b04..0c70d28e26 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/GitHubClient.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/GitHubClient.cs @@ -1309,4 +1309,10 @@ public async Task DeletePullRequestBranchAsync(string pullRequestUri) (string owner, string repo, int id) prInfo = ParsePullRequestUri(pullRequestUri); await DeleteBranchAsync(prInfo.owner, prInfo.repo, pr.HeadBranch); } + + public async Task CommentPullRequestAsync(string pullRequestUri, string comment) + { + (string owner, string repo, int id) = ParsePullRequestUri(pullRequestUri); + await GetClient(owner, repo).Issue.Comment.Create(owner, repo, id, comment); + } } diff --git a/src/Microsoft.DotNet.Darc/DarcLib/IRemote.cs b/src/Microsoft.DotNet.Darc/DarcLib/IRemote.cs index e4253d314b..0e29b3563f 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/IRemote.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/IRemote.cs @@ -188,5 +188,13 @@ Task> CommitUpdatesAsync( /// Location for the .git directory, or null for default Task CloneAsync(string repoUri, string commit, string targetDirectory, bool checkoutSubmodules, string gitDirectory); + /// + /// Comment on an existing pull request + /// + /// Uri of the pull request + /// Comment message + /// + Task CommentPullRequestAsync(string pullRequestUri, string comment); + #endregion } diff --git a/src/Microsoft.DotNet.Darc/DarcLib/IRemoteGitRepo.cs b/src/Microsoft.DotNet.Darc/DarcLib/IRemoteGitRepo.cs index d61fb75113..b9eff6d282 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/IRemoteGitRepo.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/IRemoteGitRepo.cs @@ -108,7 +108,7 @@ Task> SearchPullRequestsAsync( /// or remove each merge policy check that isn't in evaluations /// /// Url of pull request - /// List of merge policies + /// List of merge policies Task CreateOrUpdatePullRequestMergeStatusInfoAsync(string pullRequestUrl, IReadOnlyList evaluations); /// @@ -168,6 +168,13 @@ Task> SearchPullRequestsAsync( /// Repository to find the branch in /// Branch to find Task DoesBranchExistAsync(string repoUri, string branch); + + /// + /// Comment on an existing pull request + /// + /// Uri of the pull request + /// Comment message + Task CommentPullRequestAsync(string pullRequestUri, string comment); } #nullable disable diff --git a/src/Microsoft.DotNet.Darc/DarcLib/Remote.cs b/src/Microsoft.DotNet.Darc/DarcLib/Remote.cs index 8501a546d6..0d968a4687 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/Remote.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/Remote.cs @@ -405,4 +405,10 @@ public async Task> GetCommonScriptFilesAsync(string repoUri, strin return files; } + + public async Task CommentPullRequestAsync(string pullRequestUri, string comment) + { + CheckForValidGitClient(); + await _remoteGitClient.CommentPullRequestAsync(pullRequestUri, comment); + } } diff --git a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/Exceptions.cs b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/Exceptions.cs index 06bfb74d45..10a4247867 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/Exceptions.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/Exceptions.cs @@ -2,7 +2,10 @@ // The .NET Foundation licenses this file to you under the MIT license. using System; +using System.Collections.Generic; using System.IO; +using System.Linq; +using System.Text.RegularExpressions; using Microsoft.DotNet.DarcLib.Helpers; #nullable enable @@ -24,8 +27,50 @@ private static string GetExceptionMessage(VmrIngestionPatch patch, ProcessExecut + result; } -public class ConflictInPrBranchException(VmrIngestionPatch patch, string targetBranch) +/// +/// Exception thrown when the service can't apply an update to the PR branch due to a conflict +/// between the source repo and a change that was made in the PR after it was opened. +/// +public class ConflictInPrBranchException(ProcessExecutionResult gitMergeResult, string targetBranch, bool isForwardFlow) : Exception($"Failed to flow changes due to conflicts in the target branch ({targetBranch})") { - public VmrIngestionPatch Patch { get; } = patch; + public List FilesInConflict { get; } = ParseResult(gitMergeResult, isForwardFlow); + + private static readonly Regex AlreadyExistsRegex = new("patch failed: (.+): already exist in index"); + private static readonly Regex PatchFailedRegex = new("error: patch failed: (.*):"); + private static readonly Regex PatchDoesNotApplyRegex = new("error: (.+): patch does not apply"); + private static readonly Regex FileDoesNotExistRegex = new("error: (.+): does not exist in index"); + + private static readonly Regex[] ConflictRegex = + [ + AlreadyExistsRegex, + PatchFailedRegex, + PatchDoesNotApplyRegex, + FileDoesNotExistRegex + ]; + + private static List ParseResult(ProcessExecutionResult gitMergeResult, bool isForwardFlow) + { + List filesInConflict = new(); + var errors = gitMergeResult.StandardError.Split(Environment.NewLine); + foreach (var error in errors) + { + foreach (var regex in ConflictRegex) + { + var match = regex.Match(error); + if (match.Success) + { + filesInConflict.Add(match.Groups[1].Value); + break; + } + } + } + if (isForwardFlow) + { + // Convert VMR paths to normal repo paths, for example src/repo/file.cs -> file.cs + return filesInConflict.Select(file => file.Split('/', 3)[2]).Distinct().ToList(); + } + // If we're backflowing, the file paths are already normal + return filesInConflict; + } } diff --git a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrBackflower.cs b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrBackflower.cs index 6a32cdcf0a..233967c569 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrBackflower.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrBackflower.cs @@ -251,7 +251,7 @@ protected override async Task SameDirectionFlowAsync( if (!rebaseConflicts) { _logger.LogInformation("Failed to update a PR branch because of a conflict. Stopping the flow.."); - throw new ConflictInPrBranchException(e.Patch, headBranch); + throw new ConflictInPrBranchException(e.Result, targetBranch, isForwardFlow: false); } // Otherwise, we have a conflicting change in the last backflow PR (before merging) diff --git a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrForwardFlower.cs b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrForwardFlower.cs index 1aa4b1c3ca..e62a8756f2 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrForwardFlower.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrForwardFlower.cs @@ -217,7 +217,7 @@ protected override async Task SameDirectionFlowAsync( if (!rebaseConflicts) { _logger.LogInformation("Failed to update a PR branch because of a conflict. Stopping the flow.."); - throw new ConflictInPrBranchException(e.Patch, headBranch); + throw new ConflictInPrBranchException(e.Result, targetBranch, isForwardFlow: true); } // This happens when a conflicting change was made in the last backflow PR (before merging) diff --git a/src/ProductConstructionService/ProductConstructionService.Common/RedisCache.cs b/src/ProductConstructionService/ProductConstructionService.Common/RedisCache.cs index fe7c17e005..3ad02e7129 100644 --- a/src/ProductConstructionService/ProductConstructionService.Common/RedisCache.cs +++ b/src/ProductConstructionService/ProductConstructionService.Common/RedisCache.cs @@ -13,7 +13,7 @@ public interface IRedisCache Task GetAsync(string key); Task GetAsync(); Task SetAsync(string value, TimeSpan? expiration = null); - Task TryDeleteAsync(); + Task TryDeleteAsync(); Task TryGetAsync(); IAsyncEnumerable GetKeysAsync(string pattern); } @@ -44,9 +44,9 @@ public async Task SetAsync(string value, TimeSpan? expiration = null) return value.HasValue ? value.ToString() : null; } - public async Task TryDeleteAsync() + public async Task TryDeleteAsync() { - await Cache.KeyDeleteAsync(_stateKey); + return await Cache.KeyDeleteAsync(_stateKey); } public async Task GetAsync() diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/InProgressPullRequest.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/InProgressPullRequest.cs index 67950cdb91..b837db7443 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/InProgressPullRequest.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/InProgressPullRequest.cs @@ -50,4 +50,13 @@ public class InProgressPullRequest : DependencyFlowWorkItem, IPullRequest [DataMember] public DateTime? NextCheck { get; set; } + + [DataMember] + public InProgressPullRequestState MergeState { get; set; } +} + +public enum InProgressPullRequestState +{ + Mergeable, + Conflict } diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs index 0faaa0422e..535d19b557 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs @@ -1,6 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Text; using Maestro.Data.Models; using Maestro.MergePolicies; using Maestro.MergePolicyEvaluation; @@ -26,7 +27,7 @@ namespace ProductConstructionService.DependencyFlow; internal abstract class PullRequestUpdater : IPullRequestUpdater { #if DEBUG - private static readonly TimeSpan DefaultReminderDelay = TimeSpan.FromMinutes(3); + private static readonly TimeSpan DefaultReminderDelay = TimeSpan.FromMinutes(1); #else private static readonly TimeSpan DefaultReminderDelay = TimeSpan.FromMinutes(5); #endif @@ -138,7 +139,6 @@ public async Task UpdateAssetsAsync( public async Task ProcessPendingUpdatesAsync(SubscriptionUpdateWorkItem update) { _logger.LogInformation("Processing pending updates for subscription {subscriptionId}", update.SubscriptionId); - // Check if we track an on-going PR already InProgressPullRequest? pr = await _pullRequestState.TryGetStateAsync(); bool isCodeFlow = update.SubscriptionType == SubscriptionType.DependenciesAndSources; @@ -170,7 +170,7 @@ public async Task ProcessPendingUpdatesAsync(SubscriptionUpdateWorkItem up } } - // Code flow updates are handled separetely + // Code flow updates are handled separately if (isCodeFlow) { return await ProcessCodeFlowUpdateAsync(update, pr); @@ -206,8 +206,8 @@ public async Task CheckPullRequestAsync(PullRequestCheck pullRequestCheck) if (inProgressPr == null) { _logger.LogInformation("No in-progress pull request found for a PR check"); - await ClearAllStateAsync(isCodeFlow: true); - await ClearAllStateAsync(isCodeFlow: false); + await ClearAllStateAsync(isCodeFlow: true, clearPendingUpdates: true); + await ClearAllStateAsync(isCodeFlow: false, clearPendingUpdates: true); return false; } @@ -232,7 +232,7 @@ protected virtual Task TagSourceRepositoryGitHubContactsIfPossibleAsync(InProgre _logger.LogInformation("Querying status for pull request {prUrl}", pr.Url); (var targetRepository, _) = await GetTargetAsync(); - IRemote remote = await _remoteFactory.CreateRemoteAsync(targetRepository); + var remote = await _remoteFactory.CreateRemoteAsync(targetRepository); PrStatus status; try @@ -267,7 +267,8 @@ await AddDependencyFlowEventsAsync( mergePolicyResult, pr.Url); - await ClearAllStateAsync(isCodeFlow); + // If the PR we just merged was in conflict with an update we previously tried to apply, we shouldn't delete the reminder for the update + await ClearAllStateAsync(isCodeFlow, clearPendingUpdates: pr.MergeState == InProgressPullRequestState.Mergeable); return PullRequestStatus.Completed; case MergePolicyCheckResult.FailedPolicies: @@ -277,6 +278,17 @@ await AddDependencyFlowEventsAsync( case MergePolicyCheckResult.NoPolicies: case MergePolicyCheckResult.FailedToMerge: _logger.LogInformation("Pull request {url} still active (updatable) - keeping tracking it", pr.Url); + // Check if we think the PR has a conflict + if (pr.MergeState == InProgressPullRequestState.Conflict) + { + // If we think so, check if the PR head branch still has the same commit as the one we remembered. + // If it doesn't, we should try to update the PR again, the conflicts might be resolved + var latestCommit = await remote.GetLatestCommitAsync(targetRepository, pr.HeadBranch); + if (latestCommit == pr.SourceSha) + { + return PullRequestStatus.InProgressCannotUpdate; + } + } await SetPullRequestCheckReminder(pr, isCodeFlow); return PullRequestStatus.InProgressCanUpdate; @@ -311,7 +323,7 @@ await AddDependencyFlowEventsAsync( _logger.LogInformation("PR {url} has been manually {action}. Stopping tracking it", pr.Url, status.ToString().ToLowerInvariant()); - await ClearAllStateAsync(isCodeFlow); + await ClearAllStateAsync(isCodeFlow, clearPendingUpdates: pr.MergeState == InProgressPullRequestState.Mergeable); // Also try to clean up the PR branch. try @@ -800,11 +812,16 @@ private async Task SetPullRequestCheckReminder(InProgressPullRequest prState, bo await _pullRequestState.SetAsync(prState); } - private async Task ClearAllStateAsync(bool isCodeFlow) + private async Task ClearAllStateAsync(bool isCodeFlow, bool clearPendingUpdates) { await _pullRequestState.TryDeleteAsync(); await _pullRequestCheckReminders.UnsetReminderAsync(isCodeFlow); - await _pullRequestUpdateReminders.UnsetReminderAsync(isCodeFlow); + // If the pull request we deleted from the cache had a conflict, we shouldn't unset the update reminder + // as there was an update that was previously blocked + if (!clearPendingUpdates) + { + await _pullRequestUpdateReminders.UnsetReminderAsync(isCodeFlow); + } } /// @@ -905,6 +922,13 @@ await CreateCodeFlowPullRequestAsync( { await UpdateAssetsAndSources(update, pr); } + catch (ConflictInPrBranchException conflictException) + { + return await HandlePrUpdateConflictAsync( + conflictException, + update, + pr); + } catch (Exception e) { // TODO https://github.com/dotnet/arcade-services/issues/4198: Notify us about these kind of failures @@ -962,6 +986,10 @@ private async Task UpdateAssetsAndSources(SubscriptionUpdateWorkItem updat cancellationToken: default); } } + catch (Exception e) when (e is ConflictInPrBranchException) + { + throw; + } catch (Exception e) { _logger.LogError(e, "Failed to flow changes for build {buildId} in subscription {subscriptionId}", @@ -1016,6 +1044,7 @@ private async Task UpdateAssetsAndSources(SubscriptionUpdateWorkItem updat }); pullRequest.LastUpdate = DateTime.UtcNow; + pullRequest.MergeState = InProgressPullRequestState.Mergeable; await SetPullRequestCheckReminder(pullRequest, true); await _pullRequestUpdateReminders.UnsetReminderAsync(true); @@ -1171,5 +1200,54 @@ await AddDependencyFlowEventsAsync( } } + private async Task HandlePrUpdateConflictAsync( + ConflictInPrBranchException conflictException, + SubscriptionUpdateWorkItem update, + InProgressPullRequest pr) + { + // The PR we're trying to update has a conflict with the source repo. We will mark it as blocked, not allowing any updates from this + // subscription till it's merged, or the conflict resolved. We'll set a reminder to check on it. + StringBuilder sb = new(); + sb.AppendLine($"There was a conflict in the PR branch when flowing source from {update.GetRepoAtCommitUri()}"); + sb.AppendLine("Files conflicting with the head branch:"); + foreach (var file in conflictException.FilesInConflict) + { + sb.AppendLine($" - [{file}]({update.GetFileAtCommitUri(file)})"); + } + sb.AppendLine(); + sb.AppendLine("Updates from this subscription will be blocked until the conflict is resolved, or the PR is merged"); + + (var targetRepository, _) = await GetTargetAsync(); + var remote = await _remoteFactory.CreateRemoteAsync(targetRepository); + + try + { + await remote.CommentPullRequestAsync(pr.Url, sb.ToString()); + } + catch (Exception e) + { + _logger.LogWarning("Posting comment to {prUrl} failed with exception {message}", pr.Url, e.Message); + } + // If the headBranch gets updated, we will retry to update it with previously conflicting changes. If these changes still cause a conflict, we should update the + // InProgressPullRequest with the latest commit from the remote branch + var remoteCommit = pr.SourceSha; + try + { + remoteCommit = await remote.GetLatestCommitAsync(targetRepository, pr.HeadBranch); + } + catch (Exception e) + { + _logger.LogWarning("Couldn't get latest commit of {repo}/{commit}. Failed with exception {message}", targetRepository, pr.HeadBranch, e.Message); + } + + pr.MergeState = InProgressPullRequestState.Conflict; + pr.SourceSha = remoteCommit; + await _pullRequestState.SetAsync(pr); + await _pullRequestUpdateReminders.SetReminderAsync(update, DefaultReminderDelay, isCodeFlow: true); + await _pullRequestCheckReminders.UnsetReminderAsync(isCodeFlow: true); + + return true; + } + #endregion } diff --git a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItems/SubscriptionUpdateWorkItem.cs b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItems/SubscriptionUpdateWorkItem.cs index 720518e9e4..fa383a197c 100644 --- a/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItems/SubscriptionUpdateWorkItem.cs +++ b/src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItems/SubscriptionUpdateWorkItem.cs @@ -35,3 +35,16 @@ public class SubscriptionUpdateWorkItem : DependencyFlowWorkItem [DataMember] public bool IsCoherencyUpdate { get; init; } } + +public static class SubscriptionUpdateWorkItemExtensions +{ + public static string GetRepoAtCommitUri(this SubscriptionUpdateWorkItem update) => + update.SourceRepo.Contains("github.com") + ? $"{update.SourceRepo}/tree/{update.SourceSha}" + : $"{update.SourceRepo}?version=GC{update.SourceSha}"; + + public static string GetFileAtCommitUri(this SubscriptionUpdateWorkItem update, string filePath) => + update.SourceRepo.Contains("github.com") + ? $"{update.SourceRepo}/blob/{update.SourceSha}/{filePath}" + : $"{update.SourceRepo}?version=GC{update.SourceSha}&path={filePath}"; +} diff --git a/test/ProductConstructionService.DependencyFlow.Tests/MockRedisCache.cs b/test/ProductConstructionService.DependencyFlow.Tests/MockRedisCache.cs index d91fe3fe34..2997b6af53 100644 --- a/test/ProductConstructionService.DependencyFlow.Tests/MockRedisCache.cs +++ b/test/ProductConstructionService.DependencyFlow.Tests/MockRedisCache.cs @@ -36,11 +36,11 @@ public Task SetAsync(string value, TimeSpan? expiration = null) return Task.CompletedTask; } - public Task TryDeleteAsync() + public Task TryDeleteAsync() { _data.Remove(_key); - return Task.CompletedTask; + return Task.FromResult(true); } public Task TryGetAsync() => throw new NotImplementedException(); diff --git a/test/ProductConstructionService.DependencyFlow.Tests/PendingCodeFlowUpdatesTests.cs b/test/ProductConstructionService.DependencyFlow.Tests/PendingCodeFlowUpdatesTests.cs index 37870d5c51..9c34540710 100644 --- a/test/ProductConstructionService.DependencyFlow.Tests/PendingCodeFlowUpdatesTests.cs +++ b/test/ProductConstructionService.DependencyFlow.Tests/PendingCodeFlowUpdatesTests.cs @@ -77,4 +77,81 @@ public async Task PendingUpdatesUpdatablePr() AndShouldHaveInProgressPullRequestState(newBuild); } } + + [Test] + public async Task PendingUpdatesInConflictWithCurrent() + { + GivenATestChannel(); + GivenACodeFlowSubscription( + new SubscriptionPolicy + { + Batchable = false, + UpdateFrequency = UpdateFrequency.EveryBuild + }); + Build oldBuild = GivenANewBuild(true); + Build newBuild = GivenANewBuild(true); + newBuild.Commit = "sha456"; + + using (WithExistingCodeFlowPullRequest(oldBuild, canUpdate: true, newChangeWillConflict: true)) + { + await WhenProcessPendingUpdatesAsyncIsCalled(newBuild, isCodeFlow: true); + + ThenShouldHavePendingUpdateState(newBuild, isCodeFlow: true); + AndShouldNotHavePullRequestCheckReminder(); + AndShouldHaveInProgressPullRequestState( + oldBuild, + overwriteBuildCommit: ConflictPRRemoteSha, + prState: InProgressPullRequestState.Conflict); + } + } + + [Test] + public async Task PendingUpdateNotUpdatablePrAlreadyInConflict() + { + GivenATestChannel(); + GivenACodeFlowSubscription( + new SubscriptionPolicy + { + Batchable = false, + UpdateFrequency = UpdateFrequency.EveryBuild + }); + Build build = GivenANewBuild(true); + + using (WithExistingCodeFlowPullRequest(build, canUpdate: true, prAlreadyHasConflict: true)) + { + await WhenProcessPendingUpdatesAsyncIsCalled(build, isCodeFlow: true); + + ThenShouldHavePendingUpdateState(build, isCodeFlow: true); + AndShouldNotHavePullRequestCheckReminder(); + AndShouldHaveInProgressPullRequestState( + build, + overwriteBuildCommit: ConflictPRRemoteSha, + prState: InProgressPullRequestState.Conflict); + } + } + + + [Test] + public async Task PendingUpdateUpdatableConflictInPrResolved() + { + GivenATestChannel(); + GivenACodeFlowSubscription( + new SubscriptionPolicy + { + Batchable = false, + UpdateFrequency = UpdateFrequency.EveryBuild + }); + Build oldBuild = GivenANewBuild(true); + Build newBuild = GivenANewBuild(true); + newBuild.Commit = "sha456"; + using (WithExistingCodeFlowPullRequest(oldBuild, canUpdate: true, prAlreadyHasConflict: true, latestCommitToReturn: "sha4")) + { + ExpectPrMetadataToBeUpdated(); + await WhenProcessPendingUpdatesAsyncIsCalled(newBuild, isCodeFlow: true); + ThenCodeShouldHaveBeenFlownForward(newBuild); + AndShouldHaveNoPendingUpdateState(); + AndShouldHavePullRequestCheckReminder(); + AndShouldHaveInProgressPullRequestState(newBuild); + } + } } diff --git a/test/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs b/test/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs index da66914eb7..b7f6785e5e 100644 --- a/test/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs +++ b/test/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs @@ -1,6 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Security.Cryptography; using FluentAssertions; using Maestro.Data; using Maestro.Data.Models; @@ -28,6 +29,7 @@ internal abstract class PullRequestUpdaterTests : SubscriptionOrPullRequestUpdat private const long InstallationId = 1174; protected const string InProgressPrUrl = "https://github.com/owner/repo/pull/10"; protected string? InProgressPrHeadBranch { get; private set; } = "pr.head.branch"; + protected const string ConflictPRRemoteSha = "sha3"; private Mock _backFlower = null!; private Mock _forwardFlower = null!; @@ -390,12 +392,35 @@ protected IDisposable WithExistingPullRequest(Build forBuild, PrStatus prStatus, return Disposable.Create(remote.VerifyAll); } - protected IDisposable WithExistingCodeFlowPullRequest(Build forBuild, bool canUpdate) + protected IDisposable WithExistingCodeFlowPullRequest( + Build forBuild, + bool canUpdate, + bool newChangeWillConflict = false, + bool prAlreadyHasConflict = false, + string latestCommitToReturn = ConflictPRRemoteSha) => canUpdate - ? WithExistingCodeFlowPullRequest(forBuild, PrStatus.Open, null) - : WithExistingCodeFlowPullRequest(forBuild, PrStatus.Open, MergePolicyEvaluationStatus.Pending); - - protected IDisposable WithExistingCodeFlowPullRequest(Build forBuild, PrStatus prStatus, MergePolicyEvaluationStatus? policyEvaluationStatus) + ? WithExistingCodeFlowPullRequest( + forBuild, + PrStatus.Open, + null, + newChangeWillConflict, + prAlreadyHasConflict, + latestCommitToReturn) + : WithExistingCodeFlowPullRequest( + forBuild, + PrStatus.Open, + MergePolicyEvaluationStatus.Pending, + newChangeWillConflict, + prAlreadyHasConflict, + latestCommitToReturn); + + protected IDisposable WithExistingCodeFlowPullRequest( + Build forBuild, + PrStatus prStatus, + MergePolicyEvaluationStatus? policyEvaluationStatus, + bool flowerWillHaveConflict = false, + bool prAlreadyHasConflict = false, + string latestCommitToReturn = ConflictPRRemoteSha) { var prUrl = Subscription.TargetDirectory != null ? VmrPullRequestUrl @@ -407,7 +432,17 @@ protected IDisposable WithExistingCodeFlowPullRequest(Build forBuild, PrStatus p AfterDbUpdateActions.Add(() => { - var pr = CreatePullRequestState(forBuild, prUrl); + var pr = CreatePullRequestState( + forBuild, + prUrl, + overwriteBuildCommit: + prAlreadyHasConflict + ? ConflictPRRemoteSha + : forBuild.Commit, + prState: + prAlreadyHasConflict + ? InProgressPullRequestState.Conflict + : InProgressPullRequestState.Mergeable); SetState(Subscription, pr); SetExpectedState(Subscription, pr); }); @@ -442,6 +477,29 @@ protected IDisposable WithExistingCodeFlowPullRequest(Build forBuild, PrStatus p .Returns(Task.CompletedTask); } + if (flowerWillHaveConflict) + { + remote + .Setup(x => x.CommentPullRequestAsync(It.IsAny(), It.IsAny())) + .Returns(Task.CompletedTask); + + ProcessExecutionResult gitMergeResult = new(); + _forwardFlower.Setup(x => x.FlowForwardAsync( + It.IsAny(), + It.IsAny(), + It.IsAny(), + It.IsAny())) + .Throws(() => new ConflictInPrBranchException(gitMergeResult, "branch", true)); + + } + + if (flowerWillHaveConflict || prAlreadyHasConflict) + { + remote + .Setup(x => x.GetLatestCommitAsync(It.IsAny(), It.IsAny())) + .ReturnsAsync(latestCommitToReturn); + } + return Disposable.Create(remote.VerifyAll); } @@ -459,17 +517,33 @@ protected void AndShouldHavePullRequestCheckReminder() }); } + protected void AndShouldNotHavePullRequestCheckReminder() + { + RemoveExpectedReminder(Subscription); + } + protected void AndShouldHaveInProgressPullRequestState( Build forBuild, bool? coherencyCheckSuccessful = true, List? coherencyErrors = null, - InProgressPullRequest? expectedState = null) + InProgressPullRequest? expectedState = null, + string? overwriteBuildCommit = null, + InProgressPullRequestState prState = InProgressPullRequestState.Mergeable) { var prUrl = Subscription.SourceEnabled ? VmrPullRequestUrl : InProgressPrUrl; - SetExpectedState(Subscription, expectedState ?? CreatePullRequestState(forBuild, prUrl, coherencyCheckSuccessful, coherencyErrors)); + SetExpectedState( + Subscription, + expectedState + ?? CreatePullRequestState( + forBuild, + prUrl, + coherencyCheckSuccessful, + coherencyErrors, + overwriteBuildCommit, + prState)); } protected void ThenShouldHaveInProgressPullRequestState(Build forBuild, InProgressPullRequest? expectedState = null) @@ -524,12 +598,14 @@ protected InProgressPullRequest CreatePullRequestState( Build forBuild, string prUrl, bool? coherencyCheckSuccessful = true, - List? coherencyErrors = null) + List? coherencyErrors = null, + string? overwriteBuildCommit = null, + InProgressPullRequestState prState = InProgressPullRequestState.Mergeable) => new() { UpdaterId = GetPullRequestUpdaterId().ToString(), HeadBranch = InProgressPrHeadBranch, - SourceSha = forBuild.Commit, + SourceSha = overwriteBuildCommit ?? forBuild.Commit, ContainedSubscriptions = [ new SubscriptionPullRequestUpdate @@ -549,5 +625,6 @@ protected InProgressPullRequest CreatePullRequestState( CoherencyCheckSuccessful = coherencyCheckSuccessful, CoherencyErrors = coherencyErrors, Url = prUrl, + MergeState = prState }; } diff --git a/test/ProductConstructionService.DependencyFlow.Tests/UpdateAssetsForCodeFlowTests.cs b/test/ProductConstructionService.DependencyFlow.Tests/UpdateAssetsForCodeFlowTests.cs index 4058b4b378..279996518f 100644 --- a/test/ProductConstructionService.DependencyFlow.Tests/UpdateAssetsForCodeFlowTests.cs +++ b/test/ProductConstructionService.DependencyFlow.Tests/UpdateAssetsForCodeFlowTests.cs @@ -9,7 +9,7 @@ namespace ProductConstructionService.DependencyFlow.Tests; /// /// Tests the code flow PR update logic. -/// The tests are writter in the order in which the different phases of the PR are written. +/// The tests are writer in the order in which the different phases of the PR are written. /// Each test should have the inner state that is left behind by the previous state. /// [TestFixture, NonParallelizable] diff --git a/test/ProductConstructionService.ScenarioTests/CodeFlowScenarioTestBase.cs b/test/ProductConstructionService.ScenarioTests/CodeFlowScenarioTestBase.cs index 579519ae45..2095bd098b 100644 --- a/test/ProductConstructionService.ScenarioTests/CodeFlowScenarioTestBase.cs +++ b/test/ProductConstructionService.ScenarioTests/CodeFlowScenarioTestBase.cs @@ -182,4 +182,41 @@ private static async Task> CreateSourceEnabledSubsc targetIsAzDo, trigger); } + + public async Task WaitForPullRequestComment( + string targetRepo, + string targetBranch, + string partialComment, + int attempts = 40) + { + PullRequest pullRequest = await WaitForPullRequestAsync(targetRepo, targetBranch); + + while (attempts-- > 0) + { + pullRequest = await GitHubApi.PullRequest.Get(TestParameters.GitHubTestOrg, targetRepo, pullRequest.Number); + IReadOnlyList comments = await GitHubApi.Issue.Comment.GetAllForIssue(TestParameters.GitHubTestOrg, targetRepo, pullRequest.Number); + if (comments.Any(comment => comment.Body.Contains(partialComment))) + { + return pullRequest; + } + await Task.Delay(TimeSpan.FromSeconds(5)); + } + + throw new ScenarioTestException($"Comment containing '{partialComment}' was not found in the pull request."); + } + + public async Task CheckIfPullRequestCommentExists( + string targetRepo, + string targetBranch, + PullRequest pullRequest, + string[] filesInConflict) + { + IReadOnlyList comments = await GitHubApi.Issue.Comment.GetAllForIssue(TestParameters.GitHubTestOrg, targetRepo, pullRequest.Number); + var conflictComment = comments.First(comment => comment.Body.Contains("conflict")); + + foreach (var file in filesInConflict) + { + conflictComment.Body.Should().Contain(file); + } + } } diff --git a/test/ProductConstructionService.ScenarioTests/ScenarioTestBase.cs b/test/ProductConstructionService.ScenarioTests/ScenarioTestBase.cs index 897896f6cc..e6cf349fe5 100644 --- a/test/ProductConstructionService.ScenarioTests/ScenarioTestBase.cs +++ b/test/ProductConstructionService.ScenarioTests/ScenarioTestBase.cs @@ -1036,26 +1036,38 @@ protected string GetUniqueAssetName(string packageName) protected static IAsyncDisposable CleanUpPullRequestAfter(string owner, string repo, Octokit.PullRequest pullRequest) => AsyncDisposable.Create(async () => { - try - { - var pullRequestUpdate = new Octokit.PullRequestUpdate - { - State = Octokit.ItemState.Closed - }; + await ClosePullRequest(owner, repo, pullRequest); + }); - await GitHubApi.Repository.PullRequest.Update(owner, repo, pullRequest.Number, pullRequestUpdate); - await GitHubApi.Git.Reference.Delete(owner, repo, $"heads/{pullRequest.Head.Ref}"); - } - catch + protected static async Task ClosePullRequest(string owner, string repo, Octokit.PullRequest pullRequest) + { + try + { + var pullRequestUpdate = new Octokit.PullRequestUpdate { - // Closed already - } - }); + State = Octokit.ItemState.Closed + }; + + await GitHubApi.Repository.PullRequest.Update(owner, repo, pullRequest.Number, pullRequestUpdate); + } + catch + { + // Closed already + } + try + { + await GitHubApi.Git.Reference.Delete(owner, repo, $"heads/{pullRequest.Head.Ref}"); + } + catch + { + // branch already deleted + } + } - protected static async Task CreateTargetBranchAndExecuteTest(string targetBranchName, TemporaryDirectory targetDirectory, Func test) + protected static async Task CreateTargetBranchAndExecuteTest(string targetBranchName, string targetDirectory, Func test) { // first create a new target branch - using (ChangeDirectory(targetDirectory.Directory)) + using (ChangeDirectory(targetDirectory)) { await using (await CheckoutBranchAsync(targetBranchName)) { @@ -1067,4 +1079,19 @@ protected static async Task CreateTargetBranchAndExecuteTest(string targetBranch } } } + + protected static async Task WaitForNewCommitInPullRequest(string repo, Octokit.PullRequest pr, int numberOfCommits = 2) + { + var attempts = 30; + while (attempts-- > 0) + { + pr = await GitHubApi.PullRequest.Get(TestParameters.GitHubTestOrg, repo, pr.Number); + if (pr.Commits >= numberOfCommits) + { + return; + } + await Task.Delay(TimeSpan.FromSeconds(20)); + } + throw new ScenarioTestException($"The created pull request for repo targeting {pr.Base.Ref} did not have a new commit within {attempts * 20 / 60} minutes"); + } } diff --git a/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlow.cs b/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlow.cs index 9ad6b862c8..5a65cac6b9 100644 --- a/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlow.cs +++ b/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlow.cs @@ -10,23 +10,27 @@ namespace ProductConstructionService.ScenarioTests.ScenarioTests; [TestFixture] [Category("PostDeployment")] [Category("CodeFlow")] -internal class ScenarioTests_CodeFlow : CodeFlowScenarioTestBase +internal partial class ScenarioTests_CodeFlow : CodeFlowScenarioTestBase { - private const string TestFileName = "newFile.txt"; + private const string TestFile1Name = "newFile1.txt"; + private const string TestFile2Name = "newFile2.txt"; private const string DefaultPatch = "@@ -0,0 +1 @@\n+test\n\\ No newline at end of file"; private static readonly Dictionary TestFilesContent = new() { - { TestFileName, "test" } + { TestFile1Name, "test" }, + { TestFile2Name, "test" }, }; private static readonly Dictionary TestFilePatches = new() { - { $"{TestFileName}", DefaultPatch }, - { $"src/{TestRepository.TestRepo1Name}/{TestFileName}", DefaultPatch }, - { $"src/{TestRepository.TestRepo2Name}/{TestFileName}", DefaultPatch }, + { $"{TestFile1Name}", DefaultPatch }, + { $"src/{TestRepository.TestRepo1Name}/{TestFile1Name}", DefaultPatch }, + { $"src/{TestRepository.TestRepo1Name}/{TestFile2Name}", DefaultPatch }, + { $"src/{TestRepository.TestRepo2Name}/{TestFile1Name}", DefaultPatch } }; + [Test] public async Task Vmr_ForwardFlowTest() { @@ -48,9 +52,9 @@ public async Task Vmr_ForwardFlowTest() TemporaryDirectory vmrDirectory = await CloneRepositoryAsync(TestRepository.VmrTestRepoName); TemporaryDirectory reposFolder = await CloneRepositoryAsync(TestRepository.TestRepo1Name); - var newFilePath = Path.Combine(reposFolder.Directory, TestFileName); + var newFilePath = Path.Combine(reposFolder.Directory, TestFile1Name); - await CreateTargetBranchAndExecuteTest(targetBranchName, vmrDirectory, async () => + await CreateTargetBranchAndExecuteTest(targetBranchName, vmrDirectory.Directory, async () => { using (ChangeDirectory(reposFolder.Directory)) { @@ -58,7 +62,7 @@ await CreateTargetBranchAndExecuteTest(targetBranchName, vmrDirectory, async () { // Make a change in a product repo TestContext.WriteLine("Making code changes to the repo"); - await File.WriteAllTextAsync(newFilePath, TestFilesContent[TestFileName]); + await File.WriteAllTextAsync(newFilePath, TestFilesContent[TestFile1Name]); await GitAddAllAsync(); await GitCommitAsync("Add new file"); @@ -88,7 +92,7 @@ await CheckForwardFlowGitHubPullRequest( [(TestRepository.TestRepo1Name, repoSha)], TestRepository.VmrTestRepoName, targetBranchName, - [$"src/{TestRepository.TestRepo1Name}/{TestFileName}"], + [$"src/{TestRepository.TestRepo1Name}/{TestFile1Name}"], TestFilePatches); } } @@ -117,9 +121,9 @@ public async Task Vmr_BackwardFlowTest() TemporaryDirectory testRepoFolder = await CloneRepositoryAsync(TestRepository.TestRepo1Name); TemporaryDirectory reposFolder = await CloneRepositoryAsync(TestRepository.VmrTestRepoName); - var newFilePath = Path.Combine(reposFolder.Directory, "src", TestRepository.TestRepo1Name, TestFileName); + var newFilePath = Path.Combine(reposFolder.Directory, "src", TestRepository.TestRepo1Name, TestFile1Name); - await CreateTargetBranchAndExecuteTest(targetBranchName, testRepoFolder, async () => + await CreateTargetBranchAndExecuteTest(targetBranchName, testRepoFolder.Directory, async () => { using (ChangeDirectory(reposFolder.Directory)) { @@ -127,7 +131,7 @@ await CreateTargetBranchAndExecuteTest(targetBranchName, testRepoFolder, async ( { // Make a change in the VMR TestContext.WriteLine("Making code changes in the VMR"); - File.WriteAllText(newFilePath, TestFilesContent[TestFileName]); + File.WriteAllText(newFilePath, TestFilesContent[TestFile1Name]); await GitAddAllAsync(); await GitCommitAsync("Add new file"); @@ -158,7 +162,7 @@ await CheckBackwardFlowGitHubPullRequest( TestRepository.VmrTestRepoName, TestRepository.TestRepo1Name, targetBranchName, - [TestFileName], + [TestFile1Name], TestFilePatches, repoSha, build.Id); @@ -202,17 +206,17 @@ public async Task Vmr_BatchedForwardFlowTest() TemporaryDirectory vmrDirectory = await CloneRepositoryAsync(TestRepository.VmrTestRepoName); TemporaryDirectory repo1 = await CloneRepositoryAsync(TestRepository.TestRepo1Name); TemporaryDirectory repo2 = await CloneRepositoryAsync(TestRepository.TestRepo2Name); - var newFile1Path = Path.Combine(repo1.Directory, TestFileName); - var newFile2Path = Path.Combine(repo2.Directory, TestFileName); + var newFile1Path = Path.Combine(repo1.Directory, TestFile1Name); + var newFile2Path = Path.Combine(repo2.Directory, TestFile1Name); - await CreateTargetBranchAndExecuteTest(targetBranchName, vmrDirectory, async () => + await CreateTargetBranchAndExecuteTest(targetBranchName, vmrDirectory.Directory, async () => { using (ChangeDirectory(repo1.Directory)) await using (await CheckoutBranchAsync(branch1Name)) { // Make a change in a product repo TestContext.WriteLine("Making code changes to the repo"); - await File.WriteAllTextAsync(newFile1Path, TestFilesContent[TestFileName]); + await File.WriteAllTextAsync(newFile1Path, TestFilesContent[TestFile1Name]); await GitAddAllAsync(); await GitCommitAsync("Add new file"); @@ -226,7 +230,7 @@ await CreateTargetBranchAndExecuteTest(targetBranchName, vmrDirectory, async () { // Make a change in a product repo TestContext.WriteLine("Making code changes to the repo"); - await File.WriteAllTextAsync(newFile2Path, TestFilesContent[TestFileName]); + await File.WriteAllTextAsync(newFile2Path, TestFilesContent[TestFile1Name]); await GitAddAllAsync(); await GitCommitAsync("Add new file"); @@ -269,8 +273,8 @@ await CheckForwardFlowGitHubPullRequest( TestRepository.VmrTestRepoName, targetBranchName, [ - $"src/{TestRepository.TestRepo1Name}/{TestFileName}", - $"src/{TestRepository.TestRepo2Name}/{TestFileName}" + $"src/{TestRepository.TestRepo1Name}/{TestFile1Name}", + $"src/{TestRepository.TestRepo2Name}/{TestFile1Name}" ], TestFilePatches); } diff --git a/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlowConflicts.cs b/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlowConflicts.cs new file mode 100644 index 0000000000..650a8fcfbe --- /dev/null +++ b/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlowConflicts.cs @@ -0,0 +1,240 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using Microsoft.DotNet.DarcLib.VirtualMonoRepo; +using Microsoft.DotNet.ProductConstructionService.Client.Models; +using NUnit.Framework; + +namespace ProductConstructionService.ScenarioTests.ScenarioTests; + +[TestFixture] +[Category("PostDeployment")] +[Category("CodeFlow")] +internal partial class ScenarioTests_CodeFlow : CodeFlowScenarioTestBase +{ + [Test] + public async Task ConflictPrClosedTest() + { + var channelName = GetTestChannelName(); + var branchName = GetTestBranchName(); + var productRepo = GetGitHubRepoUrl(TestRepository.TestRepo1Name); + var targetBranchName = GetTestBranchName(); + + await using AsyncDisposableValue testChannel = await CreateTestChannelAsync(channelName); + + await using AsyncDisposableValue subscriptionId = await CreateForwardFlowSubscriptionAsync( + channelName, + TestRepository.TestRepo1Name, + TestRepository.VmrTestRepoName, + targetBranchName, + UpdateFrequency.None.ToString(), + TestParameters.GitHubTestOrg, + targetDirectory: TestRepository.TestRepo1Name); + + TemporaryDirectory vmrDirectory = await CloneRepositoryAsync(TestRepository.VmrTestRepoName); + TemporaryDirectory reposFolder = await CloneRepositoryAsync(TestRepository.TestRepo1Name); + var newFilePath1 = Path.Combine(reposFolder.Directory, TestFile1Name); + var newFileInVmrPath1 = Path.Combine(vmrDirectory.Directory, VmrInfo.SourceDirName, TestRepository.TestRepo1Name, TestFile1Name); + var newFilePath2 = Path.Combine(reposFolder.Directory, TestFile2Name); + var newFileInVmrPath2 = Path.Combine(vmrDirectory.Directory, VmrInfo.SourceDirName, TestRepository.TestRepo1Name, TestFile2Name); + + await PrepareConflictPR( + vmrDirectory.Directory, + reposFolder.Directory, + branchName, + targetBranchName, + newFilePath1, + newFilePath2, + newFileInVmrPath1, + newFileInVmrPath2, + channelName, + subscriptionId.Value, + async () => + { + var pr = await WaitForPullRequestAsync(TestRepository.VmrTestRepoName, targetBranchName); + + await ClosePullRequest(TestParameters.GitHubTestOrg, TestRepository.VmrTestRepoName, pr); + + using (ChangeDirectory(reposFolder.Directory)) + { + await CheckForwardFlowGitHubPullRequest( + [(TestRepository.TestRepo1Name, (await GitGetCurrentSha()).TrimEnd())], + TestRepository.VmrTestRepoName, + targetBranchName, + [ + $"src/{TestRepository.TestRepo1Name}/{TestFile1Name}", + $"src/{TestRepository.TestRepo1Name}/{TestFile2Name}" + ], + TestFilePatches); + } + }); + } + + [Test] + public async Task ConflictResolvedTest() + { + var channelName = GetTestChannelName(); + var branchName = GetTestBranchName(); + var productRepo = GetGitHubRepoUrl(TestRepository.TestRepo1Name); + var targetBranchName = GetTestBranchName(); + + await using AsyncDisposableValue testChannel = await CreateTestChannelAsync(channelName); + + await using AsyncDisposableValue subscriptionId = await CreateForwardFlowSubscriptionAsync( + channelName, + TestRepository.TestRepo1Name, + TestRepository.VmrTestRepoName, + targetBranchName, + UpdateFrequency.None.ToString(), + TestParameters.GitHubTestOrg, + targetDirectory: TestRepository.TestRepo1Name); + + TemporaryDirectory vmrDirectory = await CloneRepositoryAsync(TestRepository.VmrTestRepoName); + TemporaryDirectory reposFolder = await CloneRepositoryAsync(TestRepository.TestRepo1Name); + var newFilePath1 = Path.Combine(reposFolder.Directory, TestFile1Name); + var newFileInVmrPath1 = Path.Combine(vmrDirectory.Directory, VmrInfo.SourceDirName, TestRepository.TestRepo1Name, TestFile1Name); + var newFilePath2 = Path.Combine(reposFolder.Directory, TestFile2Name); + var newFileInVmrPath2 = Path.Combine(vmrDirectory.Directory, VmrInfo.SourceDirName, TestRepository.TestRepo1Name, TestFile2Name); + + await PrepareConflictPR( + vmrDirectory.Directory, + reposFolder.Directory, + branchName, + targetBranchName, + newFilePath1, + newFilePath2, + newFileInVmrPath1, + newFileInVmrPath2, + channelName, + subscriptionId.Value, + async () => + { + var pr = await WaitForPullRequestAsync(TestRepository.VmrTestRepoName, targetBranchName); + + using (ChangeDirectory(vmrDirectory.Directory)) + { + TestContext.WriteLine("Reverting the commit that caused the conflict"); + await CheckoutRemoteRefAsync(pr.Head.Ref); + await RunGitAsync("revert", (await GitGetCurrentSha()).TrimEnd()); + await RunGitAsync("push", "origin", pr.Head.Ref); + } + + using (ChangeDirectory(reposFolder.Directory)) + { + await WaitForNewCommitInPullRequest(TestRepository.VmrTestRepoName, pr, 4); + await CheckForwardFlowGitHubPullRequest( + [(TestRepository.TestRepo1Name, (await GitGetCurrentSha()).TrimEnd())], + TestRepository.VmrTestRepoName, + targetBranchName, + [ + $"src/{TestRepository.TestRepo1Name}/{TestFile1Name}", + $"src/{TestRepository.TestRepo1Name}/{TestFile2Name}" + ], + TestFilePatches); + } + }); + } + + private async Task PrepareConflictPR( + string vmrDirectory, + string productRepoDirectory, + string sourceBranchName, + string targetBranchName, + string newFilePath1, + string newFilePath2, + string newFileInVmrPath1, + string newFileInVmrPath2, + string channelName, + string subscriptionId, + Func test) + { + await CreateTargetBranchAndExecuteTest(targetBranchName, vmrDirectory, async () => + { + using (ChangeDirectory(productRepoDirectory)) + { + await using (await CheckoutBranchAsync(sourceBranchName)) + { + // Make a change in a product repo + TestContext.WriteLine("Making code changes to the repo"); + await File.WriteAllTextAsync(newFilePath1, "not important"); + await File.WriteAllTextAsync(newFilePath2, "not important"); + + await GitAddAllAsync(); + await GitCommitAsync("Add new files"); + + // Push it to github + await using (await PushGitBranchAsync("origin", sourceBranchName)) + { + var repoSha = (await GitGetCurrentSha()).TrimEnd(); + + // Create a new build from the commit and add it to a channel + Build build = await CreateBuildAsync( + GetGitHubRepoUrl(TestRepository.TestRepo1Name), + sourceBranchName, + repoSha, + "1", + []); + + TestContext.WriteLine("Adding build to channel"); + await AddBuildToChannelAsync(build.Id, channelName); + + TestContext.WriteLine("Triggering the subscription"); + await TriggerSubscriptionAsync(subscriptionId); + + TestContext.WriteLine("Waiting for the PR to show up"); + Octokit.PullRequest pr = await WaitForPullRequestAsync(TestRepository.VmrTestRepoName, targetBranchName); + + // Now make a change directly in the PR + using (ChangeDirectory(vmrDirectory)) + { + await CheckoutRemoteRefAsync(pr.Head.Ref); + File.WriteAllText(newFileInVmrPath1, "file edited in PR"); + File.Delete(newFileInVmrPath2); + + await GitAddAllAsync(); + await GitCommitAsync("Edit files in PR"); + await RunGitAsync("push", "-u", "origin", pr.Head.Ref); + } + + // Make a change in a product repo again, it should cause a conflict + TestContext.WriteLine("Making code changes to the repo that should cause a conflict in the service"); + await File.WriteAllTextAsync(newFilePath1, TestFilesContent[TestFile1Name]); + await File.WriteAllTextAsync(newFilePath2, TestFilesContent[TestFile2Name]); + + await GitAddAllAsync(); + await GitCommitAsync("Add conflicting changes"); + await RunGitAsync("push"); + + repoSha = (await GitGetCurrentSha()).TrimEnd(); + TestContext.WriteLine("Creating a build from the new commit"); + build = await CreateBuildAsync( + GetGitHubRepoUrl(TestRepository.TestRepo1Name), + sourceBranchName, + repoSha, + "2", + []); + + TestContext.WriteLine("Adding build to channel"); + await AddBuildToChannelAsync(build.Id, channelName); + + TestContext.WriteLine("Triggering the subscription"); + await TriggerSubscriptionAsync(subscriptionId); + + TestContext.WriteLine("Waiting for conflict comment to show up on the PR"); + pr = await WaitForPullRequestComment(TestRepository.VmrTestRepoName, targetBranchName, "conflict"); + await CheckIfPullRequestCommentExists( + TestRepository.VmrTestRepoName, + targetBranchName, + pr, + [ + $"[{TestFile1Name}](https://github.com/{TestRepository.TestOrg}/{TestRepository.TestRepo1Name}/blob/{repoSha}/{TestFile1Name})", + $"[{TestFile2Name}](https://github.com/{TestRepository.TestOrg}/{TestRepository.TestRepo1Name}/blob/{repoSha}/{TestFile2Name})" + ]); + + await test(); + } + } + } + }); + } +} diff --git a/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_SdkUpdate.cs b/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_SdkUpdate.cs index aa7d729047..5b3a48b1fa 100644 --- a/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_SdkUpdate.cs +++ b/test/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_SdkUpdate.cs @@ -185,7 +185,7 @@ public async Task ArcadeSdkVmrUpdate_E2E() TemporaryDirectory testRepoFolder = await CloneRepositoryAsync(TestRepository.TestRepo1Name); TemporaryDirectory vmrFolder = await CloneRepositoryAsync(TestRepository.VmrTestRepoName); - await CreateTargetBranchAndExecuteTest(targetBranch, testRepoFolder, async () => + await CreateTargetBranchAndExecuteTest(targetBranch, testRepoFolder.Directory, async () => { using (ChangeDirectory(vmrFolder.Directory)) { diff --git a/test/ProductConstructionService.WorkItem.Tests/FakeRedisCache.cs b/test/ProductConstructionService.WorkItem.Tests/FakeRedisCache.cs index e43da30349..edf44fb995 100644 --- a/test/ProductConstructionService.WorkItem.Tests/FakeRedisCache.cs +++ b/test/ProductConstructionService.WorkItem.Tests/FakeRedisCache.cs @@ -14,7 +14,7 @@ public Task SetAsync(string value, TimeSpan? expiration = null) return Task.CompletedTask; } - public Task TryDeleteAsync() => throw new NotImplementedException(); + public Task TryDeleteAsync() => throw new NotImplementedException(); public Task TryGetAsync() => Task.FromResult(_value); public Task GetAsync() => Task.FromResult(_value); public Task GetAsync(string key) => throw new NotImplementedException();