Friday, 23 October 2020

Quickly running ColdFusion via Docker

G'day:
Firstly, don't take this as some sort of informed tutorial as to how things should be done. I am completely new to Docker, and don't yet know my arse from my elbow with it. Today I decided to look at an issue my mate was having with ColdFusion (see "ColdFusion: looking at an issue Mingo had with ehcache and cachePut and cacheGet"). These days I do not have CF installed anywhere, so to look into this issue, I needed to run it somehow. I was not gonna do an actual full install just for this. Also I've been trying - slowly -  to get up to speed with Docker, and this seemed to be a sitter for doing something newish with it. Also note that this is not the sort of issue I could just use trycf.com to run some code, as it required looking at some server config.

All I'm gonna do here is document what I had to do today to solve the problem in that other article.

I am running on Windows 10 Pro, I have WSL installed, and I'm running an Ubuntu distribution, and using bash within that. I could use powershell, but I so seldom use it, I'm actually slightly more comfortable with bash. For general Windows command-line stuff I usually just use the default shell, and it didn't even occur to me until just now to check if the docker CLI actually works in that shell, given all the docs I have read use either Powershell or some *nix shell. I just checked and seems it does work in the default shell too. Ha. Ah well, I'm gonna stick with bash for this.

I've also already got Docker Desktop & CLI installed and are running. I've worked through the Getting Started material on the Docker website, and that's really about it, other than a coupla additional experiments.

Right. I'd been made aware that Adobe maintain some Docker images for ColdFusion, but no idea where they are or that sort of jazz, so just googled it ("docker coldfusion image"). The first match takes me to "Docker images for ColdFusion", and on there are the exact Docker commands to run CF / code in CF in various ways. What I wanted to do was to run the ColdFusion REPL, and mess around with its config files. The closest option to what I wanted was to run ColdFusions built-in web server to serve a website. I figured it'd use an image with a complete ColdFusion install, so whilst I didn't want or need the web part of it, I could SSH into the container and use the REPL that way. So the option for me was this one:

Run ColdFusion image in daemon mode (that link is just a deep link to the previous one above). That gives the command:

docker run -dt -p 8500:8500 -v c:/wwwroot/:/app \
-e acceptEULA=YES -e password=ColdFusion123 -e enableSecureProfile=true \
eaps-docker-coldfusion.bintray.io/cf/coldfusion:latest

(I've just split that over multiple lines for readability).

Here we're gonna be running the container detached (-d), so once the container runs, control returns to my shell. I'm not at all clear why they're using the pseudo-TTY option here (-t / --tty). From all my reading (basically this Q&A on Stack Overflow: "Confused about Docker -t option to Allocate a pseudo-TTY"), it's more for when not in detached mode?

We're also exposing (publishing) the container's port 8500 to the host. IE: when I make HTTP requests to port 8500 in my PC's browser, it will be routed to the container (and CF's inbuilt web server in the container will be listening).

Lastly on that line we're mounting C:\wwwroot as a volume (-v) in the container as /app. IE: files in my PC's C:\wwwroot dir will be availed within the container in the /app dir.

The second line is just a bunch of environment variables (-e) that get set in the container... the ColdFusion installation process are expecting these I guess.

And the last line is the address of where the CF image is.

I'm not going to use exactly this command actually. I'm on Ubuntu so I don't have a C:\wwwroot, and I don't even care what's in the web root for this exercise (I'm just wanting the REPL, remember), so I'm just going to point it to a temp directory in my home dir (~/tmp). Also I see no reason to enable the secure profile. That's just a pain in the arse. Although it would probably not matter one way or the other.

Also I want to give my container a name (--name) for easy reference.

So here's what I've got:

docker run --name cf -dt -p 8500:8500 -v ~/tmp:/app \
-e acceptEULA=YES -e password=ColdFusion123 -e enableSecureProfile=false \
eaps-docker-coldfusion.bintray.io/cf/coldfusion:latest

Let's run that:

