9
\$\begingroup\$

I know I can probably use MSTest (I'm under Linux without an IDE so maybe not?) or another unit testing library, but for a small project I decided to write my own unit tests without resorting to a framework (for fun and for learning).

Right now, I have only one type of assert and one test but I would like a code review to improve my code now to make sure I am on the right track.

module Tests

// modules under test
open HeightMap
open MidpointDisplacement

// assert functions
let assertAreEqual expected actual =
    printf "... "
    if expected <> actual then printfn "%s" ("Test failed, expected " + expected.ToString() + ", actual " + actual.ToString())  
    else printfn "Test passed"

// tests
let testNewHeightMapReturnZeroInitializedHm () = 
    let hm = newHeightMap 5
    let result = hm.Map |> Array.sum
    assertAreEqual 0.0 result

// tests included in run
let testsToRun = 
    [
        ("test newHeightMap will return a 0 initialized height map",
         testNewHeightMapReturnZeroInitializedHm)
    ]

// test runner
let runSingleTest test = 
    let testName, testFunction = test
    printf "%s" testName    
    testFunction()

let runTests =
    testsToRun |> List.map runSingleTest |> ignore
    printfn "%s" "Ran all tests."

One thing that bugs me is that I there is some redundancy by specifying the test function and then including it in a list inside a tuple.

\$\endgroup\$
1

2 Answers 2

7
\$\begingroup\$

Overall this looks ok, just a couple nitpicks:

  1. In assertAreEqual, you're using string concatenation and then passing the resulting string to formatting function. You could use the formatting function to begin with:

    if expected <> actual then 
        printfn "Test failed, expected %A, actual %A" expected actual  
    else 
        printfn "Test passed"
    
  2. Parentheses around tuples are not required, so:

    let testsToRun = 
    [
        "test newHeightMap will return a 0 initialized height map",
        testNewHeightMapReturnZeroInitializedHm
    ]
    
  3. Function parameters are actually patterns, so you can destructure the tuple right there, instead of on a separate line:

    let runSingleTest (testName, testFunction) = 
        ...
    
  4. As a general rule, if you have to use ignore, it means that there is something wrong somewhere. ignore is a hack, a way to squeeze a round peg in a square hole. In your particular case, you're mapping over a list solely for the side-effects. To do this, use List.iter, which returns unit, so no need for `ignore.

    testsToRun |> List.iter runSingleTest
    
  5. The pervasive use of side-effecting computations (i.e. printfn etc.) sets off my spidey sense: this makes your code inflexible (e.g. what if you want to have different reporting formats - one for console run, one for CI server, etc.).
    You should structure your code in such a way that it doesn't produce side effects as it runs, but instead produces a value that describes the run, and which can later be used (or not used) to produce the effects. For example, in your case, rather than printing out errors, I would return a list of them, and have the consumer decide what to do with that list. Of course, this would mean significant restructuring of your whole program, so I won't give a code example.

  6. Finally, "parametrize all the things!" Try to take stuff as parameters as much as possible, rather than relying on stuff existing in the environment. For example, the runTests function should really take the list of tests as parameter.

  7. As far as the thing that bugs you, there is an easy solution for it: you don't have to name your functions, you can declare them inline as lambda-expressions:

    let testsToRun = 
        [
            "test newHeightMap will return a 0 initialized height map",
            fun() -> 
                let hm = newHeightMap 5
                let result = hm.Map |> Array.sum
                assertAreEqual 0.0 result
        ]
    
  8. Or, alternatively, you can give a name to the pair of name+function, and then include that pair in the list:

    let theTest = "test newHeightMap will return a 0 initialized height map", fun() -> 
        let hm = newHeightMap 5
        let result = hm.Map |> Array.sum
        assertAreEqual 0.0 result
    
    let testsToRun = [ theTest ]
    
\$\endgroup\$
1
  • \$\begingroup\$ Just added two points about the "thing that bugs you". Didn't notice it at first, sorry. \$\endgroup\$ Mar 1, 2016 at 14:12
0
\$\begingroup\$

One thing that bugs me is that I there is some redundancy by specifying the test function and then including it in a list inside a tuple.

That's a familiar smell. I came across the same thing when I was building a test framework in . Fortunately, you've got better tools at your disposal than I did.

This is easiest handled by creating a custom attribute, marking your test methods with it, and then reflecting over the code to generate a list of test methods to be run on the fly. Implementation is left as an exercise for the reader.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.