Friday 20 December 2019

PHPUnit: trying to be clever with data providers and failing

G'day:
Yesterday we had a seemingly strange situation with a test one of my colleagues had written. It was behaving as if PHPUnit's exactly matcher was not working. The developer concerned was in the process of concluding that that is exactly what the problem is: PHPUnit. I suggested we pause and back-up a bit: given thousands of people uses PHPUnit and the exactly matcher every day (including ourselves) and have not reported this issue, whereas the code we are running it with was freshly written and not used by anyone yet... almost certainly the problem lay with our code.

Before I get on with what the problem was, this bears repeating:

If you are doing something new and it's going wrong, the problem will almost certainly not be related to any established third-party tools you are using. It'll be down to how you're using them.

I find it's a really helpful mindset when troubleshooting problems to just own the issue from the outset, rather than looking around for something else to blame. And if you've looked and looked and can't see the problem with your own code, this probably just means you haven't spotted the problem yet. Absence of evidence is not evidence of absence.

Anyway, the issue at hand. Here's a pared-down repro of the situation. The code is meaningless as I've removed all business context from it, but we have this:

class MyService
{
    private $dependency;

    function __construct(MyDependency $dependency){
        $this->dependency = $dependency;
    }

    function myMethod() {
        return $this->dependency->someMethod();
    }
}


And MyDependency is this:

class MyDependency
{
    function someMethod(){
        return "ORIGINAL VALUE";
    }
}

myMethod is new, so the dev had written a test for this:

class MyServiceTest extends TestCase
{

    /** @dataProvider provideTestCasesForMyMethodTests */
    public function testMyMethodViaDataProvider($dependency)
    {
        $dependency
            ->expects($this->once())
            ->method('someMethod')
            ->willReturn("MOCKED VALUE");

        $service = new MyService($dependency);

        $result = $service->myMethod();

        $this->assertEquals("MOCKED VALUE", $result);
    }

    public function provideTestCasesForMyMethodTests(){
        return [
            'it should call someMethod once' => [
                'dependency' => $this->createMock(MyDependency::class)
            ]
        ];
    }
}

I hasten to add that it's completely daft to have the data provider in this example. However in the real-world example there were valid test variations, and given the method being tested was a factory method, part of the test was to check that the relevant method had been called on the relevant object being returned by the factory method. The data provider was legit.

This test passes:

PS C:\src\php\general> vendor\bin\phpunit .\test\phpunit\expectsWithDataProviderIssue\MyServiceTest.php
PHPUnit 8.5.0 by Sebastian Bergmann and contributors.

.                                                                   1 / 1 (100%)

Time: 77 ms, Memory: 4.00 MB

OK (1 test, 1 assertion)
PS C:\src\php\general>

However when we were trouble shooting something, we ended up with this assertion:

$dependency
    ->expects($this->exactly(10))
    ->method('someMethod')
    ->willReturn("MOCKED VALUE");


And the test still passed:

PS C:\src\php\general> vendor\bin\phpunit .\test\phpunit\expectsWithDataProviderIssue\MyServiceTest.php
PHPUnit 8.5.0 by Sebastian Bergmann and contributors.

.                                                                   1 / 1 (100%)

Time: 77 ms, Memory: 4.00 MB

OK (1 test, 1 assertion)
PS C:\src\php\general>

Whut the?

When we were looking at this I asked my colleague to simplify their tests a bit. The IRL version was testing this case via a data provider and test that was testing a bunch of things in a method that had some complexity issues, and I wanted to pare the situation back to just one simple test, testing this one thing. We ended up with this test:

public function testMyMethodViaInlineMock()
{
    $dependency = $this->createMock(MyDependency::class);
    $dependency
        ->expects($this->once())
        ->method('someMethod')
        ->willReturn("MOCKED VALUE");

    $service = new MyService($dependency);

    $result = $service->myMethod();

    $this->assertEquals("MOCKED VALUE", $result);
}


