0

I have a method I want to unit test, one that checks for a match between three cards. Because the cards are randomly generated, there's no way to set up three cards that I know will or won't match. I need to do this to unit test my isMatch() method.

Is it acceptable to alter my Card class to add a method to explicitly set its value so I can unit test it? In general is it acceptable to make small additions to source code to make unit tests possible or is there a better -- or correct -- way to do it?

Hal50000
  • 639
  • 1
  • 6
  • 16
  • 1
    You should inject the source of randomness, the `Random` instance, into your class. In your tests, you can then create a known random sequence and test the logic. Code should be written to be testable. If it isn't testable then, yes, it should be changed. – Boris the Spider Apr 12 '15 at 22:49
  • Are you not using a seed for your randomness? No little log file where you write it, so you can recreate? – Bill Woodger Apr 12 '15 at 22:49
  • @BoristheSpider, can you elaborate on injecting the `Random` instance? I'm not clear on what you mean. – Hal50000 Apr 12 '15 at 23:00
  • 2
    Read up on the [Inversion of Control](http://en.m.wikipedia.org/wiki/Inversion_of_control) pattern. – Boris the Spider Apr 12 '15 at 23:04
  • +1 for [Boris the Spider's](http://stackoverflow.com/users/2071828/boris-the-spider) comment, however I think the term [Dependency Injection](http://en.wikipedia.org/wiki/Dependency_injection) is a little bit easier to understand and comprehend. – hooknc Apr 13 '15 at 16:48
  • Yes, I dependency injection, also previously known as passing a reference. The problem is the infatuation with the design pattern here completely ignores the actual issue. Consider the problem:, given a set of cards to compare, no test based on a known set of random numbers will give you a useful test. You can test a cached or canned or repeatable set of random numbers, but that won't prove your comparator is working correctly. You need a known set of cards that will pass the test and a known set of cards that will fail it. The random numbers are irrelevant in the test. – Hal50000 Apr 15 '15 at 05:23

2 Answers2

1

Don't know what your setup is, but why not make the card generator a pluggable component of your class and fake a class that is guaranteed to return three matching cards?

You could then fake a class that is guaranteed to return three cards that do not match.

Community
  • 1
  • 1
hooknc
  • 4,854
  • 5
  • 31
  • 60
  • I considered using a card generator, or more specifically a CardFactory or CardBuilder. The issue is the Card class doesn't require a lot of top down creation. So I wrote the card class to manage its own randomization. When other classes request a card, they only need to call the default constructor and they can expect a randomized card. I believe the bottom-up design to be better, but it did require adding a little more code to explicitly create cards so the `isMatch` method could be tested. – Hal50000 Apr 13 '15 at 12:17
  • It is of course fine to do what you're doing, but adding the randomization to the Card class is now, you can never have a different strategy for generating cards. It is a fine decision, but it does come with some consequences as you've most likely noticed. Like the inability to write a clear and concise unit test. [Uncle Bob](https://sites.google.com/site/unclebobconsultingllc/) would say that your Card class is doing two things and in general a class should only ever do one thing. – hooknc Apr 13 '15 at 16:46
  • 1
    It would be trivial to alter the Card interface to take a Configuration object. After all, I'm not writing an API, so the phrase "never have a different strategy for generating cards" isn't exactly carved in stone. Your suggestion is a good one, and I did consider it when originally writing it. The problem is without the self-configuration, the Card class is just a data structure, holding the properties of the card; that's not good design either. As you mention, the whole set up is not known to you, but I thought your answer was most useful. – Hal50000 Apr 15 '15 at 05:58
-5

No. You should not modify your code for your Unit test and the modify it back, so "the code runs". For the described problem you have @Before. Design your classes around this. Construct three deterministic cards and compare them. With this annotation you can test the functionality of your code without "modifying your code for unit testing".

Turing85
  • 18,217
  • 7
  • 33
  • 58
  • I am using @before. That's not the problem, the problem is the current interface has no deterministic method for creating cards. It's not a matter of the code running. It runs in both cases, in the original and if I add a method to make cards deterministically. I'm just asking if it's "right" to add a method to the source so the test can be done. – Hal50000 Apr 12 '15 at 22:57
  • 2
    @Hal50000 That is absolutely fine. – Turing85 Apr 12 '15 at 23:02