adam@DESKTOP-QV1A45U:~$ docker run --name cf -dt -p 8500:8500 -v ~/tmp:/app \
> -e acceptEULA=YES -e password=ColdFusion123 -e enableSecureProfile=false \
> eaps-docker-coldfusion.bintray.io/cf/coldfusion:latest
Unable to find image 'eaps-docker-coldfusion.bintray.io/cf/coldfusion:latest' locally
latest: Pulling from cf/coldfusion
b234f539f7a1: Pull complete
55172d420b43: Pull complete
5ba5bbeb6b91: Pull complete
43ae2841ad7a: Pull complete
f6c9c6de4190: Pull complete
b3121a6492e2: Pull complete
cf911c0e9bab: Pull complete
ef2cca3cfd53: Pull complete
f7a96a442bce: Pull complete
5f2632eaddf0: Pull complete
f4563a5d4acb: Pull complete
ad1e188590ee: Pull complete
Digest: sha256:12e7dc19bb642a0f67dba9a6d0a2e93f805de0a968c6f92fdcb7c9c418e1f1e8
Status: Downloaded newer image for eaps-docker-coldfusion.bintray.io/cf/coldfusion:latest
c7b703635a5358f0ea91c3f2549d8ec4a308812c44b2476bd6ad4c00b2e32c12
adam@DESKTOP-QV1A45U:~$

This takes a frew minutes to run cos the ColdFusion image is 570MB and it also downloads a bunch of other stuff. As it goes, you see progress like this:

...
ef2cca3cfd53: Pull complete
f7a96a442bce: Downloading [=================================================> ]  560.9MB/570.4MB
f7a96a442bce: Pull complete
...

In theory ColdFusion should now be installed and listening on http://localhost:8500, and...


Cool. I've had some issues running the REPL before if the CFAdmin installation step hasn't been run at least once, so I let that go (the password is the one from the environment variable, yeah? ColdFusion123).

I can see the image and the container in docker now too:

adam@DESKTOP-QV1A45U:~$
adam@DESKTOP-QV1A45U:~$ docker image ls
REPOSITORY                                        TAG                 IMAGE ID            CREATED             SIZE
eaps-docker-coldfusion.bintray.io/cf/coldfusion   latest              31a6d984cf30        2 months ago        1.03GB
adam@DESKTOP-QV1A45U:~$
adam@DESKTOP-QV1A45U:~$
adam@DESKTOP-QV1A45U:~$ docker container ls
CONTAINER ID        IMAGE                                                    COMMAND                  CREATED             STATUS                    PORTS                                         NAMES
c7b703635a53        eaps-docker-coldfusion.bintray.io/cf/coldfusion:latest   "sh /opt/startup/sta…"   10 minutes ago      Up 10 minutes (healthy)   8016/tcp, 45564/tcp, 0.0.0.0:8500->8500/tcp   cf
adam@DESKTOP-QV1A45U:~$
adam@DESKTOP-QV1A45U:~$

Next we need to SSH into the container.

Tip from Pete Freitag

In his comment below, Pete points out that given I'm doing all this on the same physical machine, I don't need to use ssh, I can just do this:

docker exec -it cf /bin/bash

And this works fine:

adam@DESKTOP-QV1A45U:~$ docker container ls
CONTAINER ID        IMAGE               COMMAND             CREATED             STATUS              PORTS               NAMES
adam@DESKTOP-QV1A45U:~$
adam@DESKTOP-QV1A45U:~$
adam@DESKTOP-QV1A45U:~$ docker start cf
cf
adam@DESKTOP-QV1A45U:~$ docker container ls
CONTAINER ID        IMAGE                                                    COMMAND                  CREATED             STATUS                             PORTS                                         NAMES
c7b703635a53        eaps-docker-coldfusion.bintray.io/cf/coldfusion:latest   "sh /opt/startup/sta…"   3 days ago          Up 13 seconds (health: starting)   8016/tcp, 45564/tcp, 0.0.0.0:8500->8500/tcp   cf
adam@DESKTOP-QV1A45U:~$
adam@DESKTOP-QV1A45U:~$
adam@DESKTOP-QV1A45U:~$ curl -s -o /dev/null -I -w "%{http_code}"  http://localhost:8500/CFIDE/administrator/
200
adam@DESKTOP-QV1A45U:~$
adam@DESKTOP-QV1A45U:~$
adam@DESKTOP-QV1A45U:~$ docker exec -it cf /bin/bash
root@c7b703635a53:/opt#
root@c7b703635a53:/opt#
root@c7b703635a53:/opt# sed -n '484,485'p /opt/coldfusion/cfusion/lib/ehcache.xml
        copyOnWrite="true"
            />
