Tuesday 24 August 2021

Test coverage: it's not about lines of code

G'day

A coupla days ago someone shared this Twitter status with me:

I'll just repeat the text here for google & copy-n-paste-ability:

My personal opinion: Having 100% test coverage doesn't make your code more effective. Tests should add value but at some point being overly redundant and testing absolutely every line of code is ineffective. Ex) Testing that a component renders doesn't IN MY OPINION add value.

Emma's position here is spot on, IMO. There were also a lot of "interesting" replies to it. Quelle surprise. Go read some of them, but then give up when they get too repetitive / unhelpful.

In a similar - but slightly contrary - context, I was reminded of my erstwhile article on this topic: "Yeah, you do want 100% test coverage".

But hang on: on one hand I say Emma is spot on. On the other hand I cross-post an old article that seems to contradict this. What gives?

The point of differentiation is "testing absolutely every line of code" vs "100% test coverage". Emma is talking about lines of code. I'm talking about test cases.

Don't worry about testing lines of code. That's intrinsically testing "implementation". However do care about your test cases: the variations that define the feature you are working on. you've been asked to develop a feature and its variations, and you don't know you've delivered the feature (or in the case of automated testing: continue to having been delivered in a working state) unless you test it.

Now, yeah, sure: features are made of lines of code, but don't worry about those. A trite example I posited to a mate yesterday is this: consider this to be the implementation of your feature:

doMyFeature() {
    // line 1
    // line 2
    // line 3
    // line 4
    // line 5
    // line 6
    // line 7
    // line 8
    // line 9
    // line 10
}

I challenge you to write a test for "My Feature", and not by implictation happen to test all those lines of code. But it's the feature we care about, not the lines of code.

On the other hand, let's consider a variant:

doMyFeature() {
    // line 1
    // line 2
    // line 3
    // line 4
    // line 5
    if (someVariantOfMyFeature) {
        // line 6
    }
    // line 7
    // line 8
    // line 9
    // line 10
}

If you run your test coverage analysis and see that line 6 ain't tested, this is not a case of wringing one's hands about line 6 of the code not being tested; it's that yer not testing the variation of the feature. It's the feature coverage that's missing. Not the lines-of-code coverage.

Intrinsically your code coverage analysis tooling probably marks individual lines of code as green or red or whatever, but only if you look that closely. If you zoom out a bit, you'll see that the method the lines of code are in is either green or orange or red; and out further the class is likewise green / orange / red, and probably says something like "76% coverage". The tooling necessarily needs to work in lines-of-code because its a dumb machine, and those are the units it can work on. You are the programmer don't need to focus on the lines-of-code. You have a brain, and what the report is saying is "you've not tested part of your feature", and it's saying "the bit of the feature that is represented by these lines of code". That bit. You're not testing it. Go test yer feature properly.

There's parts of the implementation of a feature I won't test maybe. Your DAOs and your other adapters for external services? Maybe not something I'd test during the TDD cycle of things, but it might still be prudent to chuck in an integration test for those. I mean they do need to work and continue to work. But I see this as possibly outside of feature testing. Maybe? Is it?

Also - now I won't repeat too much of that other article - there's a difference between "coverage" and "actually tested". Responsible devs can mark some code as "won't test" (eg in PHPUnit with a @codeCoverageIgnore), and if the reasons are legit then they're "covered" there.

Why I'm writing this fairly short article when it's kinda treading on ground I've already covered is because of some of the comments I saw in reply to Emma. Some devs are a bit shit, and a bit lazy, and they will see what Emma has written, and their take away will be "basically I don't need to test x because x is some lines of code, and we should not be focusing on lines of code for testing". That sounds daft, but I know people that have rationalised their way out of testing perfectly testable (and test-necessary) code due to "oh we should not test implementation details", or "you're just focusing on lines of code, that's wrong", and that sort of shit. Sorry pal, but a feature necessarily has an implementation, and that's made out of lines of code, so you will need to address that implementation in yer testing, which will innately report that those lines of code are "covered".

Also this notion of "100% coverage is too high, so maybe 80% is fine" (possibly using the Pareto Pinciple, poss just pulling a number of out their arses). Serious question: which 80%? If you set that goal, the a lot of devs will go "aah, it's in that 20% we don't test". Or they'll test the easy 80%, and leave the hard 20% - the bit that needs testing - to be the 20% they don't test (as I said in the other article: cos they basically can't be arsed).

Sorry to be a bit of a downer when it comes to devs' level of responsibility / professionalism / what-have-you. There are stack of devs out there who rock and just "get" what Emma (et al) is saying, and crack on with writing excellently-designed and well-tested features. But they probably didn't need Emma's messaging in the first place. I will just always think some more about advice like this, and wonder how it can be interpreted, and then be a contrarian and say "well actually…" (cringe, OK I hope I'm not actually doing that here?), and offer a different informed observation.

So my position remains: set your sights on 100% feature coverage. Use TDD, and your code will be better and leaner and you'll also likely end up with 100% LOC coverage too (but that doesn't matter; it's just a test-implementation detail ;-).

