10

I have several models in my custom extension that only serve the purpose filling in some selects and/or multiselects in the add/edit form of my entities.
So they are what magento calls "source models".
The values involved are always the same and the methods return the same thing.
How should I unit test those? Or even better, should I write unit tests for them?
Here is an example.
The following class is used for add/edit form for a field called type and for the grid column of the same field.

<?php
namespace Sample\News\Model\Author\Source;

use Magento\Framework\Option\ArrayInterface;

class Type implements ArrayInterface
{
    const COLLABORATOR = 1;
    const EMPLOYEE = 2;

    /**
     * Get options
     *
     * @return array
     */
    public function toOptionArray()
    {
        $_options = [
            [
                'value' => '',
                'label' => ''
            ],
            [
                'value' => self::COLLABORATOR,
                'label' => __('Collaborator')
            ],
            [
                'value' => self::EMPLOYEE,
                'label' => __('Employee')
            ],
        ];
        return $_options;
    }

    /**
     * get options as key value pair
     *
     * @return array
     */
    public function getOptions()
    {
        $_tmpOptions = $this->toOptionArray();
        $_options = [];
        foreach ($_tmpOptions as $option) {
            $_options[$option['value']] = $option['label'];
        }
        return $_options;
    }
}

3 Answers 3

16

This is a great thread, and I really like both Answers from @KAndy and @fschmengler.
I would like to add some additional thoughts that I find valuable when asking a question such as "Should I test X?" or "How should I test X?".

What could go wrong?

  • I could make a dumb typo (happens all the time)
    This usually doesn't justify writing a test.
  • Will I copy the code I need from the core or a different module, and then adjust it to my needs?
    I find this actually a very dangerous thing to do that often leaves subtle bugs. In this case, I favor writing a test, if it isn't too expensive. Making source models configuration based actually would make them more risky IMO.
  • Can there be a conflict with a different module?
    This almost only applies to configuration code. In such a case I like to have an integration test that tells me when that happens.
  • Could Magento change the API in a future release?
    Very unlikely in this case, since your code only depends on an interface. But the more concrete classes are involved or if my code extends a core class, this becomes more of a potential risk.
  • A new PHP version could break my code. Or maybe I want to support PHP 5.6 for years to come.
    Again, highly unlikely here, but in some cases I want a test to warn me, should I change the code in future to use incompatible syntax.

How expensive is it to test the code?

This has two aspects:

  • The amount of effort and time it takes to write a test
  • The amount of effort and time it takes to test the piece of code I'm about to write manually.

During development of some piece of code, I tend to have to run the code I'm writing quite often until I consider it done. This is of course much easier with a unit test.

In your case writing a test is dead cheap. It doesn't take much time or effort. @KAndy is right that all code needs to be maintained, but then again, not all tests need to be kept.
This might be an example where I would write a unit test, just to check I don't make a dumb mistake, and then delete it once the class is finished. If a test doesn't provide long term value I think deleting them makes sense.

This question also is important in terms of choosing the type of test to write: unit or integration for example.

How valuable is the piece of code I'm writing?

If a piece of code I'm writing is essential for the service a module provides, I test it, regardless how trivial it is.
If it's just a little utility method, for example UI focused, and not part of the business logic, then maybe not.

Will the code will need to change?

Over time I've become so used to having test coverage, that changing uncovered code feels very insecure. That includes so simple things like adding an option to a source model, but also things like moving a class to a different folder/namespace, or renaming a method.
Having tests in place for such changes is invaluable.

Does it need documentation?

How hard is it to use the code? In you example it's trivial, but in some more complex cases, having a test is great for documentation purposes for other developers (or myself in a few months).

Exploration and Learning

If I'm working on some code and I'm not sure how to test it, I find it very valuable to write a test. The process almost always gives me a deeper understanding of what I'm dealing with.
This is especially true for developers who still consider themselves learning testing.
This also is an example where it might make sense to delete the test afterwards, because the main value it provided was learning.

