Tag: Refactoring

Code Coverage: testing private functions

I was recently discussing whether or not you should directly test the functionality of private methods in a Go project. The other person reasoned for testing public and private functions to ensure 100% test coverage.

Testing private functions in Go is very simple. To do this you put your implementation and tests in the same package. The private functions are available in the test file as they are in the implementation file.

However just because you do not write tests that directly access the private functions, this does not mean you cannot achieve 100% code coverage. In fact if you follow Test Driven Development (TDD) most private functions will only be created during refactoring.

I prefer to only test public functions.

Lets go through a slightly contrived example.

We are going to implement a number package with two functions. One will add one integer to another. The second function will take one integer away from another. Both functions will return the result of the sum as the string representation of the number. i.e. result is 4, the value returned “four”.

We will only return a string for numbers between 0 and 10. If the number is out side of that range we will return “Cannot convert # to a string”.

Add Function

First we implement the functionality of the Add function. Testing and implementing for one simple addition with a result within our range and one addition with a result outside our range.

Tests

package number_test

import (
	"testing"

	"github.com/braddle/blog-testingPrivateFunctions/number"
)

func TestTwoAddTwoReturnsStringFour(t *testing.T) {
	act := number.Add(2, 2)
	exp := "four"

	assertEquals(t, exp, act)
}

func TestSixAndFiveReturnsNumberNotConvertableString(t *testing.T) {
	act := number.Add(6, 5)
	exp := "Cannot convert 11 to a string"

	assertEquals(t, exp, act)
}

func assertEquals(t *testing.T, exp, act string) {
	if act != exp {
		t.Error("Actual value did not match Expected value")
		t.Logf("Expected: %s", exp)
		t.Logf("Actual:   %s", act)
	}

}

Implementation

package number

import "fmt"

// Add function add together the numbers given and returns the result as a 
// string
func Add(a, b int) string {
	num := a + b

	switch num {
	case 4:
		return "four"
	default:
		return fmt.Sprintf("Cannot convert %v to a string", num)
	}
}

Run Tests

$ go test -cover -v
=== RUN   TestTwoAddTwoReturnsStringFour
--- PASS: TestTwoAddTwoReturnsStringFour (0.00s)
=== RUN   TestSixAndFiveReturnsNumberNotConvertableString
--- PASS: TestSixAndFiveReturnsNumberNotConvertableString (0.00s)
PASS
coverage: 50.0% of statements
ok  	github.com/braddle/blog-testingPrivateFunctions/number	0.001s

So far everything looks good. The tests are all passing and we have 100% test coverage.

Adding Minus

Now we implement the functionality of the Minus function. Testing and implementing for one simple subtraction with a result within our range and one subtraction with a result outside our range.

Tests

package number_test

import (
	"testing"

	"github.com/braddle/blog-testingPrivateFunctions/number"
)

// Removed Add Test for Brevity.

func TestSixMinusThreeReturnsStringThree(t *testing.T) {
	act := number.Minus(6, 3)
	exp := "three"

	assertEquals(t, exp, act)
}

func TestThreeMinusSixReturnsNumberNotConvertableString(t *testing.T) {
	act := number.Minus(3, 6)
	exp := "Cannot convert -3 to a string"

	assertEquals(t, exp, act)

}

func assertEquals(t *testing.T, exp, act string) {
	if act != exp {
		t.Error("Actual value did not match Expected value")
		t.Logf("Expected: %s", exp)
		t.Logf("Actual:   %s", act)
	}
}

Implementation

package number

import "fmt"

// Removed Add function for brevity.

// Minus function take the value of b from the value of a and return the 
// result as a string
func Minus(a, b int) string {
	num := a - b

	switch num {
	case 3:
		return "three"
	default:
		return fmt.Sprintf("Cannot convert %v to a string", num)
	}
}

Run Tests

$ go test -cover -v
=== RUN   TestTwoAddTwoReturnsStringFour
--- PASS: TestTwoAddTwoReturnsStringFour (0.00s)
=== RUN   TestSixAndFiveReturnsNumberNotConvertableString
--- PASS: TestSixAndFiveReturnsNumberNotConvertableString (0.00s)
=== RUN   TestSixMinusThreeReturnsStringThree
--- PASS: TestSixMinusThreeReturnsStringThree (0.00s)
=== RUN   TestThreeMinusSixReturnsNumberNotConvertableString
--- PASS: TestThreeMinusSixReturnsNumberNotConvertableString (0.00s)
PASS
coverage: 100.0% of statements
ok  	github.com/braddle/blog-testingPrivateFunctions/number	0.002s

