Explicit vs DRY tests

Chat / Discussions
1

Hello alchemists! :woman_scientist: :man_scientist:

Tests are supposed to be readable/maintainable as every code should be in a perfect world. And explicitness is a good thing for readability.

Beginning my “one kata per day”, I wrote FizzBuzz tests in two ways. I don’t really know which style I prefer: one is very explicit and less concise. The other one is more in a DRY style.

What’s your opinion about this? :face_with_monocle:

Version 1:

defmodule Kata.FizzBuzz.Test do
  use ExUnit.Case

  import Kata.FizzBuzz

  test "1, 2, 4, 7, 8 returns number as a string" do
    assert fizz_buzz(1) == "1"
    assert fizz_buzz(2) == "2"
    assert fizz_buzz(4) == "4"
    assert fizz_buzz(7) == "7"
    assert fizz_buzz(8) == "8"
  end

  test "3, 6, 9, 12 returns Fizz" do
    assert fizz_buzz(3) == "Fizz"
    assert fizz_buzz(6) == "Fizz"
    assert fizz_buzz(9) == "Fizz"
    assert fizz_buzz(12) == "Fizz"
  end

  test "5, 10, 20, 25 returns Buzz" do
    assert fizz_buzz(5) == "Buzz"
    assert fizz_buzz(10) == "Buzz"
    assert fizz_buzz(20) == "Buzz"
    assert fizz_buzz(25) == "Buzz"
  end

  test "15, 30, 45, 60 returns FizzBuzz" do
    assert fizz_buzz(15) == "FizzBuzz"
    assert fizz_buzz(30) == "FizzBuzz"
    assert fizz_buzz(45) == "FizzBuzz"
    assert fizz_buzz(60) == "FizzBuzz"
  end
end

Version 2:

defmodule Kata.FizzBuzz.Test do
  use ExUnit.Case

  import Kata.FizzBuzz

  test "1, 2, 4, 7, 8 returns number as a string" do
    [1, 2, 4, 7, 8]
    |> Enum.each(fn n -> assert fizz_buzz(n) == "#{n}" end)
  end

  test "3, 6, 9, 12 returns Fizz" do
    [3, 6, 9, 12]
    |> Enum.each(fn n -> assert fizz_buzz(n) == "Fizz" end)
  end

  test "5, 10, 20, 25 returns Buzz" do
    [5, 10, 20, 25]
    |> Enum.each(fn n -> assert fizz_buzz(n) == "Buzz" end)
  end

  test "15, 30, 45, 60 returns FizzBuzz" do
    [15, 30, 45, 60]
    |> Enum.each(fn n -> assert fizz_buzz(n) == "FizzBuzz" end)
  end
end
2

Hmm, this is pretty subjective, but I’d personally go with a variant on Version 2 that swaps Enum.each for a for comprehension. I find it just reads better since it’s so close to spoken language i.e. “for each of these numbers, assert this condition”

  test "1, 2, 4, 7, 8 returns number as a string" do
    for n <- [1, 2, 4, 7, 8], do: assert fizz_buzz(n) == "#{n}"
  end

Since it’s generally good practice to avoid “magic strings” – or in this case numbers – when writing tests, we could generate the multiples. It’s probably a bit much to test something like FizzBuzz, but since it’s an exercise… might as well!

  test "multiple of fifteen aka three and five returns FizzBuzz" do
    for n <- 1..10, do: assert fizz_buzz(15 * n) == "FizzBuzz"
  end

Or maybe even throw in an Enum.random instead.

  test "multiple of fifteen aka three and five returns FizzBuzz" do
    assert fizz_buzz(15 * Enum.random(1..100)) == "FizzBuzz"
  end
1 Like
3

To me the test labels also stand out the most. They should at best specify what needs to happen and not just repeat the examples.

At that point property testing (e.g. using stream_data) might become the more appropriate tool to use.

3 Likes
4

I took all of your feedback into account; except the use of a random because having non predictable tests fears me :scream:

This is how I would write it now:

