Showing posts with label Unit Testing. Show all posts
Showing posts with label Unit Testing. Show all posts

Saturday, 27 August 2016

Friday puzzle: my PHP answer

G'day:
So as per my previous article - "Friday code puzzle" - here's my answer to the current puzzle.

I knocked the first version of this out over a beer whilst waiting for my flight to Galway, last night. Then scrapped that as being crap, and rewrote whilst in transit. And then... erm... back-filled my unit tests y/day evening. I feel guilty about not doing proper TDD,  but... so be it. It's got test coverage now.

Just on TDD for a second. It was wrong of me not to do the testing first, and whilst I was just about to manufacture an excuse as to why not ("I didn't have PHPUnit installed on this machine", "I couldn't be arsed making a whole project out of it", "um [something about being offline on the flight]"), it was all bullshit laziness. I had to write tests to prove that my function would work outside of the immediate quiz requirements: the easiest way to do this is with unit tests and clear expectations etc. These tests were of limited benefit to me, given I already had the code written, and had already been through the trial-and-error process of getting the thing working correctly.  The thing is, in a professional environment the tests aren't for you necessarily. They're for the next person who comes along to maintain your code. Or to troubleshoot your code when an edge-case crops up. It should not be down to the next person to write the tests for your code. That's lazy-arse, ego-centric shit. Do your job properly. I have encountered a coupla devs recently who think they're too good for tests, and refuse to do them properly. I don't want to work with people like that as they're all "me me me me".

But anyway... some code.

Well first a recap. Here's the "official" description from Jesse. But in summary:

We have this data set:

id,parentId,nodeText
1,,File
2,1,New
3,2,File
4,2,Folder
5,,Edit
6,5,Copy
7,5,Cut
8,5,Paste
9,,Help
10,9,About

This reflects hierarchical data (I would not represent a hierarchy that way, but that's a different story for another time), and we want to convert it to a hierarchical representation, eg:

[
  {
     nodeText : "File"
    ,children : [
      {
         nodeText : "New"
        ,children : [
           { nodeText: "File" }
          ,{ nodeText: "Folder" }
        ]
      }
    ]
  }
  ,{
     nodeText : "Edit"
    ,children : [
       {nodeText: "Copy"}
      ,{nodeText: "Cut"}
      ,{nodeText: "Paste"}
    ]
  }
  ,{
     nodeText : "Help"
    ,children : [
      { nodeText: "About" }
    ]
  }
]

There are a few "rules of engagement" at that link I mentioned, but that's the gist of it.

I decided I had better write some PHP code for a change, so knocked something together hastily. Initially I thought I might need to write some recursive monster to generate the hierarchy, but instead realised I did not need to do that, I just needed to track each "parent" node as I encountered it, and then append children to it as appropriate. Note that the data is sorted, so each record could be a parent of a subsequent node, and it will never be the case one will encounter a node before already having processed its parent. So all I needed to do is iterate over the data from top to bottom. Once. Nothing more tricky than that. Then by the time I had finished, the first parent would represent the entire tree. That was easy enough but then I had to convert it to JSON. Note the above representation is not JSON, but it's close enough, so that's what I decided to run with. As it turns out this was pretty easy to achieve in PHP as it has the ability to provide a custom serialiser for a class, and given I'd used a mix of associative and indexed arrays to build the data structure, it was simply a matter of returning the first parent node's children. Done.

Enough chatter, here's the code...

Update:

I have updated this from the first version I posted. This is slightly simpler, and also works even if the data is not pre-sorted. Thanks to Mingo for encouraging me to look into this.


<?php

namespace puzzle;

class Tree implements \JsonSerializable {

    private $parents = [];

    function __construct() {
        $tree = [
            "children" => []
        ];
        $this->parents[0] = $tree;
    }

    static function loadFromCsv($filePath) {
        $dataFile = fopen($filePath, "r");

        $tree = new Tree();
        while(list($id, $parent, $nodeText) = fgetcsv($dataFile)) {
            $tree->addNode($nodeText, $id, $parent);
        }

        return $tree;
    }

    private function addNode($nodeText, $id, $parent) {
        $parent = $parent === "" ? 0 : $parent;

        $this->parents[$id]["nodeText"] = $nodeText;
        $this->parents[$parent]["children"][] = &$this->parents[$id];
    }

    function jsonSerialize() {
        return $this->parents[0]["children"];
    }
}

The only other thing to note here is that the requirements indicated the data "should be in the form of an RDBMS resultset", but I could not be arsed horsing around with DB data for this, so I'm just reading a CSV file instead.

I also wrote the tests, as I said:

<?php

namespace test\puzzle;

use puzzle\Tree;

/** @coversDefaultClass \puzzle\Tree */
class TreeTest extends \PHPUnit_Framework_TestCase {

    private $testDir;

    function setup() {
        $this->testDir = realpath(__DIR__ . "/testfiles");
    }

    /**
     * @covers ::loadFromCsv
     * @covers ::__construct
     * @covers ::addNode
     * @covers ::jsonSerialize
     * @dataProvider provideCasesForLoadFromCsvTests
     */
    function testLoadFromCsv($baseFile){
        $files = $this->getFilesFromBase($baseFile);

        $result = Tree::loadFromCsv($files["src"]);
        $resultAsJson = json_encode($result);
        $resultAsArray = json_decode($resultAsJson);

        $expectedJson = file_get_contents($files["expectation"]);
        $expectedAsArray = json_decode($expectedJson);

        $this->assertEquals($expectedAsArray, $resultAsArray);
    }

    private function getFilesFromBase($base){
        return [
            "src" => sprintf("%s/%s/data.csv", $this->testDir, $base),
            "expectation" => sprintf("%s/%s/expected.json", $this->testDir, $base)
        ];
    }

    function provideCasesForLoadFromCsvTests(){
        return [
            "puzzle requirements" => ["testSet" => "puzzle"],
            "one element" => ["testSet" => "one"],
            "deep" => ["testSet" => "deep"],
            "flat" => ["testSet" => "flat"],
            "not ordered" =&gt ["testSet" =&gt "notOrdered"]
        ];
    }
}

There's no real surprise of discussion point there, beside highlighting the test cases I decided upon:

  • the puzzle requirements;
  • a single element;
  • a deep structure;
  • a flat structure.

I think those covered all the bases for a Friday Puzzle. An example of the data and expectation are thus (this is the "deep" example):

1,,Tahi
2,1,Rua
3,2,Toru
4,3,Wha
5,4,Rima
6,5,Ono
7,6,Whitu
8,7,Waru
9,8,Iwa
10,9,Tekau


[
  {
    "nodeText": "Tahi",
    "children": [
      {
        "nodeText": "Rua",
        "children": [
          {
            "nodeText": "Toru",
            "children": [
              {
                "nodeText": "Wha",
                "children": [
                  {
                    "nodeText": "Rima",
                    "children": [
                      {
                        "nodeText": "Ono",
                        "children": [
                          {
                            "nodeText": "Whitu",
                            "children": [
                              {
                                "nodeText": "Waru",
                                "children": [
                                  {
                                    "nodeText": "Iwa",
                                    "children": [
                                      {
                                        "nodeText": "Tekau"
                                      }
                                    ]
                                  }
                                ]
                              }
                            ]
                          }
                        ]
                      }
                    ]
                  }
                ]
              }
            ]
          }
        ]
      }
    ]
  }
]

All the rest are on GitHub.

I ran the code coverage report on the tests, and they're all green:


So that's that: surprisingly simple once I got into my head how to approach it.

Give it a blast. Reminder: the puzzle details are in this gist.

Righto.

--
Adam

Wednesday, 11 May 2016

JavaScript TDD: getting Mocha tests running on Bamboo

G'day:
We've finally having a chance to move forward with our JavaScript TDD / testing. You might have recalled that I'd lamented in the past we'd wanted to take this sort of thing more seriously with our JS, but it was taking an age to get it moving. Well now it's been placed in the hands of the devs, so it's... moving.

This came to us on Friday, and we had a wee faff around with it then, but didn't make a huge amount of progress. On the weekend I decided to get a proof of concept working, which I did on Saturday. It was kind of a bit of a mission, and I didn't write down what I did so I replicated it again on my other laptop this morning, and took some notes. Which I'll now pad out into an article.

We'd been looking at Jasmine for our JavaScript TDD, but this was based solely on that being the one JS testing framework I was aware of. Jasmine works really well, and superficially I like the approach it has (I say "superficially" cos that's the full extent of my exposure to it). However when I went to see how to get Bamboo running Jasmine tests, all I could really find was chatter about Mocha. I had a quick look at Mocha and it seemed much the same as Jasmine, and looked pretty good, so decided to shift my attention onto using that instead. It seems like Bamboo was kinda pushing me in that direction anyhow.

