PHPUnit test for a PaymentMethodRuleManager

Question

The following code snippet is about adding rules that filters Payment Methods listing to customer, for email we provide 4 different types of payment methods, but we do show them depending on some rules.

<?php
namespace App\Services\Checkout\Module;

use App\Services\Checkout\Module\PaymentMethodRules\PaymentMethodRule;

class PaymentMethodRuleManager
{
    /**
     * @var PaymentMethodList
     */
    private $list = [];

    /**
     * @var PaymentMethodList
     */
    private $blackList = [];

    /**
     * @var PaymentMethodRule[]
     */
    private $rules = [];

    /**
     * PaymentRuleManager constructor.
     * @param PaymentMethodList $list
     * @param PaymentMethodList $blackList
     */
    public function __construct(PaymentMethodList $list, PaymentMethodList $blackList)
    {
        $this->list = $list;
        $this->blackList = $blackList;
    }

    /**
     * @return array
     */
    public function filter()
    {
        return array_diff($this->list->get(), $this->blackList->get());
    }

    /**
     * @return PaymentMethodList
     */
    public function getList()
    {
        return $this->list;
    }

    /**
     * @return PaymentMethodList
     */
    public function getBlackList()
    {
        return $this->blackList;
    }

    /**
     * @param PaymentMethodRule $rule
     * @return $this
     */
    public function addRule(PaymentMethodRule $rule)
    {
        $this->rules[] = $rule;

        return $this;
    }

    public function applyRules()
    {
        foreach ($this->rules as $rule) {
            $rule->run($this);
        }
    }
}

namespace App\Services\Checkout\Module\PaymentMethodRules;

use App\Library\Payment\Method;
use App\Services\Checkout\Module\PaymentMethodRuleManager;

class AdminRule implements PaymentMethodRule
{
    /**
     * @var boolean
     */
    private $isAdmin;

    /**
     * @var bool
     */
    private $isBankTransferAvailable;

    /**
     * @param boolean $isAdmin
     * @param bool $isBankTransferAvailable
     */
    public function __construct($isAdmin, $isBankTransferAvailable)
    {
        $this->isAdmin = $isAdmin;
        $this->isBankTransferAvailable = $isBankTransferAvailable;
    }

    /**
     * @param PaymentMethodRuleManager $paymentMethodRuleManager
     */
    public function run(PaymentMethodRuleManager $paymentMethodRuleManager)
    {
        if ($this->isAdmin) {
            $paymentMethodRuleManager->getList()->add([Method::INVOICE]);
        }

        if ($this->isAdmin && $this->isBankTransferAvailable) {
            $paymentMethodRuleManager->getList()->add([Method::BANK_TRANSFER]);
        }
    }
}

[[Test]]

<?php
namespace tests\Services\Checkout\Module;

use App\Library\Payment\Method;
use App\Services\Checkout\Module\PaymentMethodList;
use App\Services\Checkout\Module\PaymentMethodRuleManager;
use App\Services\Checkout\Module\PaymentMethodRules\AdminRule;

class AdminRuleTest extends \PHPUnit_Framework_TestCase
{
    const IS_ADMIN = true;
    const IS_NOT_ADMIN = false;
    const IS_BANK_TRANSFER = true;
    const IS_NOT_BANK_TRANSFER = false;

    /**
     * @test
     * @dataProvider runDataProvider
     *
     * @param bool $isAdmin
     * @param bool $isBankTransferAvailable
     * @param array $expected
     */
    public function runApplies($isAdmin, $isBankTransferAvailable, $expected)
    {
        $paymentMethodRuleManager = new PaymentMethodRuleManager(
            new PaymentMethodList([]),
            new PaymentMethodList([])
        );

        $adminRule = new AdminRule($isAdmin, $isBankTransferAvailable);
        $adminRule->run($paymentMethodRuleManager);

        $this->assertEquals($expected, $paymentMethodRuleManager->getList()->get());
    }

    /**
     * @return array
     */
    public function runDataProvider()
    {
        return [
            [self::IS_ADMIN, self::IS_BANK_TRANSFER, [Method::INVOICE, Method::BANK_TRANSFER]],
            [self::IS_ADMIN, self::IS_NOT_BANK_TRANSFER, [Method::INVOICE]],
            [self::IS_NOT_ADMIN, self::IS_BANK_TRANSFER, []],
            [self::IS_NOT_ADMIN, self::IS_NOT_BANK_TRANSFER, []]
        ];
    }
}

