git-workflow/autogits
SHA256
21
2
Fork 14
Code Issues 48 Pull Requests 13 Actions Projects 2 Activity

Fixing PR tests in workflow-pr bot #106

Manually merged
adamm merged 15 commits from pr-tests into main 2026-01-27 13:41:47 +01:00
Conversation 3 Commits 15 Files Changed 15 +1780 -401
adamm commented 2026-01-09 13:07:41 +01:00
Owner
Copy Link

This is initial set of fixes for the unit tests. Will add additional tests to increase coverage.

This is initial set of fixes for the unit tests. Will add additional tests to increase coverage.
adamm added 5 commits 2026-01-09 13:07:42 +01:00
pr: fix unit tests 362e481a09
add PRProcessor tests 37c9cc7a57
pr: repo_check unit tests 09001ce01b
pr: add error handling unit tests b7f5c97de1
pr: move common test helpers to dedicated area 2f18adaa67
adamm requested review from jzerebecki 2026-01-09 13:07:44 +01:00
adamm commented 2026-01-09 13:08:45 +01:00
Author
Owner
Copy Link

I think the best to review here is just one commit at a time instead of entire set of changes. They are incremental.

I think the best to review here is just one commit at a time instead of entire set of changes. They are incremental.
adamm added 9 commits 2026-01-10 00:55:55 +01:00
pr: add tests for RebaseAndSkipSubmoduleCommits e8738c9585
pr: add some tests for UpdatePrjGitPR 5f5e7d98b5
pr: add test cases for PRProcessor corner cases 86a7fd072e 
- Add scenarios for closed/merged project PRs that trigger
   submodule checks and downstream PR updates (manual merge vs close).
- Test the "Consistency check" logic where submodules are reset
  if they don't match the PR set.
- Test the "superfluous PR" check (no-op PRs that should be closed).
pr: test verifyRepositoryConfiguration 4f132ec154
pr: test PRProcessor that is triggered by webhook abf8aa58fc 
- PullRequestWebhookEvent: Verified that PR events correctly
  trigger processing with all necessary Gitea and Git mocks.
- IssueCommentWebhookEvent: Verified that issue comment events
  (which Gitea often uses for PR comments) are handled correctly.
- Recursion Limit: Verified that the recursion protection logic
  correctly terminates and cleans up when the limit is reached.
- Invalid Data Format: Verified that non-event data types return
  appropriate errors.
pr: revive PRProcessor sync tests e806d6ad0d 
- Uncomment and fix the existing tests for `synchronized` actions.
- Ensure it uses the new `PullRequestProcessor` interface and mocked dependencies.
pr: fix PR lists to check packages not just project PRs c866303696 
Also,
- Add simple unit tests to verify mapping of `models.StateType`
  to internal event strings.
- Verify it correctly wraps `ProcesPullRequest` and handles panics
  via the deferred recovery block.
- Add tests for scenarios where `GetRecentPullRequests` fails.
- Verify the random sleep interval logic (can be tested by mocking
  `time.Sleep` if refactored, or verifying behavior with interval=0).
pr: Add additional unit tests c05fa236d1 
- Add a test case specifically verifying that `Gitea.SetLabels`
  is called with `staging/Auto` when a *new* project PR is created
  for submodules.
- Verify `PrjGitDescription` and `SetSubmodulesToMatchPRSet` behave
  correctly when a single `PRSet` contains 5+ different package
  repositories.
pr: move interfaces and mocks to parent package
Some checks failed
go-generate-check / go-generate-check (pull_request) Failing after 22s
Details
18f7ed658a
adamm added 1 commit 2026-01-10 00:57:33 +01:00
pr: interfaces moved to main package
All checks were successful
go-generate-check / go-generate-check (pull_request) Successful in 7s
Details
f959684540
jzerebecki approved these changes 2026-01-26 20:05:45 +01:00
jzerebecki left a comment
Owner
Copy Link

Can be merged, but needs followup. Also some questions.

