Good start! Nice to see you add Model tests.

Feedback:

Have a look at: https://github.com/piwik/piwik/blob/master/plugins/API/tests/CsvRendererTest.php#L101-101 - I like how Thomas writes unit tests. the naming of methods is this way:

test_$methodBeingTested_should$Condition

So for example shouldReturnNullIfSiteDoesNotExists() would be: test_getSite_shouldReturnFalseWhenSiteDoesNotExist

This way we know by looking at test name function which method it is testing.

Awesome to see those kinda tests :)

Actually, I'm usually trying to use test_$methodBeingTested_should$ExpectedBehavior_If$Condition so test_getSite_shouldNotDeleteSite_IfSiteIdIsInvalid. But it is not always possible and sometimes I forget an underscore ;)

Maybe you also want to create the Model in the constructor to remove duplicated code.

To make tests further easier to maintain, to read and to remove more duplicated code creating your own asserts and helper methods could be nice and reusable. Ideally a test consists only of 2 or 3 lines like this:

public function shouldReturnSiteById()
{
    $id = $this->createSite('1');

    $this->assertGetSitesFromSiteIds(array('test1'), array($id));
}

private function createSite($appendix)
{
    return $this->siteCreator->createSite(array(
        'name' => 'test' . $appendix,
        'main_url' => 'http://example' . $appendix . '.com'
    ));
}

private function assertGetSitesFromSiteIds($expectedSiteNames, $siteIds)
{
    $sites = $this->model->getSitesFromIds($siteIds);

    foreach ($sites as $index => $site) {
        $this->assertEquals($expectedSiteNames[$index], $site['name']);
    }
}

I've also learned the hard way that is good practice to compare the whole returned array since you never know what happens with the other values otherwise. For instance when testing only the name of a site it is hard to tell what happens with the other site parameters. Especially when we think of a database where we don't know whether it'll store all values, whether it'll convert/truncate any values, ... But it'd be ok for now to do it like this and we would maybe rather create those kinda tests in test_createSite maybe ;)

Thank you for this proposed pull request.

Because it was last updated more than one month ago, it is our policy to close pull requests opened for a long time without updates. If you would like to continue work on the pull request, please simply ping us to have it re-opened (after you have pushed a new commit).

We hope you understand this and we look forward to seeing an update from you on this pull request or another one!

Thanks.