Fix bug #3797: Ensure PR is still open before commenting#3804
Open
Fix bug #3797: Ensure PR is still open before commenting#3804
Conversation
6d611ae to
6049847
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3804 +/- ##
==========================================
+ Coverage 90.03% 90.51% +0.48%
==========================================
Files 174 176 +2
Lines 5058 5084 +26
Branches 464 445 -19
==========================================
+ Hits 4554 4602 +48
+ Misses 504 482 -22 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2b83abc to
0792ddf
Compare
b0c90b0 to
9fe2ffc
Compare
b5323a6 to
668875b
Compare
668875b to
c1bcc6e
Compare
Fixing bug #3797 (Scala Steward commenting unnecessarily & noisily on PRs that have already been closed) needs a way for Scala Steward to _know_ whether a PR has been closed by external action (for example, simply a user _merging_ the PR, which is what we _want_ the users to do!). This is needed within `NurtureAlg.closeObsoletePullRequests()`, and typically there could be _several_ PRs where we need to get latest status - and so a bulk query (where we specify several PR ids) would probably be ideal. For the GitHub _REST_ API, at least, that's not available, and so we're settling with adding a single-item-fetching `ForgeApiAlg.getPullRequest()` method which we will call several times. Scala Steward supports SIX different Forge types (GitHub, GitLab, Gitea, etc) and so adding even one method to the `ForgeApiAlg` involves multiple implementations. I've included tests for all 6 implementations, but do not have accounts on many of the forge types, and so have included best-guess stub test data that's based on what we already had: ### Reusing existing test data without modification * `GitLab` * `BitbucketServer` ### Reusing existing test data, single item extracted from the 'list PRs' case * `GitHub` * `Bitbucket` * `AzureRepos` * `Gitea`
ffe0808 to
a0a73d9
Compare
rtyley
commented
Mar 4, 2026
Comment on lines
-124
to
113
| update = pullRequestRepository | ||
| update = pullRequestService | ||
| .getObsoleteOpenPullRequests(data.repo, _) |
Contributor
Author
There was a problem hiding this comment.
This is the key line that actually fixes issue #3797 - instead of just checking our file-based pullRequestRepository to query which of the pull-requests Scala Steward has opened are now obsolete, we now call our new pullRequestService - and that service additionally hits the Forge API to ensure that the PRs we get from the pullRequestRepository remain currently open.
This change introduces `PullRequestService`, encapsulating `PullRequestRepository` & `ForgeApiAlg`
a0a73d9 to
7b428aa
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes this issue:
This PR is made of two commits:
ForgeApiAlg.getPullRequest()(implementing this for the 6 Forge types, GitHub, GitLab, BitBucket, etc) - this new capability, of getting current information for the state of an existing PR, is required for this fix.PullRequestService, encapsulatingPullRequestRepository&ForgeApiAlg...when reviewing them, it's probably helpful to review each commit separately.
PullRequestService: combining calls toPullRequestRepository&ForgeApiAlgAlthough the key fix here was to additionally hit the Forge API to see if the PRs returned by
pullRequestRepository.getObsoleteOpenPullRequests()actually are still open, this leads to the creation of aPullRequestServicethat provides a combined interface to calls of the Forge API & the flat-file Repository.It turns out there are several places within
NurtureAlgwhere complementary pairs of calls to the Forge API and the flat-file Repository were occurring, and it simplifiesNurtureAlgto make sure thatPullRequestServicetakes over all of them:createPullRequestreplacesforgeApiAlg.createPullRequest&pullRequestRepository.createOrUpdateclosePullRequestreplacespullRequestRepository.changeState&forgeApiAlg.closePullRequestlistPullRequestsForUpdatereplacesforgeApiAlg.listPullRequests&pullRequestRepository.createOrUpdateAdditionally, these two
PullRequestServicemethods enhance existingPullRequestRepositorymethods withforgeApiAlg.getPullRequestwhere it previously should have been used, but wasn't:getObsoleteOpenPullRequestsenhancespullRequestRepository.getObsoleteOpenPullRequestsgetRetractedOpenPullRequestsenhancespullRequestRepository.getRetractedPullRequestsTesting
Testing
ForgeApiAlg.getPullRequest()Scala Steward supports six different Forge types (GitHub, GitLab, Gitea, etc) and so adding even one method to the
ForgeApiAlginvolves multiple implementations. I've included tests for all 6 implementations, but do not have user accounts on many of the forge types, and so have included best-guess stub test data that's based on what we already had:Reusing existing test data without modification
GitLabBitbucketServerReusing existing test data, single item extracted from the 'list PRs' case
GitHubBitbucketAzureReposGiteaTesting
PullRequestService: requires newMockForgeApiAlgIn order to test the substantial new
PullRequestServiceclass, it was necessary to mock its two encapsulated components;PullRequestRepository(already supported inMockContext) andForgeApiAlg- which did not have any existing test implementation. Mocking it requires a minimal in-memory implementation of the Forge API (eg, like the GitHub or BitBucket API) - something that can remember a PR has been created, what its state is, what the next PR number should be, etc.This has been done with
MockForgeApiAlg, implementing all the methods required for testingPullRequestRepository, and leaving other methods unimplemented with???. The state for all this (MockForgeState) has been added as a field to the existingorg.scalasteward.core.mock.MockStateclass, so it can used with existingMockEfftesting.Coverage
Coverage of the project as a whole slightly increases this this PR, but the codecov also points out that of the lines touched in the diff, 8 lines (all in
NurtureAlg) are currently not covered by tests at all:Having reviewed the fairly straightforward changes to
NurtureAlg, and given the effort already expended getting testing working onPullRequestService, I feel that it's acceptable to allow the diff coverage to be lower than the target of 90%.Next steps
After this has bedded in, we can re-enable the commenting temporarily disabled by:
cc @danicheg