BatchResponse implementation makes testing difficult.

[ToxicBakery]

• Operating System version: N/A
• Firebase SDK version: 2
• Library version: 6.12.2
• Firebase Product: Messaging

Description:

BatchResponse is implemented as a final class with a package-private constructor. As such, this means that as a consumer of the library I have to either wrap the response object or use reflection to make it testable.

Steps to reproduce:

// This will not compile
new BatchResponse(responses);

Suggestions

I would be happy to submit a PR for this but I would like guidance on implementation preference.

a) Remove the final keyword from the BatchResponse class so tools like Mockito can mock the class.
b) Make the constructor public
c) Add a builder or some other method of instantiating the class. This doesn't seem like a useful change.

[hiranya911]

Our typical response for this sort of requests is asking developers to not depend directly on the SDK interfaces (this is a general best practice when consuming any third party library/API). Instead define your own interfaces, and implement your logic and tests around those interfaces. I suppose this is what you refer to as "wrapping".

interface MulticastSender {
  MulticastResponse sendMulticast(String title, String body, List<String> tokens);
}

interface MulticastResponse {
  int getSuccessCount();
  int getFailureCount();
}

class FirebaseMulticastResponse implements MulticastResponse {
  private final BatchResponse response;
  // rest of the impl
}

class TestMulticastResponse implements MulticastResponse {
  // test impl
}

If we must make a change in the SDK to make this a bit easier, how about we turn BatchResponse into an interface, and provide a package-private implementation of it in the SDK? Then developers can potentially come up with their own implementation of the interface for testing purposes.

[ToxicBakery]

Sure an interface is fine too. If that's the preference, what naming is preferred in the library for the concrete implementation and interface? IBatchResponse seems like it would be the lowest possible impact. You recommended making BatchResponse the interface which is fine too I'm just not sure what to name the concrete class in that scenario.

Based on other interfaces and concrete implementations, I moved the implementation to BatchResponseImpl and converted the original class to an interface as suggested.

[robertdue]

Sure an interface is fine too. If that's the preference, what naming is preferred in the library for the concrete implementation and interface? IBatchResponse seems like it would be the lowest possible impact. You recommended making BatchResponse the interface which is fine too I'm just not sure what to name the concrete class in that scenario.

Based on other interfaces and concrete implementations, I moved the implementation to BatchResponseImpl and converted the original class to an interface as suggested.

Could this be done for the nested class "SendResponse" as well?

[ccadek]

Sure an interface is fine too. If that's the preference, what naming is preferred in the library for the concrete implementation and interface? IBatchResponse seems like it would be the lowest possible impact. You recommended making BatchResponse the interface which is fine too I'm just not sure what to name the concrete class in that scenario.
Based on other interfaces and concrete implementations, I moved the implementation to BatchResponseImpl and converted the original class to an interface as suggested.

Could this be done for the nested class "SendResponse" as well?

This would greatly benefit testing. I currently need to integrate PowerMock just so I can Mock SendResponse.