We now have two functions and all the tests are passing with 100% test coverage. So far so good. However we have some duplication, this can be removed with a small refactor.

refactor

We already have all the test we need. We just need to move the duplicate code to it own function. There is currently no need for its functionality to be accessed from outside of the number package so we make it a private function.

package number

import "fmt"

// Add function add together the numbers given and returns the result as a
// string
func Add(a, b int) string {
	num := a + b

	return intToString(num)
}

// Minus function take the value of b from the value of a and return the 
// result as a string
func Minus(a, b int) string {
	num := a - b

	return intToString(num)
}

func intToString(num int) string {
	switch num {
	case 3:
		return "three"
	case 4:
		return "four"
	default:
		return fmt.Sprintf("Cannot convert %v to a string", num)
	}

}

Run Tests

$ go test -cover -v
=== RUN   TestTwoAddTwoReturnsStringFour
--- PASS: TestTwoAddTwoReturnsStringFour (0.00s)
=== RUN   TestSixAndFiveReturnsNumberNotConvertableString
--- PASS: TestSixAndFiveReturnsNumberNotConvertableString (0.00s)
=== RUN   TestSixMinusThreeReturnsStringThree
--- PASS: TestSixMinusThreeReturnsStringThree (0.00s)
=== RUN   TestThreeMinusSixReturnsNumberNotConvertableString
--- PASS: TestThreeMinusSixReturnsNumberNotConvertableString (0.00s)
PASS
coverage: 100.0% of statements
ok  	github.com/braddle/blog-testingPrivateFunctions/number	0.002s

Now when we run the tests they all pass, so our refactor is good. We still have 100% test coverage without having to add new tests to cover our new private function intToString. The moved functionality is being indirectly tested by the existing tests on our public functions.

The completed implementation that ensure all some that result in a value between 0 and 10 can be be found in this Github repository.

Single Table Inheritance

The Doctrine single table inheritance feature maps a number of different entity classes to a single table in your database.

When creating an entity you simple construct the desired class. When you persist the new entity to your data store, Doctrine stores the discriminator (class type identifier) with the rest of the entity’s data.

When retrieving a specific entity from the database Doctrine uses a discriminator to decide which class to construct with the data.

I have recently used the single table inheritance feature to refactor a Price class that used to use a boolean field to determine if the price contained within the entity was inclusive or exclusive of tax.

The previous code that made use of the Price entity looked something like this

$price = $priceRepository->findById(1);

$priceWithTax = null;

if ($price->isTaxIncluded()) {
    $priceWithTax = $price->getPrice();
} else {
    $priceWithTax = $price->getPrice() + $tax;
}

This code is difficult to init test this calling code given the number of routes through it.

However if we use single table inheritance we can refactor out the conditionals for Polymorphism.

Before I started to refactor the functionality the entity’s configuration YAML looked something like this

Slapi\Model\Product\Price\Price:
  type: entity
  table: price
  id:
    id:
      type: integer
      generator:
        strategy: AUTO
  fields:
    price:
      type: float
    taxIncluded:
      type: boolean

I then changed the YAML set up to something similar to this

# Model.Price.Price.yml
Model\Price\Price:
  type: entity
  table: price
  inheritanceType: SINGLE_TABLE
  discriminatorColumn:
    name: type
    type: integer
  discriminatorMap:
    including_tax: IncludingTax
    excluding_tax: ExcludingTax
  id:
    id:
      type: integer
      generator:
        strategy: AUTO
  fields:
    price:
      type: float

# Model.Price.IncludingTax.yml
Model\Price\IncludingTax:
  type: entity

# Model.Price.ExcludingTax.yml
Model\Price\ExcludingTax:
  type: entity

The new configuration gives us the following classes.

class price
{
    /**
     * @var float
     */
    private $price;

    /**
     * @return float
     */
    abstract public function getPrice();
}

class ExcludingTax extends Price
{
    /**
     * @return float
     */
    public function getPrice()
    {
        return $this->price + $tax;
    }
}

class IncludingTax extends Price
{
    /**
     * @return float
     */
    public function getPrice()
    {
         return $this->price;
    }
}

After the refactoring the entity to use single table inheritance to create a PriceIncludingTax and PriceExcludingTax class the code makes use of the price entities now looks something like this

$price = $priceRepository->findById(1);

$priceWithTax =  $price->getPrice();

The code in the second example is much easier to read and has no knowledge of the fact that a price entity return from the price repository could be inclusive or exclusive of tax. The calling code is also a lot easier to unit test as there is now only a single route through it.