\$\begingroup\$

You implementation makes assumptions about how Builder.add() is called. If they are not explicitly documented, they are errors in the code.

Divisors not added in increasing order: @Test public void BuilderAdd_DifferentDivosorOrder_SameOutput() { FizzBuzz fizzBuzz = FizzBuzz.builder().add("Fizz", 3).add("Buzz", 5).build(); FizzBuzz buzzFizz = FizzBuzz.builder().add("Buzz", 5).add("Fizz", 3).build(); assertEquals(fizzBuzz.getValue(15), buzzFizz.getValue(15)); }

A divisor is only added once: For this case you need to deice how to handle it. Currently both strings will be output. Should the second call take precedence, be ignored, cause an exception? @Test public void BuilderAdd_DivosorAddedMultipleTimes_EXPECTED_RESULT() { FizzBuzz fizzBuzz = FizzBuzz.builder().add("Fizz", 3).add("Buzz", 3).build(); // assert expected result }

Your test names are fairly generic. When a test fails the first thing you see is the test name. A good test name should be able to give you a good idea of what might be failing even before you see the test. Similarly, when someone is reading the test code for the first time, the shouldn't have to work to get a sense for the intent of the test.

Part of this issue is a result of your test cases being general categories. With this specific setup, try a number of things. Instead it is better to have many specific test cases instead of a few general test cases.

The biggest issue with doing this is that once one thing fails, no other assertions are executed. If there many things broken, you want to know about all of them. What you don't want is to find out that the thing you just fixed was only part of the problem and other things are still failing.

A good test case has three main sections: Arrange, Act, and Assert. In your test cases, you are asserting many unrelated things. Checking a multiple of one is one test. Checking a multiple of both is a different test. Checking a non-multiple of both is yet another test.

When you write your test cases like this, giving specific names becomes much easier. Look at the test case I added to show the error in your code. The name tells you:

What is being tested.

The context in which it is being tested.

The expected result.

If this test fails, I can tell you exactly what is wrong without looking at the code of the test case. Any one seeing the code for the first time doesn't have to think about what was the intent of this specific assertion.

Of the test cases you have, then seem to follow that same pattern. For the most part, they only differ on the inputs. Just like with production code, repeated code in tests is a bad thing and can lead to errors.

You can use inheritance to create the core functionality to a set of tests that are similar. An abstract test class can implement a number of test cases. Then you create a subclass for each specific situation. Each subclass can implement a abstract method that produces the FizzBuzz to use in each test case.

For tests when the test cases are testing the same thing, but need to try many different inputs values, you can use parameterized test. This is one good way to avoid the issue of having the first failure prevent other assertions from being executed.