Can be merged, but needs followup. Also some questions.
workflow-pr/pr_processor_opened_test.go
@@ -71,0 +147,4 @@
gitea.EXPECT().CreatePullRequestIfNotExist(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(mockCreatePR, nil, true).AnyTimes()
gitea.EXPECT().RequestReviews(gomock.Any(), gomock.Any()).Return(nil, nil).AnyTimes()
gitea.EXPECT().FetchMaintainershipDirFile(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, "", nil).AnyTimes()
gitea.EXPECT().FetchMaintainershipFile(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, "", nil).AnyTimes()
jzerebecki commented 2026-01-23 14:36:51 +01:00
Owner
Copy Link

For future: In tests with many expected functions, it is difficult to spot which mock expectation is important to the test and which ones are just to define required return values. What is the best way? I'm not sure. What do you think?

Either way it is probably a good idea to be explicit by using Times(0) (instead of no coded expectation) or Times(1) or a small count instead of AnyTimes for important ones.

https://pkg.go.dev/go.uber.org/mock@v0.6.0/gomock#Call should have a way to specify an explanation added to the error message for what an expectation tests, but it currently does not. E.g. it could support a reason("to ensure foo was tested") function on Call which would change the errors to read "expected call to ensure foo was tested at".

So for now what is an alternative. A comment above the important ones might be a good idea. But then I have seen comments above unimportant mocks, so it is still a better idea to separate those. Perhaps use an anonymous function assigned to an explaining variable name like unimportantMocking or a code block{} around them.

I suspect that some tests don't test the important parts from data that is set, but only which functions are called in the test case. Which may be a result from aiming for coverage. Forgetting to make tests useful or forgetting to test the important effects usually causes pain later. But I only found one where an important test might have been omitted, though to be sure if that is so or to find all I would have needed to review in more detail, so maybe my suspicion is in error.

For future: In tests with many expected functions, it is difficult to spot which mock expectation is important to the test and which ones are just to define required return values. What is the best way? I'm not sure. What do you think? Either way it is probably a good idea to be explicit by using Times(0) (instead of no coded expectation) or Times(1) or a small count instead of AnyTimes for important ones. https://pkg.go.dev/go.uber.org/mock@v0.6.0/gomock#Call should have a way to specify an explanation added to the error message for what an expectation tests, but it currently does not. E.g. it could support a reason("to ensure foo was tested") function on Call which would change the errors to read "expected call to ensure foo was tested at". So for now what is an alternative. A comment above the important ones might be a good idea. But then I have seen comments above unimportant mocks, so it is still a better idea to separate those. Perhaps use an anonymous function assigned to an explaining variable name like unimportantMocking or a code block{} around them. I suspect that some tests don't test the important parts from data that is set, but only which functions are called in the test case. Which may be a result from aiming for coverage. Forgetting to make tests useful or forgetting to test the important effects usually causes pain later. But I only found one where an important test might have been omitted, though to be sure if that is so or to find all I would have needed to review in more detail, so maybe my suspicion is in error.
adamm commented 2026-01-27 12:40:28 +01:00
Author
Owner
Copy Link

I agree with you here. But the correct way of handling this would be to start the loop of,

  1. refactor the code into smaller chunks (smaller interfaces), not breaking the tests, then
  2. refactor the unit tests into saner chunks and reduce usage of mocks.
  3. go back to step 1

When we start with case of no unit tests, any unit tests are better than nothing. But you are correct that too many mocks, mostly as result of code that is not doing one thing as it's a series of git commands, makes things confusing in the test.

In this particular snippet though, the use of AnyTimes() is sufficient, though ideally yes, it should be reduced to specific amount of calls. But this requires refactoring and removal of the if() paths.

I agree with you here. But the correct way of handling this would be to start the loop of, 1. refactor the code into smaller chunks (smaller interfaces), not breaking the tests, then 2. refactor the unit tests into saner chunks and reduce usage of mocks. 3. go back to step 1 When we start with case of no unit tests, any unit tests are better than nothing. But you are correct that too many mocks, mostly as result of code that is not doing one thing as it's a series of git commands, makes things confusing in the test. In this particular snippet though, the use of AnyTimes() is sufficient, though ideally yes, it should be reduced to specific amount of calls. But this requires refactoring and removal of the if() paths.
jzerebecki commented 2026-01-27 14:03:07 +01:00
Owner
Copy Link

While too many mocks make things harder, having fewer of them does not solve knowing when a mock is intended to test something by itself vs just enabling a later test.

So where is the test that this causes no action? Is it because the function for an action is not mocked? Then that should be made explicit by expecting it with Times(0).

While too many mocks make things harder, having fewer of them does not solve knowing when a mock is intended to test something by itself vs just enabling a later test. So where is the test that this causes no action? Is it because the function for an action is not mocked? Then that should be made explicit by expecting it with Times(0).
jzerebecki commented 2026-01-27 15:13:09 +01:00
Owner
Copy Link

Filed feature request for mock expect reason: https://github.com/uber-go/mock/issues/296

Filed feature request for mock expect reason: https://github.com/uber-go/mock/issues/296
workflow-pr/pr_processor_sync_test.go
@@ -103,3 +75,1 @@
}
t.Run("PR sync request against PrjGit == no action", func(t *testing.T) {
t.Run("PR_sync_request_against_PrjGit_==_no_action", func(t *testing.T) {
jzerebecki commented 2026-01-26 19:17:28 +01:00
Owner
Copy Link

Most of these _ are wrong. The LLM got confused by the error when running the test?

I suspect this is not testing the no action part, as there is no expect with Times(0).

Most of these _ are wrong. The LLM got confused by the error when running the test? --- I suspect this is not testing the no action part, as there is no expect with Times(0).
adamm commented 2026-01-27 12:53:50 +01:00
Author
Owner
Copy Link

gitea.EXPECT().UpdatePullRequest(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil).AnyTimes()

The UpdatePullRequest() is not called, that's why it has "no action". It basically parses the PrjGit only PR and there's nothing to do.

By default, all mocks are Times(0) unless specified.

gitea.EXPECT().UpdatePullRequest(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil).AnyTimes() The UpdatePullRequest() is not called, that's why it has "no action". It basically parses the PrjGit only PR and there's nothing to do. By default, all mocks are Times(0) unless specified.
jzerebecki commented 2026-01-27 13:54:06 +01:00
Owner
Copy Link

No, by default mocks are Times(1) and AnyTimes() overwrites that to 0-veryBigNumber, so here it allows UpdatePullRequest to be called.

No, by default mocks are Times(1) and AnyTimes() overwrites that to 0-veryBigNumber, so here it allows UpdatePullRequest to be called.
👍 1
jzerebecki commented 2026-01-27 14:09:16 +01:00
Owner
Copy Link

To be more specific expected mocks by default are Times(1). You are right that those where EXPECT() is never called will fail the test if called, but here expect was called.

To be more specific _expected_ mocks by default are Times(1). You are right that those where EXPECT() is never called will fail the test if called, but here expect was called.
adamm commented 2026-01-27 17:42:48 +01:00
Author
Owner
Copy Link

UpdatePullRequest() (Gitea mock) is never called here. This is not mocked in this instance and the other mocked function calls are all reading only, not updating anything (except for the SetRepoOptions, but that does it always and can be ignored)

So, it's unexepcted if this would be called here. "PR sync" test below is expected to call it, but here it is not since the updated PR is the project git PR, where we do not expect to update package PRs from it. Hence the "no action" in the title.

Or are you here simply mentioning that the _ in the tests names is not necessary? In that case, you'd be correct. I think it's added here simply for consistency, so you can find the test by copy-paste the failed test name.

UpdatePullRequest() (Gitea mock) is never called here. This is *not* mocked in this instance and the other mocked function calls are all reading only, not updating anything (except for the SetRepoOptions, but that does it always and can be ignored) So, it's unexepcted if this would be called here. "PR sync" test below is expected to call it, but here it is not since the updated PR is the project git PR, where we do not expect to update package PRs from it. Hence the "no action" in the title. Or are you here simply mentioning that the _ in the tests names is not necessary? In that case, you'd be correct. I think it's added here simply for consistency, so you can find the test by copy-paste the failed test name.
workflow-pr/pr_processor_test.go
@@ -0,0 +574,4 @@
// CreatePullRequestIfNotExist returns isNew=true
gitea.EXPECT().CreatePullRequestIfNotExist(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(prjPR, nil, true).AnyTimes()
// Expect SetLabels to be called for new PR
gitea.EXPECT().SetLabels("test-org", gomock.Any(), int64(10), gomock.Any()).Return(nil, nil).AnyTimes()
jzerebecki commented 2026-01-26 19:56:36 +01:00
Owner
Copy Link

Should be Times(1). Possibly compare that the right the label is set?

Should be Times(1). Possibly compare that the right the label is set?
workflow-pr/pr_processor_test.go
@@ -0,0 +680,4 @@
{PR: pkgPR},
},
}
_ = prset // Suppress unused for now if it's really unused, but it's likely used by common.FetchPRSet internally if we weren't mocking everything
jzerebecki commented 2026-01-26 17:17:31 +01:00
Owner
Copy Link

I don't think that is possible? LLM or human mistake? If I didn't miss anything delete it.

I don't think that is possible? LLM or human mistake? If I didn't miss anything delete it.
adamm commented 2026-01-27 12:59:42 +01:00
Author
Owner
Copy Link

This is LLM comment why prset is unused. It's not used because it's re-created internally from the PR body text. So this prset here is not returned by any of the mocks or used by them.

LLM pasted it in here and the was, I guess, surprised that it's not used 😉 but it's harmless here.

This is LLM comment why prset is unused. It's not used because it's re-created internally from the PR body text. So this prset here is not returned by any of the mocks or used by them. LLM pasted it in here and the was, I guess, surprised that it's not used 😉 but it's harmless here.
workflow-pr/pr_processor_test.go
@@ -0,0 +725,4 @@
{PR: prjPR},
},
}
_ = prset
jzerebecki commented 2026-01-26 17:24:30 +01:00
Owner
Copy Link

Same as above.

Same as above.
adamm marked this conversation as resolved
workflow-pr/pr_processor_test.go
@@ -0,0 +862,4 @@
gitea.EXPECT().GetPullRequestReviews(gomock.Any(), gomock.Any(), gomock.Any()).Return([]*models.PullReview{}, nil).AnyTimes()
gitea.EXPECT().FetchMaintainershipFile(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, "", nil).AnyTimes()
gitea.EXPECT().FetchMaintainershipDirFile(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, "", nil).AnyTimes()
gitea.EXPECT().SetRepoOptions(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil).AnyTimes()
jzerebecki commented 2026-01-26 19:10:30 +01:00
Owner
Copy Link

The commit message says "PR events correctly trigger processing", but as all these allow anything, so do these actually do that?

The commit message says "PR events correctly trigger processing", but as all these allow anything, so do these actually do that?
adamm commented 2026-01-27 13:04:52 +01:00
Author
Owner
Copy Link

This looks like a smoke test that different message formats either do something or return an error. The last test here expects an error if the message format is unknown.

This looks like a smoke test that different message formats either do something or return an error. The last test here expects an error if the message format is unknown.
workflow-pr/repo_check_test.go
@@ -269,3 +259,3 @@
git: git,
}*/
process.EXPECT().Process(gomock.Any(), gomock.Any(), gomock.Any())
// process.EXPECT().Process(gomock.Any())
jzerebecki commented 2026-01-23 14:42:12 +01:00
Owner
Copy Link

For future improvement: This and the surrounding comments should make clear why the code is in a comment here.

For future improvement: This and the surrounding comments should make clear why the code is in a comment here.
adamm commented 2026-01-27 13:10:37 +01:00
Author
Owner
Copy Link

Thanks for the review. Quite a bit of functionality for PR processing happens in the /common code which have actual tests there. In the code here we have either a lot of git operations or it's calling the other routines in /common.

Because of a lot of mocking, it was rather tedious to create almost any unit tests here, so LLM helps. Eventually, this code needs to be refactored and then the tests end up with fewer mocks and be more precise. But one step at a time.

Thanks for the review. Quite a bit of functionality for PR processing happens in the /common code which have actual tests there. In the code here we have either a lot of git operations or it's calling the other routines in /common. Because of a lot of mocking, it was rather tedious to create almost any unit tests here, so LLM helps. Eventually, this code needs to be refactored and then the tests end up with fewer mocks and be more precise. But one step at a time.
adamm manually merged commit 59a47cd542 into main 2026-01-27 13:41:47 +01:00
Sign in to join this conversation.
Reviewers
No Reviewers
jzerebecki
Labels
No items
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
M0ses adamm adamm_super adrianSuSE andriinikitin atartamo bigironman dancermak darix dirkmueller dmach eroca jdsn jzerebecki mstrigl oertel smithfarm
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: git-workflow/autogits#106
Delete Branch "pr-tests"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?