The difference here being we are not using a data provider any more, the mocking of the dependency is in the test. And this passes:

PS C:\src\php\general> vendor\bin\phpunit .\test\phpunit\expectsWithDataProviderIssue\MyServiceTest.php                 PHPUnit 8.5.0 by Sebastian Bergmann and contributors.

.                                                                   1 / 1 (100%)

Time: 61 ms, Memory: 4.00 MB

OK (1 test, 2 assertions)
PS C:\src\php\general>                                                                                                                              

Good, fine, but now we tested out the ->expects($this->exactly(10)) issue again:

PS C:\src\php\general> vendor\bin\phpunit .\test\phpunit\expectsWithDataProviderIssue\MyServiceTest.php                 PHPUnit 8.5.0 by Sebastian Bergmann and contributors.

F                                                                   1 / 1 (100%)

Time: 102 ms, Memory: 4.00 MB

There was 1 failure:

1) me\adamcameron\general\test\phpunit\expectsWithDataProviderIssue\MyServiceTest::testMyMethodViaInlineMock
Expectation failed for method name is "someMethod" when invoked 10 time(s).
Method was expected to be called 10 times, actually called 1 times.

FAILURES!
Tests: 1, Assertions: 2, Failures: 1.
PS C:\src\php\general>                                                                                                                                                    

This is more like it. But... erm... why?

I'd had "unexpected" behaviour previously when referring to $this inside a data provider method, because the data provider method is not called on the same instance of the test class that the tests are run with (why? Buggered if I know, but it is), and had my suspicions that this could be contributing to it. This is when we just shrugged and left the test as it was - working now - and I undertook to investigate some more later. The code I've been sharing here is the start of my investigation. I was at this point none the wiser though.

On a whim I tried another test variation, to see if there was any change in behaviour across the threshold of the count (so checking "off by one" situations). Here I have a new method that calls the same dependency method five times:
function myOtherMethod(){
     $this->dependency->someOtherMethod();
     $this->dependency->someOtherMethod();
     $this->dependency->someOtherMethod();
     $this->dependency->someOtherMethod();
    return  $this->dependency->someOtherMethod();
}
And a test for it that just checks what happens when we expect it to be called 3-7 times:

/** @dataProvider provideTestCasesForMyOtherMethodTests */
public function testMyOtherMethodCallsDependencyMethodEnoughTimes($dependency, $expectedCount) {
    $dependency
        ->expects($this->exactly($expectedCount))
        ->method('someOtherMethod')
        ->willReturn("MOCKED VALUE");

    $service = new MyService($dependency);

    $result = $service->myOtherMethod();

    $this->assertEquals("MOCKED VALUE", $result);
}

public function provideTestCasesForMyOtherMethodTests(){
    return [
        'it expects myOtherMethod to be called 3 times' => [
            'dependency' => $this->createMock(MyDependency::class),
            'expectedCount' => 3
        ],
        'it expects myOtherMethod to be called 4 times' => [
            'dependency' => $this->createMock(MyDependency::class),
            'expectedCount' => 4
        ],
        'it expects myOtherMethod to be called 5 times' => [
            'dependency' => $this->createMock(MyDependency::class),
            'expectedCount' => 5
        ],
        'it expects myOtherMethod to be called 6 times' => [
            'dependency' => $this->createMock(MyDependency::class),
            'expectedCount' => 6
        ],
        'it expects myOtherMethod to be called 7 times' => [
            'dependency' => $this->createMock(MyDependency::class),
            'expectedCount' => 7
        ]
    ];
}


All things being equal, I was expecting no failures here, given what we'd seen before. But... erm...

PS C:\src\php\general> vendor\bin\phpunit .\test\phpunit\expectsWithDataProviderIssue\MyServiceTest.php --filter=testMyOtherMethodCallsDependencyMethodEnoughTimes                                   PHPUnit 8.5.0 by Sebastian Bergmann and contributors.