Answer 1

First off I'll answer your main question which was about unit testing, but the review of the code under test will follow.

Unit Test

/**
 * @test
 * @dataProvider runDataProvider
 *
 * @param bool $isAdmin
 * @param bool $isBankTransferAvailable
 * @param array $expected
 */
public function runApplies($isAdmin, $isBankTransferAvailable, $expected)
{
    $paymentMethodRuleManager = new PaymentMethodRuleManager(
        new PaymentMethodList([]),
        new PaymentMethodList([])
    );

    $adminRule = new AdminRule($isAdmin, $isBankTransferAvailable);
    $adminRule->run($paymentMethodRuleManager);

    $this->assertEquals($expected, $paymentMethodRuleManager->getList()->get());
}

• Extra dependencies are included in the test: PaymentMethodRuleManager and PaymentMethodList.
• Coverage should be set with @cover to cover only the AdminRule that is being tested.
• The assertion relies on the PaymentMethodRuleManager so it is not really unit testing the AdminRule class.

Some of these will be easily fixed with the suggestions below.

Code

Structure

Let's have a look at the basics:

class PaymentMethodRuleManager {}

class AdminRule implements PaymentMethodRule {}

This is a good structure. You have a rule manager which can be configured by adding rules. This should allow the rule manager to accept generic rules and apply them to obtain the list of payment methods.

But, there is a problem...

Your rules are too smart. In fact they even take on a management role:

public function run(PaymentMethodRuleManager $paymentMethodRuleManager)
{
    if ($this->isAdmin) {
        $paymentMethodRuleManager->getList()->add([Method::INVOICE]);
    }

    if ($this->isAdmin && $this->isBankTransferAvailable) {
        $paymentMethodRuleManager->getList()->add([Method::BANK_TRANSFER]);
    }
}

This rule is telling the $paymentMethodRuleManager what to do. Also, it is breaking the Law of Demeter. You know you are breaking the law when you are talking to a stranger. $paymentMethodRuleManager is a friend, but what it returns with $paymentMethodRuleManager->getList() is a stranger. Calling add on that stranger is too trusting and it is hard to tell what the stranger will do with it.

Solution

I think the key thing you want for your rule is to determine whether a payment method should be usable and then what the payment method is:

interface PaymentMethodRule
{
    /**
     * Get the method of payment for this rule.
     *
     * @return int
     */
    function getMethod();

    /**
     * Return whether the payment method is usable.
     *
     * @return bool
     */
    function isUsable();
}

The rule then becomes more of a real object. Instead of an AdminRule you would have an InvoiceRule. This makes more sense as an object:

class InvoiceRule implements PaymentMethodRule
{
    /**
     * @var boolean
     */
    private $isAdmin;

    /**
     * @param boolean $isAdmin
     */
    public function __construct($isAdmin)
    {
        $this->isAdmin = $isAdmin;
    }

    /**
     * @inheritdoc
     */
    public function getMethod()
    {
        return Method::INVOICE;
    }

    /**
     * @inheritdoc
     */
    public function isUsable()
    {
        return $this->isAdmin;
    }
}

The logic for each payment method is now within its rule. Each rule stands alone without any dependency on the PaymentMethodRuleManager.

The PaymentMethodRuleManager becomes something like:

class PaymentMethodRuleManager
{
    private $rules = [];

    public function addRule(PaymentMethodRule $rule)
    {
        $this->rules[] = $rule;
    }

    public function getList()
    {
        $list = [];

        foreach ($this->rules as $rule) {
            if ($rule->isUsable()) {
                $list[] = $rule->getMethod();
            }
        }

        return $list;
    }
}

You could actually get rid of blacklists by just configuring the rules that should match.

Unit Testing is now easy

The unit test for the InvoiceRule is quite easy. You don't have to worry about any dependencies. When unit testing the PaymentMethodRuleManager be sure to use mocks so as not to inject another class which could cause side effects:

/**
  * @covers PaymentMethodRuleManager
  */