Righto.

--
Adam

Tuesday 3 August 2021

CFML: static methods and properties

G'day:

Context

In Lucee 5.0 (so: ages ago) and ColdFusion 2021 (so: you know… just now), support for static properties and methods was added to CFML. This isn't a feature that you'd use very often, but it's essential to at least know about it. And it's bloody handy, and makes your code clearer sometimes. I had reason to be googling for docs / examples today, and what's out there ain't great so I thought I'd write my own "probably still not great" effort.

OK so that's the bar set suitably low. Let's get on with it.

What?

What are static properties and methods. I'm going to start from the other end of things. What are general properties and methods? They're properties and methods of an object; you know, one of those things one gets as the result of calling new MyComponent() (or from a createObject call if yer old skool). An object is a set of properties and methods that maintain a current state: values specific to that object.

son = new Person("Zachary")
dad = new Person("Adam")

We have two instances of the Person CFC: one for Zachary and one for me. The behaviour - ie: methods - of these Person objects is defined in Person.cfc, but when one calls an object method on a given object, it acts on the state of that specific object. So if one was to call son.getName(), one would get "Zachary"; if one was to call dad.getName(), one would get "Adam". The implementation of getName is the same for both son and dad - as defined by Person - but their implementation acts on the values associated with the object ("Zachary" and "Adam" respectively). You know all this stuff already, but it's just to contextualise things.

So what these object-based properties and methods are to an object; static properties and methods are to the class (the CFC itself). To get that, one has to have clear in one's mind that objects - instances of a class - are not the same thing as the class. Also it's important to understand that a CFC is not simply a file of source code: it still gets loaded into memory before any objects are made, and it is still a data structure in its own right, and it too can have its own properties, and methods to act on them.

We saw above examples of calling methods on an object. We can also call methods on the class itself, without needing an object. EG:

dog = Animal::createFromSpecies("canis familiaris")

Here we have an Animal class - and that is a reference to Animal.cfc, not an object created from Animal.cfc - and it has a factory method which we use to return a Dog object. Internally to createFromSpecies there might be some sort of look-up table that saying "if they ask for a 'canis familiaris', return a new Dog object"; the implementation detail doesn't matter (and I'll get to that), the important bit is that we are calling the method directly on the Animal class, not on an object. We an also reference static properties - properties that relate to the class - via the same :: syntax. I'll show a decent example of that shortly.

How?

Here's a completely contrived class that shows static syntax:

// Behaviour.cfc
component {

    static {
        static.defaultMyVarValue = "set in static constructor"
        static.myVar = static.defaultMyVarValue
    }

    static.myVar = "set in pseudo-constructor"

    public static function resetMyVar() {
        static.myVar = static.defaultMyVarValue
    }
}

There's a coupla relevant bits here.

The static constructor. This is to the class what the init method is to an object. When the class is first loaded, that static constructor is executed. It can only reference other static elements of the class. Obviously it can not access object properties or object methods, because when the static constructor is executed, it's executed on the class itself: there are no objects in play. the syntax of this looks a bit weird, and I don't know why this was picked instead of having:

public static function staticInit() {
    static.defaultMyVarValue = "set in static constructor"
    static.myVar = static.defaultMyVarValue
}

That's more clear I think, and doesn't require any new syntax. Oh well.

Note that when the class is first initialised the pseudo-constructor part of the CFC is not executed. That is only executed when a new object is created from the class.

A static method can only act on other static elements of the class, same as the static constructor, and for the same reason.

I demonstrate the behaviour of this code in its tests:

import testbox.system.BaseSpec
import cfmlInDocker.miscellaneous.static.Behaviour

component extends=BaseSpec {

    function run() {

        describe("Tests for Behaviour class", () => {
            beforeEach(() => {
                Behaviour::resetMyVar()
            })

            it("resets the test variable when resetMyVar is called", () => {
                behaviour = new Behaviour()
                expect(behaviour.myVar).toBe("set in pseudo-constructor")
                expect(Behaviour::myVar).toBe("set in pseudo-constructor")
                Behaviour::resetMyVar()
                expect(behaviour.myVar).toBe("set in static constructor")
                expect(Behaviour::myVar).toBe("set in static constructor")
            })

            it("doesn't use the pseudo-constructor for static values using class reference", () => {
                expect(Behaviour::myVar).toBe("set in static constructor")
            })

            it("does use the pseudo-constructor for static values using object reference", () => {
                behaviour = new Behaviour()
                expect(behaviour.myVar).toBe("set in pseudo-constructor")
            })
        })
    }
}

I need that resetMyVar method there just to make the testing easier. One thing to consider with static properties is that they belong to the class, and that class persists for the life of the JVM, so I want to make the initial state of the class for my tests the same each time. It's important to fully understand that when one sets a static property on a class, that property value will be there for all usages of that class property for the life of the JVM. So it persists across requests, sessions and even the lifetime of the application itself.

Why?

Properties