Discipline and Stress

Sticking to the red-green-refactor loop helps me to go fast. This is especially true under pressure. So even if some piece of code isn't really test-worthy, I might still follow TDD, especially if the code is trivial to test.
This keeps me in the flow and alert.

What and how to test?

Also consider you can write the test in very different granularity.

  • Testing the exact return value.
    This will be a very rigid test that will have to be adjusted to every change. Do you want the test to break, for example, if the order of items in the return array changes?
  • Testing the structure of the return value.
    For the source model this could be checking each sub-array as two records, one with a label and one with a value key.
  • Checking the class implements ArrayInterface.
  • Testing the class provides getOptions() even though that method is not part of the implemented interface.

For each possible thing that can be tested, consider value, maintainability and cost.

Summary

To sum it up: there is no true single answer to a question if some piece of code should be tested or not. The answer will be different for every developer depending on the circumstances.

2
  • 2
    Great answer! I want to higlight the "delete tests when they don't provide value anymore" part - sometimes old tests that helped during initial developent, are just getting in the way long-term. Commented Jun 6, 2016 at 8:20
  • 1
    you just answered 2 other questions I had doubt about. thank you
    – Marius
    Commented Jun 6, 2016 at 8:38
6

In my opinion, there is no general answer to "write unit tests for source models, yes or no"

I've written unit tests for source models, but those were dynamic source models that fetched external data and there it makes total sense.

For source models that are nothing more than glorified arrays (as in your example), I would not bother writing unit tests. But in some way, you need to make sure you did not make a mistake. There are several options:

  1. unit test
  2. integration test
  3. manually look at the configuration

Are you following TDD? Then pick between (1) and (2), or both. Otherwise, pick between (2) and (3).


If you use the source models for system configuration options, an integration test (one test for all your configuration options) could render the configuration section and check if the <select> elements are present and contain the expected values. Remember that this is not about testing one particular class but testing that everything is tied together correctly.


And as @KAndy said, ideally you would not need that much boilerplate, just extend a base class which is already unit tested and override a property or define the values in external configuration.

In that scenario, unit tests for concrete implementations don't provide any additional value. If you have many of these classes, it might be a good idea to write a base class ArraySource yourself, as long as Magento does not provide it.


With such a base class, your source model could look like this:

class Type extends ArraySource
{
    const COLLABORATOR = 1;
    const EMPLOYEE = 2;
    protected $elements = [
        self::COLLABORATOR => 'Collaborator',
        self::EMPLOYEE     => 'Employee',
    ];
    protected $withEmpty = true;
    protected $translate = true;
}
4
  • Thanks for the confirmation. I will try to turn my glorified arrays into a configurable option list, but does the same apply for models that are going to have exactly N options? Like a "status" source model.
    – Marius
    Commented Jun 6, 2016 at 7:21
  • I don't see how a "status" source model would be any different to your "author type" example Commented Jun 6, 2016 at 7:24
  • because I might use the values 0 and 1 (as class constants) on frontend, to filter for example the enabled entities. I mean in this case, the option values have logic behind them. they are not just key=>value pairs.
    – Marius
    Commented Jun 6, 2016 at 7:27
  • But this logic is not part of the source model, right? That means it does not matter here. You'll still have the constants to use in other places. I added an example how I would use such an implentation. Commented Jun 6, 2016 at 7:29
5

How should I unit test those?

I think you should not.

Add code to system increase support and maintenance cost but testing process should be LEAN.

More over this code should not exists. I believe than in Magento should be one generic declarative way to define sets of options to use in different places. And your reluctance of writing test for this class show me smell of bad code

1
  • 2
    Thanks. So you are saying that these options should not be hard-coded but come from a config file (di.xml for example). I can do that. But is the same true for status source models? I mean, if I have the same class as above but only with status enabled and disabled (1,0) should I use the same config approach? If yes, I want to say that it seams like over-engineering for me.
    – Marius
    Commented Jun 6, 2016 at 7:05

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.