Test Driven Development in Practice - Part 1

Test Driven Development in Practice - Part 1
Refactoring of the Magento 2 Order Payment class using Test Driven Development approach

True developer starts with wine… and tests.



In world of programming and software development everyone is expected to deliver high quality code within short period of time. One of the software development goals (and Magento Inc. isn’t an exception here) is to ensure that changes we made to the system are thoroughly tested.


In the Agile world every single person plays an important role by defining requirements and expectations at an early stage of a development cycle as well as all iterations till product is released live. Whether he or she is a Product Owner, Scrum Master, Quality Engineer or Software Engineer, everyone is a part of the ecosystem and important part of a Team.


You probably have the same question as I do: "What the hell am I talking about?". 
The answer is simple: “Everyone should prove that the system works as expected”. There are only 2 ways that I know:

  • The first one is to manually demonstrate working functionality each time you or your teammate made a change in code. 
However, it becomes a big problem in case you have such a big platform as Magento. Even if a change is a single line of code.
  • The second option is to automate your efforts by various tests (e.g. Unit, Integration or Functional). 


There is a third option where you pay somebody else to perform all dirty work instead of you. In this case you should be a billionaire like Steve Jobs or Bill Gates. Otherwise, fill free to continue reading, my dear Reader.

In this post I am sharing personal experience on how to refactor code in a safe way by using the Test Driven Development approach. Since most of my time I work at Magento 2 project, all code that you will see in this post is going to be based on Magento 2. 


Disclaimer: This post is my personal opinions and not that of my employer.

What is Test Driven Development?

Before we start let’s be sure we are on the same page about Test Driven Development or TDD.

There are good posts written about TDD and I recommend them. Here are few links: Guidelines for Test-Driven Development from Jeffrey Palermo and Introduction to Test Driven Development (TDD). In addition to this, Kent Beck provides a definition of TDD in his “Test Driven Development by Example” book. This book is a “must have” to read for every developer. Also, please check the “Refactoring, improving the design of existing code” book by Martin Fowler. You will learn a process of refactoring with great examples. By the way, this is one of my favorite books. And finally, please check “Clean Code: A Handbook of Agile Software Craftsmanship” book by Robert C. Martin.

Test Driven Development is a software development process which helps teams to build loosely coupled and highly cohesive code with low defect rate. One of the rules defined for TDD says that a developer should only change code when it is covered with automated tests. It means that automated tests should be created and executed first, and more important, you first should have a failing automated test. Having a test that fails helps you quickly make it pass by creating necessary code. Once the test passes, you need to refactor code to eliminate all code duplication which could appear when you were making the test pass. This is the main rule or TDD mantra: “Red, Green, Refactor”.

TDD approach

Pic. 1. TDD - Red, Green, Refactor steps

There are many advantages for using TDD and here are some of them:

  • When you think about your code as a client, application design becomes more solid
  • You definitely have tests in place
  • Software program is ready to go live once all tests successfully pass

Magento 2 Tests

There are 7 different types of tests in the Magento 2 project. Check @alant99 blog post about Magento 2 Testing. Mostly all tests (except Unit Tests) are located under the corresponding folder in the < magento2root >/dev/tests directory. Starting from 0.74.0-beta1 release Magento team moved all Unit Tests to module directories. More information you may get from release notes.

Magento 2 tests directory

Pic. 2. Magento 2 tests directory

In order to perform small and fast “Red, Green, Refactor” iterations, we are going to work with Unit Tests.

Unit Tests

Unit Test is a test which helps to verify single class or method. A class or method under test should be isolated from external class dependencies. Usually, in order to create test you need to download and configure PHPUnit framework. More information on writing tests you may find at PHPUnit Documentation space.

You should be aware that Magento 2 project stores Unit Tests phpunit.xml.dist configuration file under < magento2root >/dev/tests/unit directory. Unit Tests in Magento 2 project are located in corresponding Modules < magento2root >/app/code/< ModuleName >/Test .

Let’s Get This Party Started…

Searching for some issues on the Magento 2 GitHub, I’ve found an interesting comment from user ihor-sviziev. He mentioned that existing implementation of Magento\Sales\Model\Order\Payment class is overcomplicated. Refactoring of this class might be a great example of using TDD in practice.

I am going to use Magento 2 0.74.0-beta3 release. This release does not include the fix which simplifies the Payment class so it is going to be a good refactoring exercise.

The Payment class implements Magento\Sales\Api\Data\OrderPaymentInterface interface as part of a Module Service Contract (a.k.a. Module API) of the Magento\Sales module. The interface provides getters and setters for manipulations with payment related information. In addition to this, the Payment class provides service methods which help to process order payment information. Here is the list of service methods:

Payment class diagram

Pic. 3. The Magento\Sales\Model\Order\Payment public methods.

This diagram shows only the public service API defined in the Payment class. All getter and setter methods are out of the diagram.

The Payment class has a total of 2595 lines of code. This class-that-can-do-any-operation-with-payment class has code smell. This Large Class is difficult to read, understand, and troubleshoot.