That Behaviour.cfc example was rubbish. It gives one no sense of why on might want to use static properties or methods. Here's a real world usage of static properties. I have a very cut down implementation of a Response class; like one might have in an MVC framework to return the value from a controller that represents what needs to be sent back to the client agent.

// Response.cfc
component {

    static {
        static.HTTP_OK = 200
        static.HTTP_NOT_FOUND = 404
    }

    public function init(required string content, numeric status=Response::HTTP_OK) {
        this.content = arguments.content
        this.status = arguments.status
    }

    public static function createFromStruct(required struct values) {
        return new Response(values.content, values.status)
    }
}

Here we are using static properties to expose some labelled values that calling code can use to create their response objects. One of its tests demonstrates this:

it("creates a 404 response", () => {
    testContent = "/bogus/path was not found"
    response = new Response(testContent, Response::HTTP_NOT_FOUND)

    expect(response.status).toBe(Response::HTTP_NOT_FOUND)
    expect(response.content).toBe(testContent)
})

Why not just use the literal 404 here? Well for common HTTP status codes like 200 and 404, I think they have ubiquity we could probably get away with. But what about a FORBIDDEN response. What status code is that? 403? Or is it 401? I can never remember, can you? So what wouldn't this be more clear, in the code:

return new Response("nuh-uh, sunshine", Response::HTTP_FORBIDDEN)

I think that's fair enough. But OK, why are they static? Why not just use this.HTTP_FORBIDDEN? Simply to show intended usage. Do HTTP status codes vary from object to object? Would one Response object have a HTTP_BAD_GATEWAY of 502, but another one having a HTTP_BAD_GATEWAY value of 702? No. These properties are not part of an object's state, which is what's implied by using the this scope (or variables scope). They are specific to the Response class; to the concept of what it is to be a Response.

Methods

That Response.cfc has a static method createFromStruct which I was going to use as an example here:

it("returns a Response object with expected values", () => {
    testValues = {
        content = "couldn't find that",
        status = Response::HTTP_NOT_FOUND
    }
    response = Response::createFromStruct(testValues)

    expect(response.status).toBe(testValues.status)
    expect(response.content).toBe(testValues.content)
})

But it occurred to me after I wrote it that in CFML one would not use this strategy: one would simply use response = new Response(argumentCollection=testValues). So I have a request class instead:

// Request.cfc
component {

    public function init(url, form) {
        this.url = arguments.url
        this.form = arguments.form
    }

    public static Request function createFromScopes() {
        return new Request(url, form)
    }
}

This is the other end of the analogy I started with an MVC controller. This is a trimmed down implementation of a Request object that one would pass to one's controller method; encapsulating all the elements of the request that was made. Here I'm only using URL and form values, but a real implementation would also include cookies, headers, CGI values etc too. All controller methods deal in the currency of "requests", so makes sense to pass a Request object into them.

Here we have a basic constructor that takes each property individually. As a rule of thumb, my default constructor always just takes individual values for each property an object might have. And the constructor implementation just assigns those values to the properties, and that's it. Any "special" way of constructing an object (like the previous example where's there's not discrete values, but one struct containing everything), I use a separate method. In this case we have a separate "factory" method that instead of taking values, just "knows" that most of the time our request will comprise the actual URL scope, and the actual form scope. So it takes those accordingly.

That's all fairly obvious, but why is it static? Well, if it wasn't static, we'd need to start with already having an object. And to create the object, we need to give the constructor some values, and that… would defeat the purpose of having the factory method, plus would also push implementation detail of the Request back into the calling code, which is not where that code belongs. We could adjust the constructor to have all the parameters optional, and then chain a call to an object method, eg:

request = new Request().populateFromScopes() 

But I don't think that's semantically as good as having the factory method. But it's still fine, that said.

My rationale for using static methods that way is that sometimes creating the object is harder than just calling its constructor, so wrap it all up in a factory method. Now static methods aren't only for factories like that. Sometimes one might need to implement some behaviour on static properties, and to do that, it generally makes sense to use a static method. One could use an object method, but then one needs to ask "is it the job of this object to be acting on properties belonging to its class?" The answer could very well be "no". So implement the code in the most appropriate way: a static method. But, again, there could be legit situations where it is the job of an object method to act upon static properties. Code in object methods can refer to static properties as much as they need to. Just consider who should be doing the work, and design one's implementation accordingly.

A note on syntax

I could not think of a better place to put this, further up.

I've shown how to call static methods and access static properties via the class, it's: TheClassName::theStaticMethodName() (or TheClassName::theStaticPropertyName). However sometimes you have an object though, and legit need to access static elements of it. In this situation use the dot operator like you usually would when accessing an object's behaviour: someObject.theStaticMethodName() (or someObject.theStaticPropertyName). As per above though, always give thought to whether you ought to be calling the method/property via the object or via the class though. It doesn't matter, but it'll make your code and your intent clearer if you use the most appropriate syntax in the given situation.

Outro

Bottom line it's pretty simple. Just as an object can have state and behaviour; so can the class the object came from. Simple as that. That's all static properties and methods are.

Righto.

--
Adam

 

Oh. There are official docs of sorts: