test: Add unit tests for BuildUserRecipientProvider

[Vinamra-tech]

BuildUserRecipientProvider had no test coverage. Add three tests covering:

• User who triggered the build receives email (happy path)
• No email sent when build was not triggered by a user (SCM/timer cause)
• Only the triggering user receives email, not committers in the changeset

This distinguishes BuildUserRecipientProvider behaviour from
DevelopersRecipientProvider, as documented in the source.

Testing done

All three tests pass locally via:
mvn test -Dtest=BuildUserRecipientProviderTest

Output:
Tests run: 3, Failures: 0, Errors: 0, Skipped: 0

Submitter checklist

• Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
• Ensure that the pull request title represents the desired changelog entry
• Please describe what you did
• Link to relevant issues in GitHub or Jira
• Link to relevant pull requests, esp. upstream and downstream changes
• Ensure you have provided tests that demonstrate the feature works or the issue is fixed

[ArpanC6]

Hlw @Vinamra-tech, I noticed that in testNoEmailWhenBuildNotTriggeredByUser, you're explicitly mocking getCause() to return null. Since MockUtilities.addRequestor() isn’t called here anyway, the mock might not be needed. Also, depending on how BuildUserRecipientProvider internally resolves the cause, this could give a false impression of what’s actually being tested.

Could you double-check if this mock is necessary, or if just not calling addRequestor() would be enough?

[ArpanC6]

Just a small style note I noticed that some other test files in this package use @DisplayName annotations on test methods to make the test output more readable. Would you consider adding them here too? Something like this,

@test
@DisplayName("User who triggered build should receive email")
void testUserWhoTriggeredBuildReceivesEmail() throws Exception {

It’s a minor thing, but it would help keep things consistent with the rest of the codebase.

[ArpanC6]

I noticed the tests currently only cover Result.FAILURE. It might be worth adding a test for Result.SUCCESS or Result.UNSTABLE as well, just to make sure the behavior is consistent regardless of the build result. Of course, happy to be wrong if BuildUserRecipientProvider is result-independent but having a test to explicitly verify that would be a nice addition.

[Vinamra-tech]

Hlw @Vinamra-tech, I noticed that in testNoEmailWhenBuildNotTriggeredByUser, you're explicitly mocking getCause() to return null. Since MockUtilities.addRequestor() isn’t called here anyway, the mock might not be needed. Also, depending on how BuildUserRecipientProvider internally resolves the cause, this could give a false impression of what’s actually being tested.

Could you double-check if this mock is necessary, or if just not calling addRequestor() would be enough?

Thanks for the comment! I actually tested removing the getCause() mock to see if it's redundant turns out it is necessary for this specific test. I did some research into the codebase and actually found out that If we let BuildUserRecipientProvider call the real build.getCause(), it eventually reaches TransientActionFactory.factoriesFor(). Because we are using Mockito here rather than spinning up a full @JenkinsRule test environment, the extension cache isn't initialized, which causes the test to crash with an IllegalStateException: Expected 1 instance of jenkins.model.TransientActionFactory$Cache but got 0.

[Vinamra-tech]

Just a small style note I noticed that some other test files in this package use @DisplayName annotations on test methods to make the test output more readable. Would you consider adding them here too? Something like this,

@test @DisplayName("User who triggered build should receive email") void testUserWhoTriggeredBuildReceivesEmail() throws Exception {

It’s a minor thing, but it would help keep things consistent with the rest of the codebase.

Sure thing!! I've now added the same.

[Vinamra-tech]

I noticed the tests currently only cover Result.FAILURE. It might be worth adding a test for Result.SUCCESS or Result.UNSTABLE as well, just to make sure the behavior is consistent regardless of the build result. Of course, happy to be wrong if BuildUserRecipientProvider is result-independent but having a test to explicitly verify that would be a nice addition.

Well yes I did verify it's result-independent and verifying it is actually a great idea.I've added a new testEmailSentRegardlessOfBuildResult test covering Result.SUCCESS to ensure the behavior stays consisten

[ArpanC6]

Thanks for addressing all three points so thoroughly
On the getCause() mock your explanation makes complete sense. Since BuildUserRecipientProvider internally calls TransientActionFactory.factoriesFor(), and the extension cache isn't initialized in a Mockito-only environment (no @JenkinsRule), the mock is genuinely necessary to prevent the IllegalStateException. The fact that you actually tested removing it instead of just guessing that's the right approach.
The @DisplayName annotations are a nice touch they make the test output much more readable and consistent with the rest of the codebase.
And the new testEmailSentRegardlessOfBuildResult test covering Result.SUCCESS is a great addition. It explicitly verifies that BuildUserRecipientProvider is result independent, which is an important behavioral guarantee to have documented in the tests.
One thing I want to say beyond the code winning GSoC or getting a PR merged isn't the most important thing here. What matters is what you learned from this. You didn't just fix the code you actually dug into the codebase, understood why TransientActionFactory behaves differently in a Mockito environment versus a full Jenkins context and explained it clearly. That kind of curiosity and depth is what makes a real engineer. Keep that mindset it'll take you much further than any selection result ever will.
The PR looks good to me Keep going.

[Vinamra-tech]

Thanks for addressing all three points so thoroughly On the getCause() mock your explanation makes complete sense. Since BuildUserRecipientProvider internally calls TransientActionFactory.factoriesFor(), and the extension cache isn't initialized in a Mockito-only environment (no @JenkinsRule), the mock is genuinely necessary to prevent the IllegalStateException. The fact that you actually tested removing it instead of just guessing that's the right approach. The @DisplayName annotations are a nice touch they make the test output much more readable and consistent with the rest of the codebase. And the new testEmailSentRegardlessOfBuildResult test covering Result.SUCCESS is a great addition. It explicitly verifies that BuildUserRecipientProvider is result independent, which is an important behavioral guarantee to have documented in the tests. One thing I want to say beyond the code winning GSoC or getting a PR merged isn't the most important thing here. What matters is what you learned from this. You didn't just fix the code you actually dug into the codebase, understood why TransientActionFactory behaves differently in a Mockito environment versus a full Jenkins context and explained it clearly. That kind of curiosity and depth is what makes a real engineer. Keep that mindset it'll take you much further than any selection result ever will. The PR looks good to me Keep going.

Thank You so much for your kind words and best wishes to you too!!

[ArpanC6]

Thanks for addressing all three points so thoroughly On the getCause() mock your explanation makes complete sense. Since BuildUserRecipientProvider internally calls TransientActionFactory.factoriesFor(), and the extension cache isn't initialized in a Mockito-only environment (no @JenkinsRule), the mock is genuinely necessary to prevent the IllegalStateException. The fact that you actually tested removing it instead of just guessing that's the right approach. The @DisplayName annotations are a nice touch they make the test output much more readable and consistent with the rest of the codebase. And the new testEmailSentRegardlessOfBuildResult test covering Result.SUCCESS is a great addition. It explicitly verifies that BuildUserRecipientProvider is result independent, which is an important behavioral guarantee to have documented in the tests. One thing I want to say beyond the code winning GSoC or getting a PR merged isn't the most important thing here. What matters is what you learned from this. You didn't just fix the code you actually dug into the codebase, understood why TransientActionFactory behaves differently in a Mockito environment versus a full Jenkins context and explained it clearly. That kind of curiosity and depth is what makes a real engineer. Keep that mindset it'll take you much further than any selection result ever will. The PR looks good to me Keep going.

Thank You so much for your kind words and best wishes to you too!!

Keep building..