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.
// 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.
[furrowed brow]
Ah. OK, it's this:
if(arrayLen(arguments) is 0) arguments.date = now();
There's a logic error in that. It tests the length of the argument array, and if there's no arguments passed, it defaults
arguments.date. However my code calls the function thus:firstDayOfWeek(variables.testDate)
So I am passing an argument (so the
arrayLen() is 1), and so arguments.date does not get defaulted. But the argument I passed isn't date. It's just arguments[1]. So then the code that then assumes there's an arguments.date then fails.If the function definition had specified the date argument (which it should have), then this problem would not exist. Or if the logic was more sound it also wouldn't be an issue. If the code requirement is that arguments.date needs to exist (otherwise default it), then test for that, eg:
if(!structKeyExists(arguments, "date")) arguments.date = now();
So I had some fixing to do. But first I rounded out 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(){
var isRailo = function(){
return structKeyExists(server, "railo");
};
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 as positional argument", function(){
expect(
dateCompare(variables.testFirstDayOfWeek, firstDayOfWeek(variables.testDate), "d")
).toBe(0);
});
it("returns most recent sunday for current date as named argument", function(){
expect(
dateCompare(variables.testFirstDayOfWeek, firstDayOfWeek(date=variables.testDate), "d")
).toBe(0);
});
it("returns same date if passed-in date is already first day of week", function(){
expect(
dateCompare(variables.testFirstDayOfWeek, firstDayOfWeek(date=variables.testFirstDayOfWeek), "d")
).toBe(0);
});
it(
title = "gives typeful error if non-date is passed (CF)",
body = function(){
expect(function(){
firstDayOfWeek(date="NOT_A_DATE");
}).toThrow(regex="The DATE argument passed to the firstDayOfWeek function is not of type date");
},
labels = "",
skip = isRailo()
);
it(
title = "gives typeful error if non-date is passed (Railo)",
body = function(){
expect(function(){
firstDayOfWeek(date="NOT_A_DATE");
}).toThrow(regex="first Argument \(date\) is of invalid type, can't cast String \[not_a_date] to a value of type \[date]");
},
labels = "",
skip = !isRailo()
);
});
}
}
Most of the TestBox constructs I deal with here I've already covered in an earlier article, but I've just linked through to the docs on the first example of each.
The only new construct I am using here is
skip. On a call to it(), one can specify a skip argument which - if true - will not run the test. TBH I don't like how this has been implemented, as I think the logic has been reversed from what is intuitive. It would be more intuitive if the condition predicated whether to run the test (defaulting to true), rather than to skip the test. This is more clear in intent, I think:it("gives typeful error if non-date is passed (CF)", function, "", isColdFusion());
One needs to use a negative-sounding test to make it work: specifying that it is appropriate to run is more natural than specifying that it's not appropriate to run, or it is inappropriate (if you see the subtle difference here). I also think it really breaks the otherwise excellent readability / intuitiveness of this syntax in general. Still: it's a minor thing.
In this case I am testing for the exception message when one doesn't pass a date to the function. Unfortunately ColdFusion and Railo throw both different exception types and they have different messages too. Also their types are very vague ("expression" on ColdFusion, and I can't remember what on Railo), so I don't really think testing just for an "expression" exception - which could be just about bloody anything - is accurate enough here. So I'm testing that the message is the one one would get if passing an incorrectly typed argument value to a function. Which is what I want to test for here.
The results on ColdFusion are as follows:
We expect that second test to fail due to the bug I mentioned above. And I also kinda set the function up a bit on that last test, as I knew it would fail. It has another logic error, in that it requires the date argument to be a date, but it never validates it to be one, instead just letting the internal code fail. This is pretty sloppy in my opinion, so I'm gonna fix it (it's easy: just specify the argument in the function definition).
There's a coupla slight tweaks to the function before I can approve it:
- fix the deal with the positional arguments thing;
- also ensure the passed-in argument value is actually a date.
Then something else occurred to me. There's an awful lot of code there to do something very simple. Contrast the function 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;
}}
With my code to create my expected test result value:
variables.testFirstDayOfWeek = dateAdd("d", -(dayOfWeek(variables.testDate)-1), variables.testDate);
That does the same thing. So... erm... let's just do that. I fix the bugs and use the simplified version of the logic, and end up with this:
date function firstDayOfWeek(date date=now()){
return dateAdd("d", -(dayOfWeek(date)-1), date);
}
That's a helluva lot clearer as to what is going on here, I think.
And, bless, here are the test results now:
That's enough effort for a one-line UDF I think.
But here's the lesson... don't just eyeball yer code and go "looks OK". You could well be wrong. So test it.
--
Adam