defmodule Kata.FizzBuzz.Test do
  use ExUnit.Case

  import Kata.FizzBuzz

  test "non multiples of 3 and 5 returns the number as a string" do
    for n <- 1..100, rem(n, 3) != 0, rem(n, 5) != 0 do
      assert fizz_buzz(n) == Integer.to_string(n)
    end
  end

  test "multiples of 3 but not of 5 returns Fizz" do
    for n <- 1..100, rem(n, 3) == 0, rem(n, 5) != 0 do
      assert fizz_buzz(n) == "Fizz"
    end
  end

  test "multiples of 5 but not of 3 returns Fizz" do
    for n <- 1..100, rem(n, 3) != 0, rem(n, 5) == 0 do
      assert fizz_buzz(n) == "Buzz"
    end
  end

  test "multiples of 3 and 5 returns Fizz" do
    for n <- 1..100, rem(n, 3 * 5) == 0 do
      assert fizz_buzz(n) == "FizzBuzz"
    end
  end
end

Seems nice to me! :star_struck:

However, this testing strategy seems to be a bit CPU intensive for a trivial test? Maybe that’s why you suggest the use of a random? :face_with_monocle:

5

I think I am an outlier when it comes to tests, but I would not include any logic to generate assertions in unit tests. I would just choose one type of input for each type of output, with a test for each. If this logic was extra critical or issues keep popping up with unexpected inputs, I guess I would consider adding some sort of property based test to supplement the unit tests, but I would consider carefully before doing it.

4 Likes
6

I agree here. It’s nice having very explicit, clear failures and not having to guess which one caused a failure. The assert message may provide a clear failure in this case. I think one should default to minimum abstraction in your tests, if there are no other pressing concerns like complexity of function under test.

I usually look at the failure message from my tests to decide whether I like the style of the test.

2 Likes
7

ExUnit outputs me this in an error case:

  1) test &fizz_buzz/1: multiples of 3 and 5 returns Fizz (Kata.FizzBuzz.Test)
     test/kata/fizz_buzz_test.exs:27
     Assertion with == failed
     code:  assert fizz_buzz(n) == "FizzBuzz"
     left:  "FizzBug"
     right: "FizzBuzz"

It point the problem: we would liked to know the value of n in this case :face_with_peeking_eye:

I’m surprised ExUnit does not already print out the values of the variables used in the failing code.

2 Likes
8

My approach, as your is IMHO too repeating the implementation:

defmodule Kata.FizzBuzzTest do
  use ExUnit.Case
  use ExUnitProperties

  property "multiples of 3 contains `Fizz`" do
    check all n <- integer() do
      result = fizz_buzz(n * 3)
      assert String.contains?(result, "Fizz")
    end
  end

  property "multiples of 5 contains `Buzz`" do
    check all n <- integer() do
      result = fizz_buzz(n * 5)
      assert String.contains?(result, "Buzz")
    end
  end

  property "returned value is a binary" do
    check all n <- integer() do
      assert is_binary(fizz_buzz(n))
    end
  end

  property "result for 'plain' values is stringified integer" do
    check all n <- integer(), rem(n, 3) * rem(n, 5) != 0 do
      assert fizz_buzz(n) == Integer.to_string(n)
    end
  end
end

There probably could be some additional properties like the above

7 Likes
9

True! Maybe because it tests all the case from 1 to 100 (or more easily), which is maybe not the purpose of a readable (as a spec) unit test.

I think it’s a bit too much to have a dependency to stream_data for a trivial unit test, I will avoid to use it for now :thinking:

I will start to apply a combination of the version 1 and 3 :grin:

10

After all, I really consider it as a nice feature proposal for ExUnit :grimacing:

11

For me stream_data is a way to go from day one. It is “must have” just like credo and ex_doc.

3 Likes
12

You can get around this by making a loop around the “test” instead of the “assert” and putting the test value into the test name, but I generally don’t bother with the loop because of reasons stated above.

1 Like
13

Weird how nobody did tests to get best from both worlds:

for n <- [1, 2, 4, 7, 8] do
  n = Macro.escape(n)

  test "fizz_buzz(#{n}) returns the input number as a string" do
    n = unquote(n)
    assert fizz_buzz(n) == "#{n}"
  end
end

Basically have your code generate tests. And if one fails you’ll have an exact test title + assert diff showing exactly what’s wrong.

I do such tests any chance I get because it helps cover several potentially-gotcha values.

But if we’re going into that territory then I’ll side with @hauleth – when you need to prove properties of your code then property testing is what you want. And it’s not an overkill at all, you use it 4-5 times and you get used to it. I have never spent more than 20-30 minutes adding property tests once I learned stream_data (which admittedly took me a weekend of tinkering, though it was well worth it).

2 Likes
14

I always prefer explicit tests. Yes, they are more tedious to write, but much easier to understand when they start to fail.

Also, if trying to keep tests as DRY as production code then at one point they might get so complex that they need their own tests.

2 Likes