test: Add unit tests for RequesterRecipientProvider

[Vinamra-tech]

RequesterRecipientProvider had no test coverage despite containing
non-trivial upstream cause traversal logic. Add four tests covering:

• Direct user trigger with no upstream cause receives email
• No email sent when no user and no upstream cause exists
• Original upstream user receives email when traversing upstream chain
• No crash when upstream project linkage is broken (graceful degradation)

Testing done

All four tests pass locally via:
mvn test -Dtest=RequesterRecipientProviderTest

Output:
Tests run: 4, 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]

1) Windows CI failure

The failure on windows 21 (only 8 tests passing vs ~370 on Linux) usually points to the test not running inside a proper Jenkins test harness. Calling new FreeStyleBuild(project) directly can cause exactly this kind of issue especially on Windows where Jenkins isn’t fully initialized. It’d be safer to align this with how other tests in the same package are set up most likely using JenkinsRule or @jenkinstest.

2) Missing edge case upstream build number not found

You’ve already covered the case where the upstream project is null in testBrokenUpstreamProjectLinkage(), which is great. But there’s still a small gap when the project exists but getBuildByNumber() returns null. That’s basically the other half of the broken linkage scenario so worth adding something like -

Mockito.when(upstreamProject.getBuildByNumber(1)).thenReturn(null);
TestUtilities.checkRecipients(build, new RequesterRecipientProvider());

3) Minor debug mode on mock

descriptor.setDebugMode(true) doesn’t really do anything here since ExtendedEmailPublisherDescriptor is mocked it won’t retain state unless explicitly stubbed. Not a functional issue, just a bit misleading. You can either stub it properly or just remove the line to keep things clean.

[Vinamra-tech]

1) Windows CI failure

The failure on windows 21 (only 8 tests passing vs ~370 on Linux) usually points to the test not running inside a proper Jenkins test harness. Calling new FreeStyleBuild(project) directly can cause exactly this kind of issue especially on Windows where Jenkins isn’t fully initialized. It’d be safer to align this with how other tests in the same package are set up most likely using JenkinsRule or @jenkinstest.

2) Missing edge case upstream build number not found

You’ve already covered the case where the upstream project is null in testBrokenUpstreamProjectLinkage(), which is great. But there’s still a small gap when the project exists but getBuildByNumber() returns null. That’s basically the other half of the broken linkage scenario so worth adding something like -

Mockito.when(upstreamProject.getBuildByNumber(1)).thenReturn(null); TestUtilities.checkRecipients(build, new RequesterRecipientProvider());

3) Minor debug mode on mock

descriptor.setDebugMode(true) doesn’t really do anything here since ExtendedEmailPublisherDescriptor is mocked it won’t retain state unless explicitly stubbed. Not a functional issue, just a bit misleading. You can either stub it properly or just remove the line to keep things clean.

Thanks for detailed review , it's the second time you've made some genuine and interesting comments on my pr and that too back to back!!

1. Sure thing , that new FreeStyleBuild(project) triggered unitialised file system resulting in a crash which Linux forgives.To keep these as fast unit tests and avoid rewriting the whole suite to use JenkinsRule, I updated the setup to fully mock FreeStyleBuild (Mockito.mock(FreeStyleBuild.class)). This bypasses the constructor and isolates the tests entirely.

2. That' an excellent point!!! I've added a 5th test (testUpstreamProjectExistsButBuildIsNotFound) to cover the scenario where the upstream project is found, but getBuildByNumber(1) returns null.

3. Sure, I just added the line to be safe but actually testing it proves it's of no use.

Once again, Thank You for your time

[ArpanC6]

1) Windows CI failure

The failure on windows 21 (only 8 tests passing vs ~370 on Linux) usually points to the test not running inside a proper Jenkins test harness. Calling new FreeStyleBuild(project) directly can cause exactly this kind of issue especially on Windows where Jenkins isn’t fully initialized. It’d be safer to align this with how other tests in the same package are set up most likely using JenkinsRule or @jenkinstest.

2) Missing edge case upstream build number not found

You’ve already covered the case where the upstream project is null in testBrokenUpstreamProjectLinkage(), which is great. But there’s still a small gap when the project exists but getBuildByNumber() returns null. That’s basically the other half of the broken linkage scenario so worth adding something like -
Mockito.when(upstreamProject.getBuildByNumber(1)).thenReturn(null); TestUtilities.checkRecipients(build, new RequesterRecipientProvider());

3) Minor debug mode on mock

descriptor.setDebugMode(true) doesn’t really do anything here since ExtendedEmailPublisherDescriptor is mocked it won’t retain state unless explicitly stubbed. Not a functional issue, just a bit misleading. You can either stub it properly or just remove the line to keep things clean.

Thanks for detailed review , it's the second time you've made some genuine and interesting comments on my pr and that too back to back!!

1. Sure thing , that new FreeStyleBuild(project) triggered unitialised file system resulting in a crash which Linux forgives.To keep these as fast unit tests and avoid rewriting the whole suite to use JenkinsRule, I updated the setup to fully mock FreeStyleBuild (Mockito.mock(FreeStyleBuild.class)). This bypasses the constructor and isolates the tests entirely.
2. That' an excellent point!!! I've added a 5th test (testUpstreamProjectExistsButBuildIsNotFound) to cover the scenario where the upstream project is found, but getBuildByNumber(1) returns null.
3. Sure, I just added the line to be safe but actually testing it proves it's of no use.

Once again, Thank You for your time

Point 1 - You’re absolutely right about mocking FreeStyleBuild instead of using new FreeStyleBuild(project). The key issue with using the constructor directly is that it triggers Jenkins' internal chain which tries to interact with the filesystem. This is the main culprit behind the Windows CI crash. By mocking it fully you avoid that and keep things isolated which makes the test purely a unit test without any external dependencies. This approach definitely feels cleaner and safer.

Point 2 - I’m really glad that resonated with you. The test for testUpstreamProjectExistsButBuildsNotFound is an important one. In real production scenarios it's totally possible for build records to be missing (due to manual deletions retention policies etc) even if the project itself is still there. So having that case explicitly covered in the tests helps make sure the code handles it gracefully when it comes up in the wild.

Point 3 - I agree removing the silent state change in the mock makes the code clearer. If you ever need to test debug logging behavior it’s much better to stub it properly like with Mockito.when(descriptor.isDebugMode()).thenReturn(true). Directly setting state on a mock can be really confusing for anyone reading the code later and it doesn’t actually have any effect on the test. So the cleaner approach is definitely the way to go.