Installing Mocha is done via NPM:

C:\temp>npm install -g mocha
npm WARN deprecated jade@0.26.3: Jade has been renamed to pug, please install the latest version of pug instead of jade
npm WARN deprecated graceful-fs@2.0.3: graceful-fs v3.0.0 and before will fail on node releases >= v7.0. Please update to graceful-fs@^4.0.0 as soon as possible. Use 'npm ls graceful-fs' to find it in the tree.
C:\Users\adam.cameron\AppData\Roaming\npm\mocha -> C:\Users\adam.cameron\AppData\Roaming\npm\node_modules\mocha\bin\mocha
C:\Users\adam.cameron\AppData\Roaming\npm\_mocha -> C:\Users\adam.cameron\AppData\Roaming\npm\node_modules\mocha\bin\_mocha
mocha@2.4.5 C:\Users\adam.cameron\AppData\Roaming\npm\node_modules\mocha
├── escape-string-regexp@1.0.2
├── commander@2.3.0
├── diff@1.4.0
├── growl@1.8.1
├── supports-color@1.2.0
├── debug@2.2.0 (ms@0.7.1)
├── mkdirp@0.5.1 (minimist@0.0.8)
├── jade@0.26.3 (commander@0.6.1, mkdirp@0.3.0)
└── glob@3.2.3 (inherits@2.0.1, graceful-fs@2.0.3, minimatch@0.2.14)

