Unit testing protected methods which have no reason to be public

Question

I need to make a sponsorship system with complex business requirements. Basically, after a user makes a payment, the system should get triggered. There are many different types of sponsoring, so I found abstractions to make my life simpler now and in the future.

To use polymorphism, I first created an abstract class that every sponsoring system would inherit from, and implement each abstract method needed.

Here's the class :

abstract class SponsorshipService
{
    /** @var UserKang */
    protected $user;
    /** @var UserKang */
    protected $sponsor;
    /** @var SponsorUser */
    protected $sponsorship;
    /** @var int */
    protected $amount;
    /** @var BillingMicroService */
    protected $billingMicroService;
    /** @var MailMicroService */
    protected $mailMicroService;

    public function __construct(BillingMicroService $billingMicroService, MailMicroService $mailMicroService)
    {
        $this->billingMicroService = $billingMicroService;
        $this->mailMicroService = $mailMicroService;
    }

    public function apply(UserKang $user, int $amount): bool
    {
        $this->user = $user;
        $this->amount = $amount;

        if (!$this->user->isSponsored()) {
            return false;
        }

        $this->sponsor = $this->user->getSponsor();
        $this->sponsorship = $this->user->getSponsorship();

        if (!$this->checkSponsorshipAppliance()) {
            return false;
        }

        $offeredCash = $this->getOfferedCash();
        $isCreditOffered = $this->offerCreditSponsorship($offeredCash);

        if (!$isCreditOffered) {
            return false;
        }

        $this->sendEmail();
        $this->updateSponsorship();
    }

    abstract protected function checkSponsorshipAppliance(): bool;

    abstract protected function getOfferedCash(): int;

    abstract protected function offerCreditSponsorship(): bool;

    abstract protected function sendEmail(): void;

    abstract protected function updateSponsorship(): void;
}

This is all good so far, but I'd like to unit tests each of these abstract methods for each SponsorshipService implementation I'll make. But I made these protected, because it doesn't seem like they have any reason to be public, and I think the less public methods a class has, the easier it is to use, and the safer it is too.

What's wrong in this design ? Clearly I don't want to unit test apply in every possible way for every implementation as it would be a nightmare as much now as it'd be in the future.

related: [Testing private methods as protected](https://softwareengineering.stackexchange.com/q/292087/31260) — gnat, Dec 09 '18 at 13:36
@gnat I don't understand how this is related ? Isn't that answer talking about private vs protected ? I feel like something is wrong because I'm using protected, but I don't want to use public nor private. — Steve Chamaillard, Dec 09 '18 at 14:14
the point is, one generally better abstains of weakening access restrictions for (incorrectly understood) testability considerations — gnat, Dec 09 '18 at 14:37
@gnat That's not what I want to do, which is why I feel something is wrong about the design itself. I feel maybe I should use composition for example instead of inheritance. — Steve Chamaillard, Dec 09 '18 at 14:44
If you absolutely feel the need to use protected members, then one option is just write a unit test which involves a class which inherits the class being tested (with protected members) and exposes the test functions in its public interface. The test subclass can then access what it needs for the sake of testing. — , Dec 09 '18 at 14:53
I do think gnat's suggestion and answer is very good though if you can read between some lines and don't tunnel vision yourself into feeling like the option is only public, protected, or private, to reconsider the design if you feel the temptation to use protected access (and I might even go further than gnat by including protected methods in there and not just protected variables, though it's a bit difficult to explain alternatives without dissecting specific examples). Ideally the only thing you need for test coverage is to thoroughly test public interfaces. — , Dec 09 '18 at 14:58
If I don't go that far though and we accept protected methods, then still you should be able to cover all the test "gaps" for a particular class by thoroughly its public interface. Whatever use a particular subclass gains from its protected interface is still just part of properly ensuring that its public interface functions correctly. To put it another way, theoretically you might have a protected method which functions incorrectly in some peculiar edge use case. But if it's never used that way by the subclass, then for all practical purposes to the outside world, the class works as intended. — , Dec 09 '18 at 15:03
@DragonEnergy Oh I see, so basically what you're saying is that since it's protected/private, it won't be used in any other way than the ways which are used/tested in the public interface, which means the edge cases are bound to the upper level of visibility ? — Steve Chamaillard, Dec 09 '18 at 15:25
@SteveChamaillard Yes, precisely, and if some change occurs in those classes when you have very complete coverage which runs into some edge case in the protected functionality (which it couldn't before), then those tests should pick it up. And if you absolutely feel the need to test the protected methods on their own, then inheriting from the class which defines them and providing a public method to test them is one way to get around the access restriction for the particular test. — , Dec 09 '18 at 15:27
@SteveChamaillard Another way to put it is that your classes can use all sorts of things to implement itself. You can think of protected methods as part of that, albeit available to all subclasses. And those could include things like third party APIs which could glitch out in some obscure edge case. But if the class constraints itself in its implementation in a way such that its range of use cases cannot possibly encounter said edge case, then it becomes a non-concern as far as unit testing it. That's a separate concern involving a separate interface, and I'd apply that same mindset [...] — , Dec 09 '18 at 15:33
[...] protected methods. Testing them is not part of testing the correctness of a particular subclass. You can only use that class in the outside world through its public interface, and I'd look at test coverage in that case as feeding the public interface all possible unique input combinations that exhaust all possible use cases (ex: all possible branches of code in the implementation). — , Dec 09 '18 at 15:34
@SteveChamaillard I wrote a second answer just now which might clarify this whole kind of idea about the flow of dependencies (and made a little Gliffy diagram): https://softwareengineering.stackexchange.com/questions/94086/decoupling-classes-from-the-user-interface/384214#384214 — , Dec 18 '18 at 16:05

score 5 · Accepted Answer · answered Dec 09 '18 at 15:25

What's wrong in this design?

It is a subjective question, though here's some food for thought:

The constructor initializes two instance members (billingMicroService, mailMicroService), yet not all instance members — the others get initialized in apply.

When we have instance fields that can be grouped into two (or more) distinct sets of lifetimes within one class, it suggests that we have conflated what could be independent domain concepts/entities — and this suggests refactoring into two (or more) classes. (Sometimes one set of instance fields could become parameters instead of another class.)

apply also conditionally initializes some fields suggesting possible further conflation.

Do you expect a single concrete subclass to implement all the features you are offering in the abstract base, like sending emails and offerings of cash as well as offerings of credit?

This feels like a violation of single responsibility, for one.

But for another, if you want to mix and match abilities to send emails, with different ways to offer cash, with yet differing ways of offering credit, you'll have to create a new subclass for each combination — this results in class explosion, and can be addressed by composition.

(While it is true that you can introduce abstract subclasses that offer partial functionality so you have more DRY (reuse) than re-implementing things in each concrete subclass, managing a class hierarchy to pick features that way is hard an unnecessary since this can be done more simply with composition instead.)

If you use composition instead of inheritance, I believe you'll find testing individual capabilities easier.

So I tried using a composition design, which leads to a significant increase in number of classes. Instead of having 3 different implementations extending the base abstract class, I now have 2 validating classes, 3 offered cash calculating classes, 2 classes to offer credits, 3 email sending classes, and 1 sponsorship repository to update the model. Doesn't it seem like a lot ? Although I understand it's **a lot** more flexible, is the complexity worth it ? Could it be that I'm having trouble finding the correct abstractions to use composition with less classes created ? — Steve Chamaillard, Dec 09 '18 at 15:48
We need as many classes as we have independent concepts in the domain -- that is not artificial complexity, it is real world complexity -- the domain is rich. — Erik Eidt, Dec 09 '18 at 15:54
So, I disagree that sheer class count represents increased complexity over inheritance (which has its own class count). The complexity introduced by conflating domain entities and their lifetimes, by inheritance and overrides, is higher in my book than having numerous simple(r) classes connected by composition & delegation. A simpler design, even with more classes, is less complex. — Erik Eidt, Dec 09 '18 at 15:57
I can solve any computing problem with only one class. I also refuse to maintain systems that use only one class. :P — candied_orange, Dec 09 '18 at 16:27

Unit testing protected methods which have no reason to be public

1 Answers1