2

I'm using a repository pattern design and I've hit a stumbling block when writing a unit test for one of my methods. I'm fairly new to writing unit tests, so I would appreciate any help!

Let's say I have a method which creates a product and uses setters to set the data.

public function addProduct(array $data)
{
    $new = $this->repository->make(); // returns new Product object

    $new->setTitle($data['title']);
    $new->setPrice($data['price']);
    $new->setImage($data['image']);
    $new->setStock($data['stock']);

    return $this->repository->save($new); // returns bool
}

Now when I'm writing unit tests for this method, what should I actually be testing?

  1. Should I just check the return type and leave it at that?
  2. Should I mock the return of make() and ensure that all the setters were run?
  3. Should I mock just the repository and ensure that make() and save() were run?

Initially, I decided to go with all three options. However, my concern came from when I started option 2, writing tests to ensure that all the setters were run

Should the unit test really be concerned about whether all the setters ran? What happens if I add more fields? It seems like doing this would mean the tiniest change could result in the unit test failing when the method actually does as performed.


This is how I've wrote my unit test so far, but I'm really unsure about how strict it is

public function testAddProductAddsProductWithCorrectAttributes()
{
    $newMock = Mockery::make(ProductInterface::class)
                    ->shouldReceive('setTitle')
                    ->withArgs(['Test title'])
                    ->shouldReceive('setPrice')
                    ->withArgs([10.99])
                    ->shouldReceive('setImage')
                    ->withArgs(['/foobar.jpg'])
                    ->shouldReceive('setStock')
                    ->withArgs(['In Stock']);

    $repoMock = Mockery::make(RepositoryInterface::class)
                    ->shouldReceive('make')
                    ->andReturn($newMock)
                    ->shouldReceive('save')
                    ->withArgs([$newMock])
                    ->andReturn(true);

    $service = new Service($repoMock);

    $add = $service->addProduct([
        'title' => 'Test title',
        'price' => 10.99,
        'image' => '/foobar.jpg',
        'stock' => 'In Stock'
    ]);

    $this->assertTrue($add);
}
7

When testing a repository the essential thing to test is whether you can get out what you put in.

so... (excuse pseudo code)

$data = ..//whatever
addProduct(array $data)
$actual = getProduct(id)

Assert $actual == $data

This will obviously fail if the setters don't work. But doesn't explicitly test every setter

  • 1
    See, this would be my preferred method of testing - run the add function, try and get it. But I'm not persisting my data in the unit tests, so I'm unsure of how I'd be able to retrieve it – Dan Johnson May 3 '17 at 9:06
  • I officially pronounce your preferred way of doing it to be the best and most correctest way :) (be careful of default values) – Ewan May 3 '17 at 9:07
  • But wouldn't that be considered an integration test? It's relying on both addProduct() and getProduct() to be working as intended – Dan Johnson May 3 '17 at 9:18
  • I did answer another question about what order to write read/write tests so you would catch the failure of either with a specific test. essentially you need to set the database up directly. But either way you still want a 'I read back what I wrote' test at some point – Ewan May 3 '17 at 10:23
  • 2
    well if you deliberately dont persist your data i your test, even to a mocked datasource, then you are limiting what you can test. Sometimes micro tests have value, but its expensive to write then – Ewan May 3 '17 at 10:26
3

The purpose of addProduct() is to ensure that (a) a new product exists and (b) it has the field values specified by the arguments. Therefore, yes, the unit test absolutely should care whether all the setters ran and whether those are all the attributes of the object.

If it didn't care, then you would very likely introduce defects when the set of attributes changes. As it is, the test suite warns you automatically immediately after you introduce a new attribute. Believe me, annoying notifications are way better than unnoticed defects.

  • I've edited the question to include how I'm currently checking if the setters were called. Do you think that is the best way of going around it? – Dan Johnson May 3 '17 at 9:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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