vendredi 26 juillet 2019

PHP Unit Testing -- Is this overly mocked? -- Method loops call to other method

FUNCTIONALITY

I have a Laravel project in which I have extended Illuminate\Database\Eloquent\Builder as an AbstractSearch class to introduce a params() method that calls the abstract method applyParams() to be defined when AbstractSearch is extended.

Example:

$models = (new ModelSearch)
    ->params(['name' => 'Model', 'createdBefore' => now()])
    ->with(['modelCategory', 'modelType'])
    ->paginate();

applyParams() is responsible for taking the array parameter from params() and properly constructing the underlying Illuminate\Database\Query\Builder query.

The AbstractSearch class allows casting of these parameters via a protected $casts attribute and a castParams(array $params = []) method.

castParams() loops through the protected $casts attribute and calls $this->castParam($params, $field, $cast) for each element.

Right now, both castParams() and castParam() are public methods, even though the likelihood of castParam() ever being called that way is practically zero.

Relevent excerpt from AbstractSearch:

public function params(array $params = [])
{
    return $this->applyParams(
        $this->prepareParams($params)
    );
}

abstract function applyParams(array $params = []);

public function prepareParams(array $params = []) : array
{
    return $this->castParams(
        $this->applyDefaults($params)
    );
}

public function applyDefaults(array $params = []) : array
{
    foreach ($this->defaults as $field => $value) {
        data_fill($params, $field, $value);
    }

    return $params;
}

public function getCasts()
{
    return $this->casts;
}

public function castParams(array $params = []) : array
{
    foreach ($this->getCasts() as $field => $cast) {
        $this->castParam($params, $field, $cast);
    }

    return $params;
}

public function castParam(&$params, $field, $cast)
{
    if (! isset($params[$field])) {
        return;
    }

    switch ($cast) {
        case 'date':
        case 'datetime':
            if (! $params[$field] instanceof Carbon) {
                $params[$field] = Carbon::parse($params[$field]);
            }
            break;

        default:
            break;
    }
}

TESTING

I have unit tests in place for both methods. The bulk of the testing is done on castParam() to ensure that casting is done properly.

The unit test for castParams() is done with a partial mock of the AbstractSearch class that expects various calls to castParam().

QUESTIONS

I recently re-read some unit testing principles that made me question this class's setup. You can see from the excerpt above that there are a lot of public methods that should probably be protected. In fact, the only method that needs to be public is params().

It logically follows that params() should be the only public method; however, params() relies on the abstract method applyParams() so any test of params() would have to be done on the extending class because the results rely on what applyParams() does.

So...

  1. Have I talked myself to the right spot with the above two paragraphs? It sounds right to me, but it would involve testing that params() returns an instance of Illuminate\Database\Eloquent\Builder and that the expected changes were made to the Builder's underlying Illuminate\Database\Query\Builder and its query. I'm not entirely sure that's even possible.

  2. Going back to castParams() and the partial mock's expectations that it will call castParam() several times. If both castParams() and castParam() remain public methods and if testing should focus on observable behavior, won't I have redundant tests if I test both? Or is it generally acceptable to mock the castParam() calls in my castParams() test since castParam() will be thoroughly tested?

Aucun commentaire:

Enregistrer un commentaire