public function testMultiplePaymentMethodRulesAreUsable()
{
    // Use a mock of the PaymentMethodRule interface.
    $ruleOne = $this->createMock('PaymentMethodRule');
    $ruleOne
        ->method('isUsable')
        ->with()
        ->will($this->returnValue(true));
    $ruleOne
        ->method('getMethod')
        ->with()
        ->will($this->returnValue(METHOD::INVOICE));

    $ruleTwo = $this->createMock('PaymentMethodRule');
    $ruleTwo
        ->method('isUsable')
        ->with()
        ->will($this->returnValue(true));
    $ruleTwo
        ->method('getMethod')
        ->with()
        ->will($this->returnValue(METHOD::BANK_TRANSFER));

    $objectUnderTest = new PaymentMethodRuleManager;
    $objectUnderTest->addRule($ruleOne);
    $objectUnderTest->addRule($ruleTwo);

    $this->assertSame(
        [METHOD::INVOICE, METHOD::BANK_TRANSFER],
        $objectUnderTest->getList()
    );
}

Answer 2

PaymentMethodRuleManager thoughts:

• Add a use statement for PaymentMethodList.
• Do you really need to calculate array_diff every time in your filter() method? Perhaps either calculate in constructor and store, or lazily calculate and store from within your filter() method.
• Add doc block for applyRules().
• Division of labor around applyRules() seems odd. Why call a method on a PaymentMethodRule object to access a PaymentMethodList object stored that is already stored in this class.

Perhaps something like this make better sense:

public function applyRules()
{
    foreach ($this->rules as $rule) {
        $this->list->add(Method::INVOICE);
    }
}

Of course this doesn't take isAdmin piece into consideration. Not sure why this is store on the rule object (this is unclear to me). At any rate, the interaction between these three classes triggered from this method seems odd.

AdminRule thoughts:

• In your constructor, you should validate you are getting boolean values passed and fail out (ideally by throwing an exception) if this is not the case.
• Outside of earlier thoughts around how run is implemented between these classes as noted above, I also question the method name itself. It doesn't seem like run() is an appropriate name, as you are doing nothing here other than adding a rule to a list of rules. Perhaps addRule() or similar is more appropriate.
• Think about changing around your conditionals in run() to simplify them and be clearer around logic of what is happening (i.e. nothing happen if isAdmin is false).

Something like this might be a little better:

public function run(PaymentMethodRuleManager $paymentMethodRuleManager)
{
    // fast return if isAdmin is false
    if (!$this->isAdmin) {
        return;
    }

    if ($this->isBankTransferAvailable) {
        $paymentMethodRuleManager->getList()->add([Method::BANK_TRANSFER]);

    } else {
        $paymentMethodRuleManager->getList()->add([Method::INVOICE]);
    }
}

The early return might even be changed to do something different. For example, if you expect that this method should never even be called with isAdmin === false, then perhaps an exception is thrown. Should this method have a return value to indicate success/failure to caller?

AdminRuleTest thoughts:

• You should consider sticking with common naming convention of test* for your test cases. testRun() in this case.
• You might consider stubbing/mocking the PaymentMethodRuleManager and PaymentMethodList dependencies in your test here to isolate your testing to only the subject class under test.
• Because of the way you are having the rule "apply itself" to the manager and list classes, you might be introducing a gap in test coverage across these classes. How do you test cases where one tries to add (run()) a rule where the rule already exists in a list? How about a case where the rule exists on the blacklist? This will be very challenging now since the run() method exists on the rule itself rather then on the list or manager where it probably should belong. To get good unit test coverage across these various cases from this class would require you to do a lot of dependency setup.
• You should test your constructor as well. Test edge cases around invalid parameters being provided (and asserting that appropriate exceptions are thrown if following early suggestion to throw exceptions in such a case).
• I could see test list follows:

Tests:

• testRunAddsRulesToListProperly (your current method and data provider renamed to what you are actually testing)
• testConstructorThrowsInvalidArgumentException (assuming you add validation to your constructor).

This test might look like:

/*
 * @test
 * @dataProvider invalidParamDataProvider
 * @expectedException \InvalidArgumentException
 */
public function testConstructorThrowsOnInvalidAdminParam($admin, $bankTransfer) {
    $notUsed = new AdminRule($admin, $bankTransfer);
}

/*
 * Provide various combinations of invalid values
 * (especially truthy/falsey values that could misbehave when interpreted as boolean).
 * Each combination should result in exception.
 * @return array
*/
public function invalidParamDataProvider() {
    return [
        ['',''],
        [true, ''],
        [false, ''],
        ['', true],
        ['', false],
        [1, 1],
        [0, 0],
        [[], true],
        // etc.
    ];
}