(don't worry about the deprecation warnings, everything still works... this is all just part of a trademark dispute).

And similarly install Chai and bamboo-mocha-reporter.

Another thing I needed to do is to make sure I had my NODE_PATH environment variable set. I was kinda expecting the Node install to do this, but it didn't. For me that needs to point to C:\Users\adam.cameron\AppData\Roaming\npm\node_modules

So a basic Mocha test looks like this:

var assert = require('chai').assert;

describe('MyClass tests', function() {
    var MyClass = require("../myApp/MyClass");
    describe('Baseline', function () {
        it('should exist and be usable', function () {
            var myObj = new MyClass();
            assert.instanceOf(myObj, MyClass, "myObj should be an instance of MyClass");
        });
    });
});

This is a tweaking of the sample they had on their website's "getting started" section:

var assert = require('chai').assert;
describe('Array', function() {
  describe('#indexOf()', function () {
    it('should return -1 when the value is not present', function () {
      assert.equal(-1, [1,2,3].indexOf(5));
      assert.equal(-1, [1,2,3].indexOf(0));
    });
  });
});

Note that the instructions there don't actually implicitly mention the dependency on Chai here, which is a bit crap of them. They seem to kinda assume one knows this. This is not a safe assumption to make.

Another thing that is "interesting" about Mocha is that it doesn't come with any assertion capabilities at all, so one needs to install Chai (or some other assertions lib) for it to be of much use. I guess this is symptomatic of this mania Node apps have for making everything a "dependency" even if it's really more of a necessity. Equally I discovered that Mocha has no mocking framework either (which almost makes the name of the thing a misrepresentation! ;-), so I think I'll be using Sinon for that. But later.

But anyhow, one one installs all that crap, one can run a test.

I've created a basic GitHub repo, JavaScriptTesting, which has a basic class and a basic test of it:

var Person = function(firstName, lastName) {
    this.firstName = firstName;
    this.lastName = lastName;
};

Person.prototype.getFullName = function(){
    return this.firstName + " " +this.lastName;
};

module.exports = Person;


var assert = require('chai').assert;

describe('Person tests', function() {
    var Person = require("../myApp/Person");
    describe('Baseline', function () {
        it('should exist and be usable', function () {
            var person = new Person();
            assert.instanceOf(person, Person);
        });
    });
    describe('Method tests', function () {
        describe('getFullName tests', function () {
            it('should return a full name', function () {
                var person = new Person("FIRST_NAME", "LAST_NAME");
                var fullName = person.getFullName();
                assert.equal("FIRST_NAME LAST_NAME", fullName);
            });
        });
    });
});

And I can now run these from the command prompt:

C:\src\JavaScriptTesting>mocha


  Person tests
    Baseline
      V should exist and be usable
    Method tests
      getFullName tests
        V should return a full name


  2 passing (47ms)


C:\src\JavaScriptTesting>

Cool. So I know the tests work.

Now I go about getting it all to work on Bamboo. I'd not installed this before so I followed the instructions. One pitfall I fell foul of the first time is the requirements state:

Servlet container requirements
You will need a servlet container that supports the Servlet 2.4 specification. Most modern containers should comply with this.

This is from "Bamboo installation guide". So I messed around getting Tomcat installed (which granted is not much messing around), only to find out that's bullshit. Bamboo installs its own instance of Tomcat, and does that out of the box as part of the installer. Oh well: this is not much of a hardship.

I knew Bamboo will be needing to write stuff to disk and do other stuff that the local system account perhaps did not have permissions to do, so I created a specific account for Bamboo (I called it "bamboo" with a password of "bamboo" ;-), and gave that user full access permissions to the Bamboo home directory.

Oh... during install I let Bamboo create its "home" (/working) directory in the default location which - in hindsight was daft both on my part, and on part of the Bamboo installer, cos it created it in my own user account's user directory. That makes no sense for a server-oriented application. So I changed this to be c:\bamboo.tmp (I'll be blowing all this away on this machine once I've done with this article). So it was that dir I gave the Bamboo user full control permissions to.

After that I dropped down to a command prompt (running as administrator) and ran c:\apps\atlassian\bamboo\InstallAsService.bat to install the Windows service, then I edited that to login with the new bamboo account and started it.

Browsing to http://localhost:8085/ yielded success! (I can't show you a screen shot as I forgot to take one at the time, and I cannot get back to that first screen again now. Just tryst me ;-)

Next Bamboo needs some configuration, so I chose the Express object, which just requires me to enter a login and a password and an email address. Then I was in, and it was time to create a new job to run my tests.

I really have no idea what's going on in Bamboo, but I kinda followed my nose.

I created a "project" called "JavaScript App" which was assigned a short code of "JA". As far as I can tell this is just like in Jira when one creates a new project, and it gets that abbreviation which ends up as the prefix to all tickets. But I dunno. I just agreed with what Bamboo suggested. Next I created a "plan" called "JS TDD CI", and that got an abbrev. of "JTC" (f*** knows if any of this will ever be relevant again, but it's what was going on on the screen as I was thinking "yes, lovely, that's nice").



Then we started to do something that seemed useful: I linked the plan to a repository which is where I point Bamboo to my GitHub repository (that JavaScriptTesting one I mentioned further up). All good.




On the next screen I add the other steps to the plan. It's already added the "Source Code checkout" one for me. So running that much would get the source code checked out.

Next I need to tell Bamboo about Mocha and Chai (and its own mocha-bamboo-reporter), so I add three NPM tasks. Here's the screens for adding the mocha one: the others are all the same:





Having done that, I then add the task to run the tests, which is done via the Mocha Test Runner:



And that's it. I can now run the tests by selecting the run option from the top right of the main plan screen. Stuff whirs into action (and it logs an awful lot of stuff), and eventually:



Now I dart back to my other machine and add a new test to my code:

describe('getAgeInYears tests', function () {
    it('should return the correct age in years', function () {
        var person = new Person("FIRST_NAME", "LAST_NAME", new Date("1970-02-17"));
        var ageInYears = person.getAgeInYears();
        assert.equal(46, ageInYears); // obviously this is a poorly-written test, but it works for a few months yet ;-)
    });
});

This fails cos I have not added the getAgeInYears method yet, but for the sake of demonstration I'll push it up to GitHub.

Bamboo is polling GitHub every three minutes, so after a bit a test run starts and...




There's my latest changes breaking the build. I've never been so pleased to see a failing test before. Hang on whilst I implement that method...



Cool.

Now I did have some problems, and I'm not that happy with a coupla things. Firstly I've got all those NPM modules installed globally already, and I have them on my NODE_PATH. But Bamboo refuses to "see" them. It only seems interested in looking in the node_modules dir in its own working directory. I could not work out how to change that.

I did have some success initially with doing an npm link instead of an npm install in my test script, but when I came to test everything today I must have screwed something up as it had stopped working. Using the install approach requires dragging the stuff down off the 'net each time I do a test run, which seems a bit wasteful. I'm sure it's something I'm doing wrong, and when I work it out, I'll update this article accordingly.

Having got this proof of concept running, we are now in the position to give it a go in our actual work environment (which we've done... we've got our first test passing).

I'm pretty pleased about this. I am now in a place with my JavaScript development that I should have been (and wanted to be) about three years ago. But at least I've got there.

Righto.

--
Adam

Thursday, 7 April 2016

Maintaining untested code, and mocking too much when backfilling test (with JavaScript & Jasmine)

G'day:
Here's another example of me doing something wrong. Well: there's a litany of wrongness in this exercise, and I'll show the whole thing from start to finish. But this all stems from me ballsing something up.

Caveat

Be warned: there's a lot of code in this one, but the main point is the volume of the code I found necessary to write to test and fix a tiny bug. You don't need to worry too much about the content of the code... the important part is the line number at the bottom of the file.

The background to this is that I was helping a former colleague with some CFML code ("just when I thought I was out..."), and it was whilst reconfiguring stuff, I got hit by a trivial bug... the resolution to which is the basis of this article. I could not use their actual code in the article here due to NDA reasons, but I also didn't want to be discussing CFML either. So I've undertaken to "refactor" the entire thing into JavaScript instead (I don't mean their code; I mean a version of their logic which I will discuss here). The JavaScript code closely resembles the actual code I was looking at, but obviously some of the approaches to doing things needed to change, and I've repurposed some of the elements of it to better suit what I'm discussing. The logic and "code distribution" is about a 95% match to the original case though, I think. Also I'm using Jasmine to do JavaScript testing in this example, and I was using a CFML-based solution for the original code. This is irrelevant to the situation as both work mostly the same way, and produce similar amounts of code for the equivalent testing and mocking tasks.

I will start by saying that the code is a product of its time and their coding policies at that time. I'm happy to say they've since revised their approach to things, and are fine-tuning it as they go. the code base - like all others - is an evolving beast. And indeed to be fair to them: it's a testament to their evolving coding process that I'm able to write this article.

Background

This code comes some a project which is built with an MVC framework: the details of which are irrelevant to this article. They also use a dependency injection container to handle their dependencies.

The code in this article is part of the TranslatorProvider, which creates a TranslationService, and loads it with translations originally stored in some (optionally cached) data repository. The implementation details of these are not relevant here. These translations are for on-screen labels and other text. The bit of code I'm focusing on in this article is the logic to load the translations from storage. They don't quite follow the service / repository model I have presented here, but it's the same beast by different names, and the service / repository model suits me better so that's how I've labelled things here.

My mate's organisation produces numerous small websites, all of which are multi-lingual, and all share this code.

New site

They're in the process of building a new site, which is where I came in. We'd been piggy-backing onto the translation dictionaries for one of the existing sites whilst we were protoyping. the onscreen-labels we need to translate follow the same general theme, so it's workable - during development - to use one site's translations for another site, to a point (some translations don't work, but that's OK). It took a while to prepare the dictionaries for the new site (a different team was doing that), but when they came back from the translator I switched the dictionary bundles over.

I did this, and the whole site broke. Switched the dictionary back to the older one: site worked again. Switch it forward: site broke. OK: there's something wrong with our translations. Oh, I switched the translations on another one of their test sites - one I knew in which all the work was sound (ie: not written by me) - and that site broke, Harrumph. What is it about some translation dictionaries that can break a site?

Bug

I dug into the translator code and quickly (well: it took a few hours) found the problem:

var translations;

// ...

for (var key in rawTranslations.primary){
    translations.primary = translations.primary || {};
    translations.primary[key] = rawTranslations.primary[key];
}

for (var key in rawTranslations.secondary){
    translations.secondary = translations.secondary || {};
    translations.secondary[key] = rawTranslations.secondary[key];
}

var ttl = config.ttl;

cacheService.put(cacheKeyPrimary, translations.primary, ttl);
cacheService.put(cacheKeySecondary, translations.secondary, ttl);


Can you see it?

What happens if - as it is in our case - one hasn't got any secondary translations? translations.secondary never gets created, so that last line will break.

The fix is simply to initialise that object outside of the loop:

translations = {primary : {}, secondary : {}};

Simple.

Well except for one thing. No test coverage. This code was written before they had a TDD policy, so it's got no tests at all. Note that this bug never would have happened had they used TDD when developing this code, because the very first test would have been with empty arrays. I made sure to point this out to them, but they rightfully pointed out when it was written and they now do have a full TDD policy. And that comes into play for my fix: I needed to use TDD for implementing my fix.

Back-fill

My policy with maintaining untested code using our new TDD approach is that I will not back-fill all missing tests. I will only implement the minimum testing needed for the case I'm fixing. if I have time I'll make shell tests with a "skip" in them so the missing coverage gets flagged in the test run, but I'll not bother too much other than that.

So in this case all I need to test is that if the secondary object is null, that null gets send to the cache. Simple.

Or is it?

Unclean code

Unfortunately this code was also written before the era of "write Clean Code", so the function itself is doing way too much. It's 50 lines long for a start, which is a red flag, but one can easily see different sections of processing in it too, so it violates the "functions should do one thing" rule. This becomes a problem because even to test my wee fix, I need to make sure the preceding 40-odd lines of code don't break or do the wrong thing before I get to my line of code. And as this code leverages a fair number of dependencies, there's a bunch of mocking work to do before I can even get to what I want to test. Note: the dependencies in the original code were handled differently in that they passed the DI container into this provider, and just plucked out whatever they need when they need it. I cannot bring myself to show code like that (even for a blog article), so I've broken them down into multiple dependencies with specific roles. The logic is the same, and it produces the same amount of code (both sample code and test code).

If the code was Clean and I was just looking at a function that map the source objects(s) into the target objects(s), then my test job would be easy. But no.

Let's look at what stands between me and where I can test the results of my line of code. I've highlighted what needs mocking:

if (!requestService.isTranslatorEnabled()){
    return;
}

var currentLocale = requestService.getLocale();

var cacheKeyPrimary = getCacheKey("primary", currentLocale);
var cacheKeySecondary = getCacheKey("secondary", currentLocale);

var okToGetFromCache = cacheService.isActive()
    && cacheService.exists(cacheKeyPrimary)
    && cacheService.exists(cacheKeySecondary);

var translations = {};
if (okToGetFromCache) {
    translations.primary = cacheService.get(cacheKeyPrimary);
    translations.secondary = cacheService.get(cacheKeySecondary);
}else{
    var rawTranslations = {
        primary : translationRepository.loadBundle("primary", currentLocale),
        secondary : translationRepository.loadBundle("secondary", currentLocale)
    };

    for (var key in rawTranslations.primary){
        translations.primary = translations.primary || {};
        translations.primary[key] = rawTranslations.primary[key];
    }

    for (var key in rawTranslations.secondary){
        translations.secondary = translations.secondary || {};
        translations.secondary[key] = rawTranslations.secondary[key];
    }

    var ttl = config.ttl;

    cacheService.put(cacheKeyPrimary, translations.primary, ttl);
    // [...]

  1. requestService.isTranslatorEnabled
  2. requestService.getLocale
  3. cacheService.isActive
  4. translationRepository.loadBundle
  5. config.ttl
  6. cacheService.put
But that's not all. The function has to complete without error. So I also need to mock this lot:

var translationService = translationFactory.getTranslator();
var arrayLoader = translationFactory.getArrayLoader();

translationService.load(arrayLoader.load(translations.primary), 'primary', currentLocale);
translationService.load(arrayLoader.load(translations.secondary), 'secondary', currentLocale);

  1. translationFactory.getTranslator
  2. translationFactory.getArrayLoader
  3. arrayLoader.load
  4. translationService.load

What a nightmare. If this code was written cleanly, I'd simply have to test a method that takes a coupla arguments and returns a value. Simple.

Tuesday, 8 March 2016

Unit testing: mocking out final, type-checked dependency

G'day:
This is a follow-on from my earlier article: "PHPUnit: trap for dumb players regarding mocking". In that article I described how I fell foul of trying to mock a method that had been declared final, and - basically - wasting time from a mix of me not thinking to RTFM, validating my assumptions, and PHPUnit not being as declarative with its feedback as it could be.

Recap of previous article

To recap/augment the situation, I had a Logger class like this:

namespace me\adamcameron\mocking\service;

use me\adamcameron\mocking\exception\NotImplementedException;

class LoggingService {

    public final function logSomething($text){
        throw new NotImplementedException(__FUNCTION__ . " not implemented yet");
    }

}

And we have a service using that Logger as a dependency:

namespace me\adamcameron\mocking\service;

class MyService {

    private $logger;

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

    public function doesStuff($value){
        $preppedValue = "PREPPED $value";
        $processedValue = $this->doesRiskyStuff($preppedValue);
        $this->logger->logSomething($processedValue);
        return "RESULT OF DOING STUFF ON $processedValue";
    }

    protected function doesRiskyStuff($preppedValue){
        return "$preppedValue (SURVIVED RISKY PROCESS)";
    }

}

We've established we cannot mock that dependency because I cannot mock that final method.

Final prevents mocking

Our tactic to work around this was to create a different stubbed LoggingService which has the same methods, but not the final restriction:

namespace me\adamcameron\mocking\stub;

use me\adamcameron\mocking\exception\StubMethodCalledException;

class LoggingService {

    public function logSomething($text){
        throw new StubMethodCalledException(__FUNCTION__ . " must be mocked");
    }

}

And then we use StubMethodCalledException for our mocked dependency:

class MyServiceTest extends \PHPUnit_Framework_TestCase {

    // ...

    function setup(){
        $mockedLogger = $this->getMockedLogger();
        $this->myService = $this->getTestMyService($mockedLogger);
    }

    // ...

    function getMockedLogger(){
        $mockedLogger = $this->getMockBuilder('\me\adamcameron\mocking\stub\StubbedLoggingService')
            ->setMethods(["logSomething"])
            ->getMock();

        $mockedLogger->expects($this->once())
            ->method("logSomething")
            ->with("MOCKED RESPONSE FROM DOESRISKYSTUFF");

        return $mockedLogger;
    }

    // ...

}

Type-checking can be unhelpful

And now we run our tests and...


C:\src\php\php.local\www\experiment\phpunit\mock>phpunit
PHPUnit 4.8.23 by Sebastian Bergmann and contributors.

.E

Time: 764 ms, Memory: 5.00Mb

There was 1 error:

1) me\adamcameron\mocking\test\service\MyServiceTest::testDoesStuff
Argument 1 passed to me\adamcameron\mocking\service\MyService::__construct() must be an instance of me\adamcameron\mocking\service\LoggingService, instance of Mock_StubbedLoggingService_e4bcfaaf given

C:\src\php\php.local\www\experiment\phpunit\mock\src\service\MyService.php:9
C:\src\php\php.local\www\experiment\phpunit\mock\test\service\MyServiceTest.php:44
C:\src\php\php.local\www\experiment\phpunit\mock\test\service\MyServiceTest.php:16
C:\Users\adam.cameron\AppData\Roaming\Composer\vendor\phpunit\phpunit\src\TextUI\Command.php:149
C:\Users\adam.cameron\AppData\Roaming\Composer\vendor\phpunit\phpunit\src\TextUI\Command.php:100

FAILURES!
Tests: 2, Assertions: 0, Errors: 1.

C:\src\php\php.local\www\experiment\phpunit\mock>


Ah FFGS.

Indeed, here's that constructor:

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

Note how it type-checks the $logger argument.

For web application code, I would simply never bother type checking arguments. PHP is a dynamic, run-time compiled language, so type-checking really makes little sense in most situations. In this situation it's one of our API libraries, so it kinda makes sense. Normally I'd go "an API should typecheck as much as possible", but to me that only applies to APIs which will be used as a third-party module. For internal libraries - even shared ones - it's just boilerplate for the sake of it, IMO.


This is important, so I'm gonna make it bold:

That aside, in a situation you know the library is going to be used as a dependency of other code - especially code you know is going to be using TDD and dependency injection - do not type check on an implementation. Use an interface.

If we'd used an interface here, I could still use my stub, just by saying it implements the interface (which it does already!).

Saturday, 5 March 2016

CFML: go have a look at a Code Review

G'day:
I've already put more effort into this than a Saturday morning warrants, so I'll keep this short and an exercise in copy and past.

Perennial CFMLer James Mohler has posted some code on Code Review:  "Filtering the attributes for a custom tag". I'll just reproduce my bit of the review, with his code by way of framing.

Go have a look, and assess our code as an exercise.

James's functions:

Original:

string function passThrough(required struct attr)   output="false"  {

    local.result = "";

    for(local.myKey in arguments.attr)    {
        if (variables.myKey.left(5) == "data-" || variables.myKey.left(2) == "on" || variables.myKey.left(3) == "ng-")  {

            local.result &= ' #local.myKey.lcase()#="#arguments.attr[local.myKey].encodeForHTMLAttribute()#"';
        } // end if 
    }   // end for

    return local.result;
}

His reworking:

string function passThrough(required struct attr)   output="false"  {

    arguments.attr.filter(
        function(key, value) { 
            return (key.left(5) == "data-" || key.left(2) == "on" || key.left(3) == "ng-");
        }
    ).each(
        function(key, value)    {
            local.result &= ' #key.lcase()#="#value.encodeForHTMLAttribute()#"';
        }
    );  

    return local.result;
}

Now for my code review (this is a straight copy and paste from my answer):

OK, now that I've had coffee, here's my refactoring:

function extractCodeCentricAttributesToMarkupSafeAttributes(attributes){
    var relevantAttributePattern = "^(?:data-|ng-|on)(?=\S)";

    return attributes.filter(function(attribute){
        return attribute.reFindNoCase(relevantAttributePattern);
    }).reduce(function(attributeString, attributeName, attributeValue){
        return attributeString& ' #attributeName#="#attributeValue#"';
    }, "");
}


Notes on my implementation:
  • I could not get TestBox working on my ColdFusion 2016 install (since fixed), so I needed to use CF11 for this, hence using the function version of encodeForHTMLAttribute(). The method version is new to 2016.
  • we could probably argue over the best pattern to use for the regex all day. I specifically wanted to take a different approach to Dom's one, for the sake of comparison. I'm not suggesting mine is better. Just different. The key point we both demonstrate is don't use a raw regex pattern, always give it a meaningful name.
  • looking at the "single-expression-solution" I have here, the code is quite dense, and I can't help but think Dom's approach to separating them out has merit.

Code review notes:
  • your original function doesn't work. It specifies variables.myKey when it should be local.myKey. It's clear you're not testing your original code, let alone using TDD during the refactoring process. You must have test coverage before refactoring.
  • the function name is unhelpfully non-descriptive, as demonstrated by Dom not getting what it was doing. I don't think my function name is ideal, but it's an improvement. I guess if we knew *why
  • you were doing this, the function name could be improved to reflect that.
  • lose the scoping. It's clutter.
  • lose the comments. They're clutter.
  • don't abbrev. variable names. It makes the code slightly hard to follow.
  • don't have compound if conditions like that. It makes the code hard to read. Even if the condition couldn't be simplified back to one function call and you still needed multiple subconditions: extract them out into meaningful variable names, eg: isDataAttribute || isOnAttribute || isNgAttribute
  • key and value are unhelpful argument names
  • slightly controversial: but unless it's an API intended to be used by third-parties: lose the type checking. It's clutter in one's own code.
  • there's no need for the output modifier for the function in CFScript.
  • don't quote boolean values. It's just false not "false".

Unit tests for this:

component extends="testbox.system.BaseSpec" {

    function beforeAll(){
        include "original.cfm";
        include "refactored.cfm";
        return this;
    }

    function run(testResults, testBox){
        describe("Testing for regressions", function(){
            it("works with an empty struct", function(){
                var testStruct = {};
                var resultFromOriginal = passThrough(testStruct);
                var resultFromRefactored = extractCodeCentricAttributesToMarkupSafeAttributes(testStruct);
                expect(resultFromRefactored).toBe(resultFromOriginal);
            });
            it("works with an irrelevant attribute", function(){
                var testStruct = {notRelevant=3};
                var resultFromOriginal = passThrough(testStruct);
                var resultFromRefactored = extractCodeCentricAttributesToMarkupSafeAttributes(testStruct);
                expect(resultFromRefactored).toBe(resultFromOriginal);
            });
            it("works with each of the relevant attributes", function(){
                var relevantAttributes = ["data-relevant", "onRelevant", "ng-relevant"];
                for (relevantAttribute in relevantAttributes) {
                    var testStruct = {"#relevantAttribute#"=5};
                    var resultFromOriginal = passThrough(testStruct);
                    var resultFromRefactored = extractCodeCentricAttributesToMarkupSafeAttributes(testStruct);
                    expect(resultFromRefactored).toBe(resultFromOriginal);
                }
            });
            it("works with a mix of attribute relevance", function(){
                var testStruct = {notRelevant=7, onRelevant=11};
                var resultFromOriginal = passThrough(testStruct);
                var resultFromRefactored = extractCodeCentricAttributesToMarkupSafeAttributes(testStruct);
                expect(resultFromRefactored).toBe(resultFromOriginal);
            });
            it("works with multiple relevant attributes", function(){
                var testStruct = {"data-relevant"=13, onRelevant=17, "ng-relevant"=19};
                var resultFromOriginal = passThrough(testStruct);
                var resultFromRefactored = extractCodeCentricAttributesToMarkupSafeAttributes(testStruct);
                expect(resultFromRefactored).toBe(resultFromOriginal);
            });
        });

    }

}


Use them. NB: the includes in beforeAll() simply contain each version of the function.

One thing I didn't put as I don't think it's relevant to this code review is that one should be careful with inline function expressions. They're not themselves testable, so if they get more than half a dozen statements or have any branching or conditional logic: extract them into their own functions, and test them separately.

Oh, and in case yer wondering... yes I did write the tests before I wrote any of my own code. And I found a coupla bugs with my planned solution (and assumptions about the requirement) in doing so. Always TDD. Always.

Anyway, all of this is getting in the way of my Saturday.

Righto.

--
Adam

Thursday, 3 March 2016

PHPUnit: trap for dumb players regarding mocking

G'day:
This is just a demonstration of me being a git. But who doesn't like that?

We use PHPUnit extensively to test our codebase. And along with that we use its mocking facilities extensively to mock-out our dependencies in these tests. It's more of a hassle to use than MockBox was on CFML, but this is down to PHP being more rigid with how it goes about things, and is not the fault of PHPUnit (or indeed anyone's "fault" at all: it's just "a thing").

Last week I was flummoxed for a lot longer than I ought to have been, because for some reason a method simply would not mock. After a number of hours I asked the other bods on the team to put their eyes on the code and my mate Amar spotted the problem instantly. The issue was down to me making assumptions about one of our code bases that I was not familiar with, and not bothering to check my assumptions. And also some user-unfriendliness in PHPUnit.

Here's a repro of the code I was running. Well: it bears absolutely no relation to the code I was running at all, but it demonstrates the issue.

Firstly I have a simple service class which has a method I want to test:

namespace me\adamcameron\mocking\service;

class MyService {

    private $logger;

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

    public function doesStuff($value){
        $preppedValue = "PREPPED $value";
        $processedValue = $this->doesRiskyStuff($preppedValue);
        $this->logger->logSomething($processedValue);
        return "RESULT OF DOING STUFF ON $processedValue";
    }

    private function doesRiskyStuff($preppedValue){
        return "$preppedValue (SURVIVED RISKY PROCESS)";
    }

}

This method uses a method from one external dependency, plus it also calls one of its own helper methods which does "risky stuff", so we actually want to mock-out both of those methods.

First things first I already know that one cannot mock private methods with PHPUnit, so we need to make that method protected. This is not ideal, but it's better than not being able to test safely, so it's a burden we are prepared to wear.

Having made that change, we need to partially mock the class we need to test:

function getTestMyService($logger){
    $partiallyMockedMyService = $this->getMockBuilder('\me\adamcameron\mocking\service\MyService')
        ->setConstructorArgs([$logger])
        ->setMethods(["doesRiskyStuff"])
        ->getMock();

    $partiallyMockedMyService->expects($this->once())
        ->method("doesRiskyStuff")
        ->with("PREPPED TEST VALUE")
        ->willReturn("MOCKED RESPONSE FROM DOESRISKYSTUFF");

    return $partiallyMockedMyService;
}

This just mocks-out doesRiskyStuff(), leaving everything else as is. So now when we call doesStuff() in our test, it won't call the real doesRiskyStuff(), but our mock instead.

Notice how some expectations are set on the mocked method itself here:

  • the number of times we expect it to be called;
  • what arguments it will be passed;
  • and a mocked value for it to return.

We call this from our setup() method:

function setup(){
    $logger = $this->getTestLogger();
    $this->myService = $this->getTestMyService($logger);
}


Our test method then is very simple:

/**
 * @covers doesStuff
 */
function testDoesStuff(){
   $result = $this->myService->doesStuff("TEST VALUE");

    $this->assertSame("RESULT OF DOING STUFF ON MOCKED RESPONSE FROM DOESRISKYSTUFF", $result);
}

It simply calls the method and checks what it returns. Simple.

But so far I've glossed over the logging part. 'ere 'tis:

namespace me\adamcameron\mocking\service;

use me\adamcameron\mocking\exception\NotImplementedException;

class LoggingService {

    final public function logSomething($text){
        throw new NotImplementedException(__FUNCTION__ . " not implemented yet");
    }

}


There's not much to that, and indeed I have not even implemented it yet. This is cool... we can just mock it out and test that it receives what it's supposed to be passed by our doesStuff() method. That completes the coverage of doesStuff(): our test of doesStuff() does not need to care whether logSomething() actually works.

Here's how we mock it:

function getTestLogger(){
    $mockedLogger = $this->getMockBuilder('\me\adamcameron\mocking\service\LoggingService')
        ->setMethods(["logSomething"])
        ->getMock();

    $mockedLogger->expects($this->once())
        ->method("logSomething")
        ->with("MOCKED RESPONSE FROM DOESRISKYSTUFF");

    return $mockedLogger;
}


All very obvious and the same as before.

When I run my tests, I see this:

C:\src\php\php.local\www\experiment\phpunit\mock>phpunit
PHPUnit 4.8.23 by Sebastian Bergmann and contributors.

E

Time: 733 ms, Memory: 5.00Mb

There was 1 error:

1) me\adamcameron\mocking\test\service\MyServiceTest::testDoesStuffme\adamcameron\mocking\exception\NotImplementedException: logSomething not implemented yet

C:\src\php\php.local\www\experiment\phpunit\mock\src\service\LoggingService.php:
10
C:\src\php\php.local\www\experiment\phpunit\mock\src\service\MyService.php:16
C:\src\php\php.local\www\experiment\phpunit\mock\test\service\MyServiceTest.php:
23
phpunit\phpunit\src\TextUI\Command.php:149
phpunit\phpunit\src\TextUI\Command.php:100

FAILURES!
Tests: 1, Assertions: 0, Errors: 1.

C:\src\php\php.local\www\experiment\phpunit\mock>


Friday, 27 November 2015

Unit testing (PHP): keeping sight of the intent of the tests

G'day:
An article with some actual code in it! That'd become a rarity. This is actuallya real-world challenge I'm faced with in backfilling some unit tests. Yeah, I know I'm an advocate of TDD so that should mean there's no "backfilling" of tests: they all get done up front. But for reasons I won't go into... I'm backfilling testing for this function.

Here's the function:

public static function loadValidatorMetadata(ClassMetadata $metadata)
{
    $metadata->addPropertyConstraint('languageId', new Assert\Range(
        ['min' => 1, 'max' => self::$maxLegacyLanguageId]
    ));

    $metadata->addPropertyConstraint('destinationId', new Assert\Range(['min' => 1]));

    $metadata->addConstraint(new Assert\Callback('validateArrivalDate'));
    $metadata->addConstraint(new Assert\Callback('validateNights'));

    $metadata->addPropertyConstraint('firstName', new Assert\NotBlank());
    $metadata->addPropertyConstraint('firstName', new Assert\Length(
        ['min' => 1, 'max' => GroupEnquiryRepository::SHORT_STRING_MAX_LENGTH]
    ));
    $metadata->addPropertyConstraint('surName', new Assert\NotBlank());
    $metadata->addPropertyConstraint('surName', new Assert\Length(
        ['min' => 1, 'max' => GroupEnquiryRepository::SHORT_STRING_MAX_LENGTH]
    ));

    $metadata->addPropertyConstraint('email', new Assert\NotBlank());
    $metadata->addPropertyConstraint('email', new Assert\Email());

    $metadata->addPropertyConstraint('groupTypeId', new Assert\Range(['min' => 1]));
    $metadata->addPropertyConstraint('groupSize', new Assert\NotNull());
    $metadata->addPropertyConstraint('groupSize', new Assert\Range(
        ['min' => GroupEnquiryService::MIN_GROUPS_THRESHOLD, 'max' => BookingService::MAXIMUM_PEOPLE]
    ));
    $metadata->addPropertyConstraint('youngestAge', new Assert\Range(
        ['min' => 1, 'max' => GroupEnquiryService::MAX_AGE]
    ));

    $metadata->addPropertyConstraint('telephoneCountryCode', new Assert\NotBlank());
    $metadata->addPropertyConstraint('telephoneCountryCode', new Assert\Length(
        ['min' => 1, 'max' => GroupEnquiryRepository::SHORT_STRING_MAX_LENGTH]
    ));
    $metadata->addPropertyConstraint('phone', new Assert\NotBlank());
    $metadata->addPropertyConstraint('phone', new Assert\Length(
        ['min' => GroupEnquiryService::MIN_TELEPHONE_NUMBER_LENGTH]
    ));
    $metadata->addConstraint(new Assert\Callback('validateMobileCountryCode'));
    $metadata->addConstraint(new Assert\Callback('validateMobile'));

    $metadata->addConstraint(new Assert\Callback('validateAdditionalInfo'));
}

First things first: yeah, normally I'd not write a function that's 40-odd lines long, but in this case there's very little actual logic, plus it's only setting some validation config. BTW this is using the Symfony 2 validation framework (Symfony: validation), as implemented by the Silex microframework (ValidatorServiceProvider: validating objects).

To provide context, we have a class - basically a validation bean - thus:

class GroupEnquiry
{
    public $languageId;
    public $destinationId;
    public $arrivalDate;
    public $nights;
    public $firstName;
    public $surName;
    public $email;
    public $telephoneCountryCode;
    public $phone;
    public $mobileCountryCode;
    public $mobile;
    public $groupTypeId;
    public $groupSize;
    public $youngestAge;
    public $additionalInfo;
}

And that loadValidatorMetadata() function provides the constraints for Symfony to enforce. As you can see: there's range validation, existence validation, length validation, date validation, and a coupla compound constraints implemented via callbacks.

So, anyway, I need to write the testing for loadValidatorMetadata(). Initially I was staring at the code going and going "how the hell?".

Then I thought: I'll assert the addConstraint() and addPropertyConstraint() methods are called for the correct properties, and the correct number of times. But that seemed unhelpful. What's the point of checking that some constraint is applied to a property, but not checking what sort of constraint. IT all seemed too contrived.

Then I thought that the constraints themselves don't need testing here because I'm either testing our callbacks elsewhere, or Symfony have done their own testing. In theory this is a fine notion, but in reality it does not actually help me test our validation code works.

I was stumped as to how to test this stuff, without basically replicating the function in parallel for test. And that clearly sucks as far as effort and common sense goes. It was the wrong approach.

Sunday, 23 August 2015

JavaScript: running Jasmine unit tests from the CLI

G'day:
I can't see as this will be very long as I've not much to say about it, and it was surprisingly quick to sort out once I bothered to do so.

My weekend mission was going to be to start getting up to speed with Clojure, by reading & working through the relevant chapters from Seven Languages in Seven Weeks, but whilst that kept me occupied on my flight from London to Shannon, I didn't really revisit it after that, as I got sidetracked on these Jasmine tests. Well firstly I decided my mission would be to do a Clojure version of my "get a subseries" quiz ("Something for the weekend? A wee code quiz (in CFML, PHP, anything really...)"), and before doing that I wanted to get a working version of the code I discussed in "Some CFML code that doesn't work" working, chosing JavaScript as my control language. And in deciding to do that, I decided not to just copy and paste it into the browser console to test it, I decided to do it properly and do a file-system-based exercise running it via Node (note to Acker: I already use Node. Just sparingly because the need for it doesn't crop up for me that often. You don't actually have any special knowledge the rest of us also don't already have). And if I was gonna do that, then I was also gonna get Jasmine working via Node too, as we actually do have a real requirement for this at work. We have an ever increasing amount of ever increasingly complex JS in our application... and so far not a line of testing gets done on it. We're shifting our mindset to be writing more testable JS: reducing inline callbacks; putting a much of our code in "class" file, and writing small, clean, testable methods an the like all ready for testing... but getting the actual test infrastructure up and running is just not happening.

So, anyway... other than installing Clojure via Leiningen and runing "G'day World" via the REPL, my Clojure investigations didn't move much.

But  I got the JavaScript version of my CFML code running, and also got its tests running via the commandline too. So that's cool.

Thursday, 6 August 2015

CFML: code challenge from the CFML Slack channel

G'day:
I wasn't gonna write anything today, but then Jessica on Slack (to use her full name) posted a code puzzle on the CFML Slack channel, which I caught whilst I was sardined in a train to Ilford en route home. I nutted out a coupla solutions in my head instead of reading my book, and I saw this as a good opportunity (read: "excuse") to pop down to the local once I got home and squared away a coupla things, and key the code in and see if it worked. I was moderately pleased with the results, and I think I've solved it in an interesting way, so am gonna reproduce here.

Jessica's problem was thus:
so, i am attempting to write a function that lets you set variables specifically in a complex structure [...]
cached:{
    foo: {
        bar: "hi"
    }
}
setProperty("foo.bar", "chicken");
writeDump(cached); // should == cached.foo.bar = chicken

On the Slack channel there was talk of loops and recursion and that sort of thing, which all sounded fine (other people came up with answers, but I purposely did not look at them lest they influenced my own efforts). The more I work with CFML and its iteration methods (map(), reduce(), etc), the more I think actually having to loop over something seems a bit primitive, and non-descriptive. I looked at this for a few minutes... [furrowed my brow]... and thought "you could reduce that dotted path to a reference to the substruct I reckon". There were a few challenges there - if CFML had proper references it'd be easier - but I got an idea of the code in my head, and it seemed nice and easy.

Whilst waiting to Skype with my boy I wrote my tests:

Tuesday, 21 April 2015

PHP: unit testing private methods... PHP 5.5 version

G'day:
The Central Line isn't running at the moment and the buses are all shagged due to everyone wanting now to catch one, so I've given up trying to get to work and gone home until things settle down a bit. Not wanting to be completely useless to my employers, I've decided to solve an issue I created yesterday by recommending using some PHP 5.6-only code on out 5.5 environment. Duh.

Tuesday, 23 December 2014

JavaScript: Jasmine for unit testing

G'day:
At work, I've been tasked with getting the team up to speed with TDD whilst we redevelop our website in PHP. I knocked together a presentation on the subject a coupla months ago, but before having a chance to present it, got shifted about in the internal dept structure for a month or so, and it kinda got temporarily shelved. I posted it online: "TDD presentation". I'm back on the PHP Team now, and need to update said presentation to be more work-requirement-specific, as well as cover unit testing our JavaScript. This has been on our agenda for a coupla years, but was never allowed to get any traction by the decision makers. Decision-making has improved now, so we're all go.

I have heard about Jasmine, and like the look of it, but have never actually downloaded / installed / ran it. I'm gonna do that today.

(Oh, blogging my work is not something I do... I'm actually off on sick leave at the moment - which I feel slightly guilty about - but I need to get this stuff done, so gonna do it today whilst I am unlikely to get interruptions. I figured as I'm doing it on my own time, I get to blog about it too ;-)

I am writing about this as I do it.

Jasmine


First up: Jasmine. This is what Wikipedia has to say about Jasmine:

Jasmine is an open source testing framework for JavaScript. It aims to run on any JavaScript-enabled platform, to not intrude on the application nor the IDE, and to have easy-to-read syntax. It is heavily influenced by other unit testing frameworks, such as ScrewUnit, JSSpec, JSpec, and RSpec.
And from its own website:

Jasmine is a behavior-driven development framework for testing JavaScript code. It does not depend on any other JavaScript frameworks.

And a code sample from the same page:

describe("A suite", function() {
  it("contains spec with an expectation", function() {
    expect(true).toBe(true);
  });
});

If you're familiar with TestBox (and if you're a CFML dev, you bloody should be!), then this will look comfortingly familiar. Indeed that code would run on TestBox. I know a bit about TestBox, so this is pleasing: I have a head start!

Download & Install

I'm gonna use 2.1.3, which is - at time of writing - the latest version of Jasmine. The download page is here: jasmine 2.1.3. I've D/Led that and unzipped it into a public directory.

Running

This is too easy... it ships with a file SpecRunner.html, and browsing to that runs the tests. Here are the samples:



Nice!

Example Code

Looking at the code within SpecRunner.html, we see this:

Tuesday, 7 October 2014

The received wisdom of TDD and private methods

G'day:
As you might know, I've recently taken on a different role as a PHP developer. My employer are shifting our code base from CFML to PHP for various reasons ("So long, and thanks for all the CF"). One facet of this is we're moving off a venerable, well-established CFML code base to a new code base in the infancy of its existence (some work on it had been done before we picked up the project). In a different language.

And we've got from having about 4000 unit tests to zero.



A coupla of us are having a quick exploratory look at PHPUnit to see what it can do, and whether there's any "gotchas" we need to be aware of when testing in PHP. So far the findings seem to be that unit testing in CFML via MXUnit and TestBox - especially in conjunction with MockBox - is a bit easier than the hoops we need to jump through in PHP to achieve the same ends. This is mostly down to CFML's objects being far more dynamic than PHP's, so injecting testing helper methods and exposing non-public methods for testing is much easier.

The challenges that PHP has thrown at us has caused us to revisit our TDD and unit testing strategy. Not necessarily to revise it, but to revisit it and see if it does need revision.

Our existing policy is that all code is written via a TDD & "Clean Code" approach:
  1. the need for a piece of functionality is identified;
  2. a failing test is written to test a facet of the functionality;
  3. code is written to pass the test;
  4. repeat from 2 until the functionality is complete;
  5. refactor if necessary.

This is applied both to new functionality as well as maintenance of existing functionality. The TDD side of things drives the code design, and also demonstrates that downstream changes don't have adverse effects on earlier requirements. The usual sort of thing.

On new work, this will mean creating a new public method (and the class it needs to go in, as required), and implementing the requirement. So all the tests are on that public method. When we refactor the code, those tests all still work, which is fine.

As a result of the refactoring we might end up with some of the code moving out into public methods of other classes, or - as often - private methods of the same class.

The newly refactored public methods go through the same TDD approach as the initial one, although this is quite often just a re-homing of the tests which were previously testing the unfactored functionality from the original public method. And the original public method's tests are selectively updated to remove any tests which are now the domain of the new methods, and mocks are used in lieu of calling these factored-out methods in the remaining tests of the original function.

And traditionally we have done exactly the same thing with the refactoring that was simply moved into private methods.

Perhaps I can demonstrate this with some pseudocode. At the end of our first TDD round, and before refactoring, we have this:

Sunday, 6 July 2014

Both ColdFusion and Railo make an incomplete job of validating email addresses

G'day:
A question came up on StackOverflow just now "new to coldfusion. is there any function that defines Email is typed correctly" (how hard, btw, is it to give a coherent title to one's question? [furrow]).

My initial reaction was along the lines of "RTFM, and if you had, you'd see isValid() validates email addresses". However I also know that isValid() is very poorly implemented (see a google of "site:blog.adamcameron.me isvalid" by way of my findings/rantings on the matter).

So I decided to check how reliable it actually is.

The rules for what constitutes a valid email address a quite simply, and a laid out very clearly in various RFCs. Or if one can't be bothered doing that (although the engineers & Adobe and Railo should be bothered), then they're fairly comprehensively defined even on Wikipedia: "Email_address (Syntax)". I found that page by googling for it.

Anyway, I won't bore you by replicating the rules here, but I wrote some TestBox tests to test all the rules, and how isValid() supports them. The answer to this on both ColdFusion (11) and Railo (4.2) is "only to a moderate level". This is kinda what I expect of the Adobe ColdFusion Team (this is the level of quality they seem to aim for, generally, it seems), but am pretty let down by the Railo guys here.

I won't reproduce all the code here as it's long and boring, but it's on GitHub: "TestIsValidEmail.cfc".

Anyhow, the bottom lines are as follows:

ColdFusion:


Railo:


Railo did slightly better, but ran substantially slower (running on same machine, same spec JVM).

I hasten to add that 50 of those tests were testing the validity of individual invalid characters, so that blows the failures out a bit (given they pretty much all failed). But both also failed on some fairly bog-standard stuff.

It's seriously as if it didn't occur to the engineers involved to actually implement the functionality as per the published specification; instead they simply made some shit up, based on what their own personal knowledge of what email addresses kinda look like. This is a bit slack, I'm afraid, chaps. And would just be so easy to do properly / thoroughly.

NB: there are rules around comments in email addresses that I only gave superficial coverage of, but I covered all the other rules at least superficially in these tests.



One new TestBox thing which I learned / noticed / twigged-to today... one can write test cases within loops. One cannot do that with MXUnit! eg:

describe("Tests for invalid ASCII characters", function(){
    for (var char in [" ", "(", ")", ",", ":", ";", "<", ">", "@", "[", "]"]){
        var testAddress = "example#char#example@example.com";
        it("rejects [#char#]: [#testAddress#]", function(){
            expect(
                isValid("email", testAddress)
            ).toBeFalse();
        });
        testAddress = '"example#char#example"@example.com';
        it("accepts [#char#] when quoted: [#testAddress#]", function(){ 
            expect(
                isValid("email", testAddress)
            ).toBeTrue();
        });
    }
});

Here I loop through a bunch of characters, and Here for each character perform two tests. This is possible because testBox uses function expressions for its tests, whereas MXUnit uses declared functions (which cannot be put in loops). This is not a slight on MXUnit at all, because it predates CFML's support for function expressions; it's more just that I'm so used to having to write individual test functions it did not occur to me that I could do this in TestBox until today. I guess there's nothing stopping one from writing one's test functions as function expressions in MXUnit either, that said. But this is the encouraged approach with TestBox.

Nice one, TestBox.

Anyway, this is not helping me prep for my presentation tomorrow. Give me a ballocking if you hear from me again today, OK?

--
Adam

Saturday, 8 March 2014

Unit testing: the importance of testing over assumption

G'day:
Today some code! That's better than rhetoric innit? I'm sure it won't be as popular though.

OK, so I have been too busy recently with ColdFusion 11 and various other odds 'n' sods to either continue writing about unit testing, and also I have been neglecting the CFLib.org submission queue. So today I am remedying both of those in one fell swoop.

This article is not really "TDD" or "BDD" as the code I am testing is a fait accompli, so the tests are not driving the development. However this is perhaps a good lesson in how to retrofit tests into functionality, and what sort of things to test for. There's a bit of TDD I suppose, because I end up completely refactoring the function, but first create a test bed so that that is a safe thing to do without regressions.

On CFLib there is already a function firstDayOfWeek(), but someone has just submitted a tweak to it so that it defaults to using today's date if no date is passed into it. Fair enough. Here's the updated code:

function firstDayOfWeek() {
    var dow = "";
    var dowMod = "";
    var dowMult = "";
    var firstDayOfWeek = "";
    if(arrayLen(arguments) is 0) arguments.date = now();
    date = trim(arguments.date);
    dow = dayOfWeek(date);
    dowMod = decrementValue(dow);
    dowMult = dowMod * -1;
    firstDayOfWeek = dateAdd("d", dowMult, date);

    return firstDayOfWeek;
}

Nothing earthshattering there, and I basically scanned down it going, "yep... uh-huh... oh, OK, yeah... cool... yeah that seems OK". And I was about to just approve it. Then I felt guilty and thought "ah... I really should test this. And I'll use TestBox and its BDD-style syntax to do so, and I can get a blog article out of this too.

It's just as well I did test it, because it has a bug. Can you see it?

I sat down and looked at the code, and went "right... what do I need to test?". I came up with this lot:
  • if I don't pass in a date, it returns the first day of the current week;
  • if I do pass in a date, it returns the first day of that week;
  • if I pass in the date that is the first day of the week already, it passes back that same date;
  • if I pass it "not a date", it errors predictably.
I think that about covers the testing need here? OK, so I started writing the tests:

// TestFirstDayOfWeek.cfc
component extends="testbox.system.testing.BaseSpec" {

    function beforeAll(){
        include "udfs/firstDayOfWeek.cfm";

        variables.testDate = now();
        variables.testFirstDayOfWeek = dateAdd("d", -(dayOfWeek(variables.testDate)-1), variables.testDate);
    }

    function run(){
        describe("Tests for firstDayOfWeek()", function(){

            it("returns most recent sunday for default date", function(){
                expect(
                    dateCompare(variables.testFirstDayOfWeek, firstDayOfWeek(), "d")
                ).toBe(0);
            });

            it("returns most recent sunday for current date", function(){
                expect(
                    dateCompare(variables.testFirstDayOfWeek, firstDayOfWeek(variables.testDate), "d")
                ).toBe(0);
            });
        });
    }

}

Notes:
  • I've got the UDF saved in a separate file, so just include it at the beginning of the tests;
  • I set some helper variables for using in the tests;
  • I'll get to this.
I was running the tests as I went, and here's what I got on the test round with these first two tests:

Wednesday, 19 February 2014

TestBox, BDD-style tests, Railo member functions and bugs therein

G'day:
One of the promised features of ColdFusion 11 is to bring "member functions" to ColdFusion's inbuilt data types. Railo's already had a good go at doing this, and has reasonably good coverage. See "Member Functions" for details.

One concern I have is whether ColdFusion 11 will implement these the same way as Railo already has. I mean... they should do, there's not much wriggle room, but who knows. If there's a way to do it wrong, I'm sure Adobe can find it. With that in mind, I want to be able to run through some regression/transgression tests on both systems once ColdFusion 11 goes beta, so in prep for that, today I knocked out some unit tests for all the struct member functions.

What I did is to go into the ColdFusion 10 docs, go to the "Structure functions" section (to remind me what all the functions were), and write unit tests for the whole lot. I used the ColdFusion docs rather than the Railo ones because the CF10 docs are more comprehensive than Railo's (this is an indictment of Railo's docs, not a recommendation for the ColdFusion ones, btw!), plus I wanted to make sure I covered any functions Railo might have overlooked.

To make this more interesting (because, let's face it, it's not an interesting task; either to undertake, write up, or for you to read about!), I decided to have a look at TestBox's BDD-inspired testing syntax. The short version of this side of things is that I rather like this new syntax.  I don't think it's got anything to do with BDD (which is an approach to documentation and test design, not a syntax model), but it's interesting anyhow.

Anyway, stand-by for a raft of code (there's a Gist of this too: Struct.cfc):

Monday, 30 December 2013

Unit Testing / TDD: not testing stuff

G'day:
It's about bloody time I got back to this series on TDD and unit testing. I've already got the next few articles semi-planned in my head: topic, if not content. I have to say I am not in the most inspired writing mood today, and the words aren't exactly flowing from fingers through keyboard to screen - this is the third attempt at this paragraph - but we'll see how I go.

To find inspiration and to free up the fingers a bit, I'm at my local pub in Merlin Park in Galway, which - today - has shown its true colours as a football pub (to you Americans, that's "soccer". To me, it's "shite"). I've tried to like football, but it all just seems too effeminate to me. The place is chock-full of colours-wearing lads yelling at the screen. Either for or against one of Chelsea or Liverpool (the former lead 2-1 at half time). It's not conducive to writing, but the Guinness next to me will be.

I'll not list the previous articles in the series as it will take up too much room. They're all tagged with "TDD" and "unit testing" though.

On with the show...

Monday, 23 December 2013

CFML/TestBox: running on actual proper tests

G'day:
Various people seem to be trying out TestBox on their old MXUnit unit tests today, and reporting back on Twitter. I'm working through our ones, and have a few findings. I have more that 140 chars of information, so I'll quickly blog instead.

I can't go into too many details regarding our tests, but we have a few thousand of them spread over a number of applications.

Running through our main tranche of... ooh... 2793 (!!) tests, I had the following issues:

assertIsQuery() is missing

This is a standard MXUnit assertion (assertIsQuery()), missing from TestBox. Lack of this probably breaks about 200 of our unit tests

assertIsEmpty() is missing

So is this (assertIsEmpty()). We use this less often, but it probably breaks 50-odd tests.

mxunit:expectedException missing

It was pointed out to me the other day ("I should pay attention to what my readers tell me! expectException()") that MXUnit has a better approach to dealing with this - and one that seems is indeed implemented in TestBox - expectException(). However we have a lot of tests, and a bunch of them still use the above annotation. Perhaps 50 tests "fail" due to this not being implemented.

makePublic() doesn't seem to work

I didn't investigate this thoroughly, but it seems TestBox has implemented makePublic(), but it doesn't actually work. I say this because I get no error on code calling makePublic(), but none of the methods made public actually are public, as a result. A few hundred failures.

mock() not implemented

MXUnit has built-in mocking. So does TestBox. However each are different, and it doesn't look like MXUnit's version has been implemented. We only use this once in our tests (we generally use MockBox).

If beforeTests() errors, testing halts

This in itself is completely the same as MXUnit, but it basically discounts any chance of us using TestBox as it stands, because given addAssertDecorator() is also not implemented, this prevents me from actually doing a test run. Because the whole thing falls over as soon as we try to bring in our custom assertions. That aside, lack of being able to call in custom assertions also kills about another 500 of our tests. Fixing beforeTests() would be really nice, but if addAssertDecorator() was implemented, that'd get us moving forward.

One really annoying side effect of this is that TestBox doesn't preserve the context of where the error happened, it just reports it as happening in the bowels of TestBox itself, which is not correct. It should bubble back the original exception. It was basically impossible for me to bandaid some of the errors I was seeing because TestBox wasn't telling me where they were happening!


The method init was not found in component TestCase.cfc

This is a very edge-case one. We have a helper CFC which is not run as a test case, but we want to run some of TestCase's methods. So we instantiate an instance of TestCase of our own. In MXUnit, TestCase.cfc has an init() method. In TestBox, it does not, so it errors. This is one of those crazy edge-cases one only ever discovers testing on real world code.

Others

I was getting a few other test failures too, in tests I know work fine. I wasn't too sure if those were a byproduct of all the stuff above, so I did not investigate. Once I have a stable system, I will look at those ones more closely.

Conclusion

So those results weren't great. About a quarter of our tests would not run without being monkeyed with, and we cannot do a test run at all. Still: this is because of a few minor glitches we use in key areas of our tests.

I'm outa lunchtime, so I will need to run the other tests later, and see if I come up with anything else. But until we get the issues above sorted out, TestBox is not a possibility for us. I'll continue to use it at home, though.

--
Adam