root@c7b703635a53:/opt#
root@c7b703635a53:/opt#
root@c7b703635a53:/opt# exit
exit
adam@DESKTOP-QV1A45U:~$


Here I show:
  • ColdFusion container not running
  • Starting the ColdFusion container
  • Showing the container started
  • Showing it is up and running and responding to requests with a 200
  • Using Pete's docker exec approach
  • Showing the change to the file I needed to make to get the caching to work as expected
  • Exiting the container's bash, back to my own PC's bash

It never occurred to me to try to do something like this, given traditionally I am never physically positioned on the boxes I am working on, so need to use ssh.

Cheers Pete!


For that I'm using some geezer called Jeroen Peters' docker-ssh container. I found this from a bunch of googling, and people saying how not do use SSH from within containers, and instead have the SSH server in a container of its own. Seems legit.

I've modified his docker command slightly to a) point to my cf container, also to detach once it's started:

adam@DESKTOP-QV1A45U:~$
adam@DESKTOP-QV1A45U:~$ docker run --detach -e FILTERS={\"name\":[\"^/cf$\"]} -e AUTH_MECHANISM=noAuth \
--name sshd-web-server1 -p 2222:22  --rm \
-v /var/run/docker.sock:/var/run/docker.sock \
jeroenpeeters/docker-ssh
5447950ce4d4d7ced8ef61696a64b8af9c4c01139c4fde58fa558ef25ba89b58
adam@DESKTOP-QV1A45U:~$
adam@DESKTOP-QV1A45U:~$

I can now SSH into the container:

adam@DESKTOP-QV1A45U:~$ ssh localhost -p 2222

 ###############################################################
 ## Docker SSH ~ Because every container should be accessible ##
 ############################################################### 
 ## container | /cf                                           ##
 ###############################################################

/opt $

I'm in! Cool!

Just to prove to myself further that I am actually "inside" the container, I jumped over to another bash session on my PC, and stuck a new file in that ~/tmp directory:

adam@DESKTOP-QV1A45U:~$ cd tmp
adam@DESKTOP-QV1A45U:~/tmp$ cat > new_file
G'day world
^Z
[2]+  Stopped                 cat > new_file
adam@DESKTOP-QV1A45U:~/tmp$ ll
total 20
drwxr-xr-x 3 adam adam 4096 Oct 23 19:10 ./
drwxr-xr-x 6 adam adam 4096 Oct 23 19:09 ../
drwxr-xr-x 4  999  999 4096 Oct 23 18:25 WEB-INF/
-rwxr-xr-x 1  999 root  370 Oct 23 18:24 crossdomain.xml*
-rw-r--r-- 1 adam adam   12 Oct 23 19:10 new_file
adam@DESKTOP-QV1A45U:~/tmp$
adam@DESKTOP-QV1A45U:~/tmp$ cat new_file
G'day world
adam@DESKTOP-QV1A45U:~/tmp$

And checked it from within the container:

/opt $ cd /app
/app $ # before

/app $ ll
total 16
drwxr-xr-x 3   1000   1000 4096 Oct 23 18:09 ./
drwxr-xr-x 1 root   root   4096 Oct 23 17:24 ../
drwxr-xr-x 4 cfuser cfuser 4096 Oct 23 17:25 WEB-INF/
-rwxr-xr-x 1 cfuser root    370 Oct 23 17:24 crossdomain.xml*
/app $
/app $ # after
/app $ ll
total 20
drwxr-xr-x 3   1000   1000 4096 Oct 23 18:10 ./
drwxr-xr-x 1 root   root   4096 Oct 23 17:24 ../
drwxr-xr-x 4 cfuser cfuser 4096 Oct 23 17:25 WEB-INF/
-rwxr-xr-x 1 cfuser root    370 Oct 23 17:24 crossdomain.xml*
-rw-r--r-- 1   1000   1000   12 Oct 23 18:10 new_file
/app $
/app $ cat new_file

G'day world

/app $

OK so I think I'm happy with that. So... now to run the code...

/app $ cd /opt/coldfusion/cfusion/bin/
/opt/coldfusion/cfusion/bin $ ./cf.sh
ColdFusion started in interactive mode. Type 'q' to quit.
cf-cli>foo = { bar = 1 };
cachePut( 'foobar', foo );

foo.bar = 2;

writeDump( cacheGet( 'foobar' ) );
struct

BAR: 1

cf-cli>
cf-cli>cf-cli>2
cf-cli>cf-cli>struct

BAR: 2

cf-cli>^Z
[1]+  Stopped                 ./cf.sh
/opt/coldfusion/cfusion/bin $

All this is discussed in the previous article (ColdFusion: looking at an issue Mingo had with ehcache and cachePut and cacheGet). This is showing the problem though... the answer should be 1 not 2

I have to fix this cache setting...

/opt/coldfusion/cfusion/bin $ cd ../lib
/opt/coldfusion/cfusion/lib $ ll ehcache.xml
-rwxr-xr-x 1 cfuser bin 25056 Jun 25  2018 ehcache.xml*
/opt/coldfusion/cfusion/lib $ vi ehcache.xml
bash: vi: command not found
/opt/coldfusion/cfusion/lib $

Erm... wat??

Wow OK so this is a really slimmed down container (except the 100s of MB of ColdFusion stuff I mean). I can fix this...

/opt/coldfusion/cfusion/lib $ sudo apt-get update
bash: sudo: command not found
/opt/coldfusion/cfusion/lib $

WAAAAAH. OK this is getting annoying now. On a whim I just tried without sudo

/opt/coldfusion/cfusion/lib $ apt-get update
Get:1 http://archive.ubuntu.com/ubuntu xenial InRelease [247 kB]
[etc]
Fetched 29.4 MB in 10s (2758 kB/s)
Reading package lists... Done
/opt/coldfusion/cfusion/lib $
/opt/coldfusion/cfusion/lib $ apt-get install vi
Reading package lists... Done
Building dependency tree
Reading state information... Done
E: Unable to locate package vi
/opt/coldfusion/cfusion/lib $

Fine. One step forward, one step sideways. Grumble. Another whim:

/opt/coldfusion/cfusion/lib $ apt-get install vim
Reading package lists... Done
Building dependency tree
Reading state information... Done
The following additional packages will be installed:
[heaps of stuff]
/opt/coldfusion/cfusion/lib $

Ooh! Progress. But... does it work? (hard to get proof of this as vi takes over the shell, but... yes it worked).

I now just have to restart the CF container, and test the fix:

/opt/coldfusion/cfusion/lib $ exit
exit
There are stopped jobs.
/opt/coldfusion/cfusion/lib $ exit
exit
Connection to localhost closed.
adam@DESKTOP-QV1A45U:~$ docker stop cf
cf
adam@DESKTOP-QV1A45U:~$ docker start cf
cf
adam@DESKTOP-QV1A45U:~$ ssh localhost -p 2222
 ###############################################################
 ## Docker SSH ~ Because every container should be accessible ##
 ###############################################################
 ## container | /cf                                           ##
 ###############################################################

/opt $ cd coldfusion/cfusion/bin/
/opt/coldfusion/cfusion/bin $ ./cf.sh
ColdFusion started in interactive mode. Type 'q' to quit.
cf-cli>foo = { bar = 1 };
cachePut( 'foobar', foo );

foo.bar = 2;

writeDump( cacheGet( 'foobar' ) );
struct

BAR: 1


cf-cli>
cf-cli>cf-cli>2
cf-cli>cf-cli>struct

BAR: 1

cf-cli>

WOOHOO! I managed to do something useful with Docker (and then reproduce it all again when writing this article). Am quite happy about that.

Final thing... if yer reading this and are going "frickin' hell Cameron, you shouldn't do [whatever] like [how I did it]", please let me know. This is very much a learning exercise for me.

Anyhow... this is the most work I've done in about four months now, and I need a... well... another beer.

Righto.

--
Adam

ColdFusion: looking at an issue Mingo had with ehcache and cachePut and cacheGet

 G'day:

Bloody Coldfusion. OK so why am I writing about a ColdFusion issue? Well about 80% of it is "not having a great deal else to do today", about 10% of being interested in this issue Mingo found. And 10% it being an excuse to mess around with Docker a bit. I am currently - and slowly - teaching myself about Docker, so there's some practise for me getting a ColdFusion instance up and running on this PC (which I no-longer have any type of CFML engine installed on).

OK so what's the issue. Mingo posted this on Twitter:


Just in case you wanna run that code, here it is for copy and paste:
foo = { bar = 1 };
cachePut( 'foobar', foo );

foo.bar = 2;

writeDump( cacheGet( 'foobar' ) );


Obviously (?) what one would expect here is {bar:1}. What gets put into cache would be a copy of the struct right?

Well... um... here goes...

/opt/coldfusion/cfusion/bin $ ./cf.sh
ColdFusion started in interactive mode. Type 'q' to quit.
cf-cli>foo = { bar = 1 };
struct
BAR: 1

cf-cli>cachePut( 'foobar', foo );
cf-cli>foo.bar = 2;
2
cf-cli>writeDump( cacheGet( 'foobar' ) );
struct
BAR: 2

cf-cli>

... errr... what?

It looks like ColdFusion is like only putting a reference to the struct into cache. So any code changing the data in the struct is changing it in CFML as well as changing it in cache. This does not seem right.

I did a quick google and found a ticket in Adobe's system about this: CF-3989480 - cacheGet returns values by reference. The important bit to note is that it's closed with


Not a great explanation from Adobe there, fortunately Rob Bilson had commented further up with a link to a cached version of an old blog article of his, explaining what's going on: "Issue with Ehcache and ColdFusion Query Objects". It's a slightly different situation, but it's the same underlying "issue". Just to copy and paste the relevant bit from his article:

Update: It looks like this is actually expected behavior in Ehcache. Unfortunately, it's not documented in the ColdFusion documentation anywhere, but Ehcache actually has two configurable parameters (as of v. 2.10) called copyOnRead and copyOnWrite that determine whether values returned from the cache are by reference or copies of the original values. By default, items are returned by reference. Unfortunately we can't take advantage of these parameters right now as CF 9.0.1 implements Ehcache 2.0.


I decided to have a look what we could do about this on ColdFusion 2018, hoping that its embedded implementation of Ehcache has been updated since Rob wrote that in 2010.

Firstly I checked the Ehcache docs for these two settings: copyOnWrite and copyOnRead. This is straight forward (from "copyOnRead and copyOnWrite cache configuration"):


<cache name="copyCache"
    maxEntriesLocalHeap="10"
    eternal="false"
    timeToIdleSeconds="5"
    timeToLiveSeconds="10"
    copyOnRead="true"
    copyOnWrite="true">
  <persistence strategy="none"/>
  <copyStrategy class="com.company.ehcache.MyCopyStrategy"/>
</cache>


The docs also confirm these are off by default

Next where's the file?

/opt/coldfusion $ find . -name ehcache.xml
./cfusion/lib/ehcache.xml
/opt/coldfusion $

Cool. BTW I just guessed at that file name.

So in there we have this (way down at line 471):


<!--
Mandatory Default Cache configuration. These settings will be applied to caches
created programmtically using CacheManager.add(String cacheName).

The defaultCache has an implicit name "default" which is a reserved cache name.
-->
<defaultCache
    maxElementsInMemory="10000"
    eternal="false"
    timeToIdleSeconds="86400"
    timeToLiveSeconds="86400"
    overflowToDisk="false"
    diskSpoolBufferSizeMB="30"
    maxElementsOnDisk="10000000"
    diskPersistent="false"
    diskExpiryThreadIntervalSeconds="3600"
    memoryStoreEvictionPolicy="LRU"
    clearOnFlush="true"
    statistics="true"
/>


That looked promising, so I updated it to use copyOnWrite:



<defaultCache
    maxElementsInMemory="10000"
    eternal="false"
    timeToIdleSeconds="86400"
    timeToLiveSeconds="86400"
    overflowToDisk="false"
    diskSpoolBufferSizeMB="30"
    maxElementsOnDisk="10000000"
    diskPersistent="false"
    diskExpiryThreadIntervalSeconds="3600"
    memoryStoreEvictionPolicy="LRU"
    clearOnFlush="true"
    statistics="true"
    copyOnWrite="false"
/>


Whatever I put into cache, I want it to be decoupled from the code immediately, hence doing the copy-on-write.

I restarted CF and ran the code again:

/opt/coldfusion/cfusion/bin $ ./cf.sh
ColdFusion started in interactive mode. Type 'q' to quit.
cf-cli>foo = { bar = 1 };
struct

BAR: 1

cf-cli>cachePut( 'foobar', foo );
cf-cli>foo.bar = 2;
2

cf-cli>writeDump( cacheGet( 'foobar' ) );

struct
BAR: 1

cf-cli>


Yay! We are getting the "expected" result now: 1

Don't really have much else to say about this. I'm mulling over writing down what I did to get ColdFusion 2018 up and running via Docker instead of installing it. Let's see if I can be arsed...

Righto.

-- 

Adam

Friday, 16 October 2020

Ben Brumm writes an interesting article on Hierarchical Data in SQL

G'day:

This is solely a heads-up regarding some potential reading for you.

A bloke called Ben Brumm came across one of my old articles, "CFML: an exercise in converting a nested set tree into an adjacency list tree". He's hit me up and asked me to add a link to that to an article he's written "Hierarchical Data in SQL: The Ultimate Guide". I've done that, but that article is buried in the mists of time back in 2015. I figured he could benefit from a more contemporary nudge too. So here it is. Me nudging.

In other news people have been urging me to get back to this blog, but I'm afraid I still am not finding anything new to write about. So... erm... yeah. If something leaps out at me I do fully intend to come back to this, but... not for now.

Righto.

-- 

Adam

Thursday, 9 April 2020

Aaaaah... *Me*

G'day
OK to continue this occasional series of commenting on ppl who have impacted my tenure at work, as they leave. Time to do another one.

Adam Cameron.

What a prick. Always nagging about doing TDD, always being pedantic about code reviews. Banging on about clean code. Asking for stuff to be refactored and simplified. Being stroppy. Then being even more stroppy.

So it's a bloody relief that after 10 years he's finally leaving, and getting out of everyone's hair.

Cameron is reluctantly leaving behind a whole bunch of people who have helped him grow as a professional (one way or another), enriched his work experience and who he genuinely admires. Both professionally and personally. He'll probably stay in touch with a bunch of them. And knowing him he'll be showing up in Porto to have beers with his squad as soon as he can.

He's been in a coupla good squads in his time, but this one that he's been leading for the last year is the best. He was lucky to get that lot as his first team as "Tech Lead". I doubt he'd've turned out reasonably OK at that role if it wasn't for them. The good thing is they hardly need leading, so will continue to do their excellent work even without him around.

10 years. Fuck me.

See ya.

--
Adam

Friday, 3 April 2020

Brian

G'day:
So this really sux.

For the bulk of the last decade I've been working alongside Brian Sadler. We first worked with each other when he joined hostelbookers.com when I'd been there for a coupla years, back in fuck-knows-when. I can recall thinking "shit... someone as old as I am, this should be interesting", in a room of devs (and managers) who were between 5-15 years younger than the both of us. Not that chronological experience necessarily means anything in any way that talent can be measured, but I had been used to being the oldest and most experienced geezer in the teams I'd been involved in. I've always been old, basically. So this new "Brian" guy seemed an interesting colleague to acquire.

My instinct was right.

[I've typed-in a paragraph of over-written shite four times now, and deleted the lot. I need to get to the point]

I have been a reasonably good programmer, technically. I'm OK with saying that.

What I've learned from Brian is that is only a small part of being a good team operative. I've always known and respected that programming is an act of communicating with humans, not the computer, but Brian really drove this home to me.

He introduced me to the concept of Clean Code.

He introduced me to the concept of TDD.

He schooled me in various concepts of refactoring, in that latter stage of Red Green Refactor. I'm still reading Martin Fowler's "Refactoring" as a result.

Every time I am looking at a code problem and I know I am not quite getting it, I hit him up and say "right, come on then... what am I missing...?" and he'll pull out a design pattern, or some OOP concept I'd forgotten about, or just has the ability to "see" what I can't in a code conundrum, and explain it to me.

Every time I ask "how can we make this code easier for our team to work with in future?", Brian's had the answer.

For a few years Brian's has been our Agile advocate, and listening to him and following his guidance has been an immeasurable benefit to my capabilities and my work focus.

I've also seen Brian help everyone else in my immediate team; other teams; and our leaders.

He's probably the most significant force for good I have had in my career.

He's also a really fuckin' good mate, for a lot of reasons that are not relevant for this sort of environment.

For reasons that are also irrelevant to this blog, today was the last day for the time being that Brian and I will be working with each other, and I am genuinely sad about that.

Genuinely sad.

Thanks bro.

--
Adam

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