We are going to review the Payment::registerPaymentReviewAction() method as one of the most complicated and complex in implementation.

    /**
     * Perform the payment review action: either initiated by merchant or by a notification
     *
     * Sets order to processing state and optionally approves invoice or cancels the order
     *
     * @param string $action
     * @param bool $isOnline
     * @return $this
     * @throws \Exception
     * @SuppressWarnings(PHPMD.CyclomaticComplexity)
     * @SuppressWarnings(PHPMD.NPathComplexity)
     */
    public function registerPaymentReviewAction($action, $isOnline)
    {
        $order = $this->getOrder();
        $transactionId = $isOnline ? $this->getLastTransId() : $this->getTransactionId();
        $invoice = $this->_getInvoiceForTransactionId($transactionId);
        // invoke the payment method to determine what to do with the transaction
        $result = null;
        $message = null;
        switch ($action) {
            case self::REVIEW_ACTION_ACCEPT:
                if ($isOnline) {
                    if ($this->getMethodInstance()->setStore($order->getStoreId())->acceptPayment($this)) {
                        $result = true;
                        $message = __('Approved the payment online.');
                    } else {
                        $result = -1;
                        $message = __('There is no need to approve this payment.');
                    }
                } else {
                    $result = (bool)$this->getNotificationResult() ? true : -1;
                    $message = __('Registered notification about approved payment.');
                }
                break;
            case self::REVIEW_ACTION_DENY:
                if ($isOnline) {
                    if ($this->getMethodInstance()->setStore($order->getStoreId())->denyPayment($this)) {
                        $result = false;
                        $message = __('Denied the payment online');
                    } else {
                        $result = -1;
                        $message = __('There is no need to deny this payment.');
                    }
                } else {
                    $result = (bool)$this->getNotificationResult() ? false : -1;
                    $message = __('Registered notification about denied payment.');
                }
                break;
            case self::REVIEW_ACTION_UPDATE:
                if ($isOnline) {
                    $this->getMethodInstance()->setStore(
                        $order->getStoreId()
                    )->fetchTransactionInfo(
                        $this,
                        $transactionId
                    );
                }
                if ($this->getIsTransactionApproved()) {
                    $result = true;
                    $message = __('Registered update about approved payment.');
                } elseif ($this->getIsTransactionDenied()) {
                    $result = false;
                    $message = __('Registered update about denied payment.');
                } else {
                    $result = -1;
                    $message = __('There is no update for the payment.');
                }
                break;
            default:
                throw new \Exception('Not implemented.');
        }
        $message = $this->_prependMessage($message);
        if ($transactionId) {
            $message = $this->_appendTransactionToMessage($transactionId, $message);
        }
        // process payment in case of positive or negative result, or add a comment
        if (-1 === $result) { // switch won't work with such $result!
            if ($order->getState() != \Magento\Sales\Model\Order::STATE_PAYMENT_REVIEW) {
                $status = $this->getIsFraudDetected() ? \Magento\Sales\Model\Order::STATUS_FRAUD : false;
                $order->setState(\Magento\Sales\Model\Order::STATE_PAYMENT_REVIEW, $status, $message);
                if ($transactionId) {
                    $this->setLastTransId($transactionId);
                }
            } else {
                $order->addStatusHistoryComment($message);
            }
        } elseif (true === $result) {
            if ($invoice) {
                $invoice->pay();
                $this->_updateTotals(['base_amount_paid_online' => $invoice->getBaseGrandTotal()]);
                $order->addRelatedObject($invoice);
            }
            $order->setState(\Magento\Sales\Model\Order::STATE_PROCESSING, true, $message);
        } elseif (false === $result) {
            if ($invoice) {
                $invoice->cancel();
                $order->addRelatedObject($invoice);
            }
            $order->registerCancellation($message, false);
        }
        return $this;
    }

The second name of this method is “Oh my gosh!”. Even PHPDocs says absolutely nothing. But let’s see what this method actually does:

  1. Prepares and accepts online payment that is in review state
  2. Denies online payment
  3. Updates online payment
  4. Processes payment result
  5. Prepares and sets order status and message
  6. Initiates invoice payment
  7. Adds status history comment to an order
  8. Adds the ‘processing’ state to an order
  9. Adds the ‘payment_review’ state to an order
  10. Initiates an order cancellation

And, if we miss or misspell some method’s responsibility it is only because the method is hard to read.

When I searched for method usages I’ve found that the second method argument $isOnline is always set to true. It means that the method processes online payments only. Here I see 2 ways of refactoring:

  1. Eliminate the method logic related to offline payment processing
  2. Use "Extract Method" refactoring to move all offline payment processing logic into a separate method.

To be continued

In the next "Test Driven Development in practice - Part 2" article I am going to share Unit testing technique as well as some helpful tips. We will also finish step-by-step refactoring of the Magento\Sales\Model\Order\Payment class method with refactoring patterns like "Move Method", "Extract Method", "Remove Parameter" and others.

Stay in touch...

Next article: Test Driven Development in Practice - Part 2

I am looking forward for your feedback and any comments on how I can share more valuable to you information.

magento 2 tdd refactoring php

Related Articles

Comments

Next Article Previous Article

LinkedIn Twitter Facebook