FF...                                                               5 / 5 (100%)

Time: 64 ms, Memory: 4.00 MB

There were 2 failures:

1) me\adamcameron\general\test\phpunit\expectsWithDataProviderIssue\MyServiceTest::testMyOtherMethodCallsDependencyMethodEnoughTimes with data set "it expects myOtherMethod to be called 3 times" (Mock_MyDependency_6a23ba62 Object (...), 3)
me\adamcameron\general\phpunit\expectsWithDataProviderIssue\MyDependency::someOtherMethod() was not expected to be called more than 3 times.

C:\src\php\general\src\phpunit\expectsWithDataProviderIssue\MyService.php:21
C:\src\php\general\test\phpunit\expectsWithDataProviderIssue\MyServiceTest.php:59

2) me\adamcameron\general\test\phpunit\expectsWithDataProviderIssue\MyServiceTest::testMyOtherMethodCallsDependencyMethodEnoughTimes with data set "it expects myOtherMethod to be called 4 times" (Mock_MyDependency_6a23ba62 Object (...), 4)
me\adamcameron\general\phpunit\expectsWithDataProviderIssue\MyDependency::someOtherMethod() was not expected to be called more than 4 times.

C:\src\php\general\src\phpunit\expectsWithDataProviderIssue\MyService.php:22
C:\src\php\general\test\phpunit\expectsWithDataProviderIssue\MyServiceTest.php:59

FAILURES!
Tests: 5, Assertions: 3, Failures: 2.
PS C:\src\php\general>                   


OK so this is a bit perplexing. When the expected call count is less than the actual call count we get it failing correcty. But when it's more... no failure. Now I'm flummoxed. For good measure I also have a test not creating the dependency in the data provider, but instead inline in the test:

/** @dataProvider provideTestCasesForMyOtherMethodTests */
public function testMyOtherMethodCallsDependencyMethodEnoughTimesWithDependencyCreatedInTest($_, $expectedCount) {
    $dependency = $this->createMock(MyDependency::class);
    $dependency
        ->expects($this->exactly($expectedCount))
        ->method('someOtherMethod')
        ->willReturn("MOCKED VALUE");

    $service = new MyService($dependency);

    $result = $service->myOtherMethod();

    $this->assertEquals("MOCKED VALUE", $result);
}


And this one behaves correctly:

PS C:\src\php\general> vendor\bin\phpunit .\test\phpunit\expectsWithDataProviderIssue\MyServiceTest.php --filter=testMyOtherMethodCallsDependencyMethodEnoughTimesWithDependencyCreatedInTest                                                   PHPUnit 8.5.0 by Sebastian Bergmann and contributors.

FF.FF                                                               5 / 5 (100%)

Time: 67 ms, Memory: 4.00 MB

There were 4 failures:

1) me\adamcameron\general\test\phpunit\expectsWithDataProviderIssue\MyServiceTest::testMyOtherMethodCallsDependencyMethodEnoughTimesWithDependencyCreatedInTest with data set "it expects myOtherMethod to be called 3 times" (Mock_MyDependency_2707ab91 Object (...), 3)
me\adamcameron\general\phpunit\expectsWithDataProviderIssue\MyDependency::someOtherMethod() was not expected to be called more than 3 times.

C:\src\php\general\src\phpunit\expectsWithDataProviderIssue\MyService.php:21
C:\src\php\general\test\phpunit\expectsWithDataProviderIssue\MyServiceTest.php:99

2) me\adamcameron\general\test\phpunit\expectsWithDataProviderIssue\MyServiceTest::testMyOtherMethodCallsDependencyMethodEnoughTimesWithDependencyCreatedInTest with data set "it expects myOtherMethod to be called 4 times" (Mock_MyDependency_2707ab91 Object (...), 4)
me\adamcameron\general\phpunit\expectsWithDataProviderIssue\MyDependency::someOtherMethod() was not expected to be called more than 4 times.

