Delete remote branch after closing pull request.

[levibostian]

Attempt at implementing this feature.

I did get stuck on writing tests for this feature as I could not determine how to get the branch name from the code inside of spr/spr_test.go.

Feel free to edit the code directly in this PR if you are interested in getting it merged in.

[lukas-mertens]

I would love to see this feature!

[Skipants]

@levibostian I think the branch name will be from_branch, based on the mocked pull request object created here in mockclient. Unfortunately that will be the branch name for every PR because the mocks don't handle different branches.

I tried replacing your ??? with from_branch but there's still quite a few test failures. I think because the expectations in other tests are not expecting the branch deletions: output.log

[levibostian]

Thanks for trying someone out! I appreciate the help.

Do you have a commit or branch you could share with me on the changes you made? It could help me test out your idea faster.

[Skipants]

Yup, @levibostian, here's a patch you can git apply:

From 43472885dda7d7ed03aa22bb04460b1e6a7a89aa Mon Sep 17 00:00:00 2001
From: Andrew Szczepanski <aszczepanski@financeit.io>
Date: Tue, 21 May 2024 15:36:45 -0400
Subject: [PATCH] Updating ??? with from_branch

---
 spr/spr_test.go | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/spr/spr_test.go b/spr/spr_test.go
index 7540358..6cd1b1a 100644
--- a/spr/spr_test.go
+++ b/spr/spr_test.go
@@ -306,13 +306,13 @@ func TestSPRBasicFlowFourCommits(t *testing.T) {
 	githubmock.ExpectMergePullRequest(c4, genclient.PullRequestMergeMethod_REBASE)
 	githubmock.ExpectCommentPullRequest(c1)
 	githubmock.ExpectClosePullRequest(c1)
-	gitmock.ExpectDeleteBranch('???') // Not sure where to find the branch name? 
+	gitmock.ExpectDeleteBranch("from_branch")
 	githubmock.ExpectCommentPullRequest(c2)
 	githubmock.ExpectClosePullRequest(c2)
-	gitmock.ExpectDeleteBranch('???')
+	gitmock.ExpectDeleteBranch("from_branch")
 	githubmock.ExpectCommentPullRequest(c3)
 	githubmock.ExpectClosePullRequest(c3)
-	gitmock.ExpectDeleteBranch('???')
+	gitmock.ExpectDeleteBranch("from_branch")
 	s.MergePullRequests(ctx, nil)
 	lines = strings.Split(output.String(), "\n")
 	assert.Equal("MERGED   1 : test commit 1", lines[0])
-- 
2.45.1

or pull down https://github.com/Skipants/spr/tree/delete-closed-branches

[chriscz]

Nice work @levibostian! Feedback:

1. To prevent other tests failing, make this a configuration option which is off by default and only enable it in one of your own tests.
2. Update the README to document the config option
3. Instead of modifying an existing test, copy the test, enable your config flag and remove any of the unnecessary checks.

[ejoffe]

Sorry about the long delay guys. I agree with @chriscz, lets add a configuration knob for this, off by default so we maintain the same logic, and then can also add a separate unit test for this case.