C:\src\php\general\src\phpunit\expectsWithDataProviderIssue\MyService.php:22
C:\src\php\general\test\phpunit\expectsWithDataProviderIssue\MyServiceTest.php:99

3) me\adamcameron\general\test\phpunit\expectsWithDataProviderIssue\MyServiceTest::testMyOtherMethodCallsDependencyMethodEnoughTimesWithDependencyCreatedInTest with data set "it expects myOtherMethod to be called 6 times" (Mock_MyDependency_2707ab91 Object (...), 6)
Expectation failed for method name is "someOtherMethod" when invoked 6 time(s).
Method was expected to be called 6 times, actually called 5 times.

4) me\adamcameron\general\test\phpunit\expectsWithDataProviderIssue\MyServiceTest::testMyOtherMethodCallsDependencyMethodEnoughTimesWithDependencyCreatedInTest with data set "it expects myOtherMethod to be called 7 times" (Mock_MyDependency_2707ab91 Object (...), 7)
Expectation failed for method name is "someOtherMethod" when invoked 7 time(s).
Method was expected to be called 7 times, actually called 5 times.

FAILURES!
Tests: 5, Assertions: 6, Failures: 4.
PS C:\src\php\general>                                                                                                                                                                                                                                               

I started diving back through the PHPUnit code, and found the point where my two test variations began to demonstrate different behaviour. I edited InvocationHandler::verify to spit out some telemetry:

public function verify(): void
{
    printf(PHP_EOL . "InvocationHandler::verify called with %d matchers" . PHP_EOL, count($this->matchers));

    foreach ($this->matchers as $matcher) {
        $matcher->verify();
    }



And here's what I get running the two tests:

PS C:\src\php\general> vendor\bin\phpunit .\test\phpunit\expectsWithDataProviderIssue\MyServiceTest.php --filter=testMyMethodViaDataProvider                                                                                                    PHPUnit 8.5.0 by Sebastian Bergmann and contributors.

.                                                                   1 / 1 (100%)
InvocationHandler::verify called with 0 matchers


Time: 60 ms, Memory: 4.00 MB

OK (1 test, 1 assertion)
PS C:\src\php\general> vendor\bin\phpunit .\test\phpunit\expectsWithDataProviderIssue\MyServiceTest.php --filter=testMyMethodViaInlineMock                                                                                                      PHPUnit 8.5.0 by Sebastian Bergmann and contributors.

F                                                                   1 / 1 (100%)
InvocationHandler::verify called with 1 matchers


Time: 62 ms, Memory: 4.00 MB

There was 1 failure:

1) me\adamcameron\general\test\phpunit\expectsWithDataProviderIssue\MyServiceTest::testMyMethodViaInlineMock
Expectation failed for method name is "someMethod" when invoked 10 time(s).
Method was expected to be called 10 times, actually called 1 times.

FAILURES!
Tests: 1, Assertions: 2, Failures: 1.
PS C:\src\php\general>                             

Well. So when we run our tests using the mock created in the test code... we get our matcher "loaded" (for lack of a better term); when we run our test using the mock created in the data provider: it's nowhere to be seen. Interesting.

Looking further up the call stack, we find that this verify method is called from TestCase::verifyMockObects:
private function verifyMockObjects(): void
{
    foreach ($this->mockObjects as $mockObject) {
        if ($mockObject->__phpunit_hasMatchers()) {
            $this->numAssertions++;
        }

        $mockObject->__phpunit_verify(
            $this->shouldInvocationMockerBeReset($mockObject)
        );
    }


(I dunno what's going on with the method name there, but I did verify the call stack... PHPUnit is doing some special magic there).

verifyMockObjects in turn is called from runBare in the same class, and that is run from TestResult::run.

The key thing here is verifyMockObjects being run in the context of the TestCase object, and it looks at $this->mockObjects. Where does $this->mockObjects come from? When we create the mock:

public function getMock(): MockObject
{
    $object = $this->generator->getMock(
        $this->type,
        !$this->emptyMethodsArray ? $this->methods : null,
        $this->constructorArgs,
        $this->mockClassName,
        $this->originalConstructor,
        $this->originalClone,
        $this->autoload,
        $this->cloneArguments,
        $this->callOriginalMethods,
        $this->proxyTarget,
        $this->allowMockingUnknownTypes,
        $this->returnValueGeneration
    );

    $this->testCase->registerMockObject($object);

    return $object;
}

// ...

public function registerMockObject(MockObject $mockObject): void
{
    $this->mockObjects[] = $mockObject;
}

(remember that our createMock call is just a wrapper for a getMockBuilder()...->getMock() call).

So what's the bottom line here? Well my initial wariness about creating the mock in the data provider method has legs... when a mock is created it's registered in the TestCase object it's created within... and the one that the data provider method is running in is not the same TestCase as the one the test is run in. So... none of the mock's constraints are checked when the test is run. That's mostly it.

But what's the story with how that last test still failed on the variants with only 3/5 and 4/5 calls made? That check is done when the mocked method is actually called. So given this:

function myMethod() {
    return $this->dependency->someMethod();
}

When the mocked someMethod is called, a check is made then as to whether it's been called too many times for the expectation. So that's one within the mock itself at the time it's executed, hence it still "works". It's just the check that the exact number of call times was hit that is done at test level, not mock level if that makes sense.

This all just goes to show that one should just use data provider methods for providing data values for test variations, not to treat them like some sort of looping mechanism that the test is just a callback for. That's not the intent, and when one tries to be tricky... one is quickly found out to not be as clever as one hopes... ;-)

That's an awful lot of carry on for what ended up to be an easy / simple explanation. I'm mostly writing this up in case anyone else gets the same issue, and also to show how I went about troubleshooting and investigating it. I'll be putting this in front of my team shortly...

Righto.

--
Adam

Saturday 9 November 2019

I need to backtrack on some TDD guidance. I think

G'day:
Yes, this thing still exists.

Possibly foolishly my employer has made me tech lead of one of our software applications, and part of that is to lead from the front when it comes to maintaining code quality, including our TDD efforts. So I tend to pester the other devs about TDD techniques and the like. O how they love that. Some of them. Occasionally. Actually seldom. Poor bastards.

Anyway, recently I started to think about the difference between "fixing tests that would break if we made this new change to the code" and "writing a TDD-oriented test case before making the code change". Just to back up a bit... this is in the context of "write a failing test for the case being tested... then writing the code to pass the test". The red and green from the "red green refactor" trope. This started from doing code review and seeing existing tests being update to accommodate new code changes, but not see any actual new test case explicitly testing the new requirement. This did not sit well with me, so I've been asking the devs "where's yer actual test case for the change we're making here?"

To help with this notion I wrote a quick article on WTF I was on about. I was initially pleased with it and thought I'd nailed it, but when I asked my more-knowledgeable colleague Brian Sadler to sanity check it, he wasn't convinced I had nailed it, and moreover wasn't sure I was even right. Doh. I've been mulling over this for the rest of the week and I've come to the conclusion... yeah, he's right and I'm wrong. I think. I'm writing this article to put my thinking down in public, and see what you lot think. If anyone still reads this I mean. I will be chasing some ppl to read it, that said...

I can't just reproduce the original article cos it was an internal document and work owns it. So I'm reproducing a similar thing here.

Here's some sample code. I stress the code used here bears no relation to any code or situation we have in our codebase. It's just a simplified case to contextualise the discussion:

class ThingRepository {

    public function getThing($id) {
        $dao = DaoFactory::getThingDao();
        $dbData = $dao->getThing($id);

        return Thing::createFromArray($dbData);
    }
}

It gets raw data for the requested thing from a DAO and returns the data as a Thing object.

It also has an existing test:


class ThingRepositoryTest extends TestCase {

    private $repository;

    protected function setUp() : void {
        $this->repository = new ThingRepository();
    }

    public function testGetThingReturnsCorrectThingForId(){
        $testId = 17;
        $expectedName = "Thing 1";
        $thing = $this->repository->getThing(17);

        $this->assertInstanceOf(Thing::class, $thing);
        $this->assertSame($testId, $thing->id);
        $this->assertSame($expectedName, $thing->name);
    }
}

And this all works fine:

C:\src\php\general>vendor\bin\phpunit test\tdd\fixingExistingTests\ThingRepositoryTest.php
PHPUnit 8.4.3 by Sebastian Bergmann and contributors.

.                                                                   1 / 1 (100%)

Time: 54 ms, Memory: 4.00 MB

OK (1 test, 3 assertions)

C:\src\php\general>

If you were paying attention, you'll've spotted we're tightly coupling the DAO implementation here to its repository. At least we're using a factory, but still it is tightly coupled to the Repository implementation. We want to use dependency injection to provide the DAO to the repository.

Here's where my stance gets contentious.

Someone has addressed this requirement, and what they've done is updated that existing test to expect the passed-in DAO:

class ThingRepositoryExistingTestUpdatedTest extends TestCase {

    private $repository;
    private $dao;

    protected function setUp() : void {
        $this->dao = $this->createMock(ThingDao::class);
        $this->repository = new ThingRepositoryWithDaoDependency($this->dao);
    }

    public function testGetThingReturnsCorrectThingForId(){
        // implementation that now uses mocked DAO
    }
}

Note that this is exactly how I presented this information in the original work article. With the test implementation changes elided. This becomes relevant later.

Having prepped that test - and if fails nicely, cool - the dev then goes and sorts out the repo class.

I went on to reject that approach as not being valid TDD. My position is that "fixing other tests so they won't fail after we make the change" is not TDD, it's just technical debt recovery. If we're doing TDD, then we need to start with an explicit test case for the change we're going to make. I'll put this in a highlight box to draw attention to it:

Fixing other tests so they won't fail after we make the change is not TDD, it's just technical debt recovery. If we're doing TDD, then we need to start with an explicit test case for the change we're going to make.
This is what I'd expect to see:

class ThingRepositoryTest extends TestCase {

    private $repository;
    private $dao;

    protected function setUp() : void {
        $this->dao = $this->createMock(ThingDao::class);
        $this->repository = new ThingRepositoryWithDaoDependency($this->dao);
    }

    public function testGetThingUsesDaoPassedIntoConstructor(){
        $testId = 17;
        $mockedThingData = ['id'=>$testId, 'thing' => 'stuff'];

        $this->dao
            ->expects($this->once())
                        ->method('getThing')
                        ->with($testId)
                        ->willReturn($mockedThingData);

        $result = $this->repository->getThing($testId);

        $this->assertEquals(
            new Thing($mockedThingData['id'], $mockedThingData['thing']),
            $result
        )
    }
    
    // ... rest of test cases that were already there, including this one...

    public function testGetThingReturnsCorrectThingForID(){
        // updated implementation that doesn't break because of the DAO change
    }
}

Here we have an explicit test case for the change we're making. This is TDD. We're explicitly injecting a DAO into the test repo, and we're checking that DAO is being used to get the data.

This test is red:

C:\src\php\general>vendor\bin\phpunit test\tdd\fixingExistingTests\ThingRepositoryTest.php
PHPUnit 8.4.3 by Sebastian Bergmann and contributors.

F                                                                   1 / 1 (100%)

Time: 61 ms, Memory: 4.00 MB

There was 1 failure:

1) me\adamcameron\general\test\tdd\fixingExistingTests\ThingRepositoryTest::testGetThingUsesDaoPassedIntoConstructor
Expectation failed for method name is "getThing" when invoked 1 time(s).
Method was expected to be called 1 times, actually called 0 times.

FAILURES!
Tests: 1, Assertions: 4, Failures: 1.

C:\src\php\general>

But that means we're now good to make the changes to the repository:

class ThingRepository {

    private $dao;

    public function __construct($dao) {
        $this->dao = $dao;
    }

    public function getThing($id){
        $dbData = $this->dao->getThing($id);

        return Thing::createFromArray($dbData);
    }
}

Now our test is green, and we're done.

I rounded out the article with this:

As a rule of thumb... if you don't start your TDD work by typing in a case name - eg testGetThingUsesDaoPassedIntoConstructor - then you're not doing TDD, you are just doing maintenance code. These are... not the same thing.

Brian read the article and - my words not his - didn't get the distinction I was trying to make. Why was the first test update not sufficient? I could not articulate myself any differently than repeat what I'd said - no, not helpful - so we decided to get together later and look at how the implementation of the test I had elided would pan out, and how that wouldn't itself be a decent test. Due to work being busy this week we never had that confab, but I continued to think about it, and this morning concluded that Brian was right, and I was mistaken.

It comes down to how I omitted the implementation of that first test we updated. I did not do this on purpose, I just didn't want to show an implementation that was not helpful to the article. But in doing that, I never thought about what the implementation would be, and set-up a straw man to support my argument. I went back this morning and implemented it...

public function testGetThingReturnsCorrectThingForId(){
    $testId = 17;
    $expectedName = "Thing 1";

    $this->dao->expects($this->once())
        ->method('getThing')
        ->with($testId)
        ->willReturn(['id' => $testId, 'name' => $expectedName]);

    $thing = $this->repository->getThing(17);

    $this->assertInstanceOf(Thing::class, $thing);
    $this->assertSame($testId, $thing->id);
    $this->assertSame($expectedName, $thing->name);
}


This is what I'd need to do to that first test to get it to not break when we change the DAO to being injected. And... erm... it's the same frickin' logic as in the test I said we need to explicitly make. I mean I wrote the two versions at different times so solved the same problem slightly differently, but it's the same thing for all intents and purposes.

So... let's revisit my original position then:

Fixing other tests so they won't fail after we make the change is not TDD, it's just technical debt recovery. If we're doing TDD, then we need to start with an explicit test case for the change we're going to make.

It's not intrinsically true. Actually fixing existing tests can indeed be precisely the TDD-oriented cover we need.

I love being wrong. No sarcsm there: I really do. Now I'm thinking better about this topic. Win.

The only thing left after the distillation is the difference between these two:

Original test name (detailing its original test case requirement):

testGetThingReturnsCorrectThingForId

And my suggested case name for the new injected-DAO requirement:

testGetThingUsesDaoPassedIntoConstructor

Now I do still think there's merit in being that explicit about what we're testing. But it's well into the territory of "perfect is the enemy of good". We're fine just updating that original test.

I thought of how to deal with this, and came up with this lunacy:

public function testGetThingUsesDaoPassedIntoConstructor()
{
    $this->testGetThingReturnsCorrectThingForId();
}

But I think that's too pedantic even for me. I think we can generally just take it as given that the existing test covers us.



I'll close by saying I do think this is good advice when starting to do the testing for a code change to first come up with the test case name. It gets one into the correct mindset for approaching things in a TDD manner. But one oughtn't be as dogmatic about this as I clearly tend to be... existing tests quite possible could provide the TDD-oriented cover we need for code changes. We do still need to start with a breaking test, and the test does actually need to cover the case for the change... but it might be an existing test.

Oh and making the code change then fixing the tests that break as a result is not TDD. That's still wrong ;-)

I'm dead keen to hear what anyone might have to say about all this...

Righto.

--
Adam