This would be much easier to test if you made hashedBytes the parameter to the method, rather than plainText. The method, in it's current state, has more responsibilities than it needs to, which makes testing harder.
–
Scroog1Jul 2 '13 at 11:16

2 Answers
2

Note: a more detailed discussion of the code is available at my website. It covers nearly all the points covered here, and shows, step by step, how this piece of code can be refactored into a piece of reliable software.

First, you may want to test the edge cases.

What if plainText is null? You don't check the inputs. Is this intentional, i.e. you rely on GetBytes to throw ArgumentNullException in your place? Is this a mistake?

What if plainText is long, and by long, I mean the maximum allowed by Windows? This is a complicated case, since unit testing it is not obvious (including in terms of performance and memory consumption: nobody wants their unit tests taking all memory available), but you still should consider it.

Hint: the easiest and fastest way to handle this case is to specify the maximum supported length in the product requirements, and then enforce this limit at input validation step.

What if plainText is an empty string?

What if you are calling the method plenty of times? You don't dispose SHA256CryptoServiceProvider, despite the fact that you should, which means that sooner or later, it will fail more or less unpredictably. This is another case where unit testing is complicated/practically impossible.

Second, you're right, you can rely on the methods from .NET Framework and assume they are correctly implemented. This means that you have to test your logic and the way you use those methods:

Write a simple test which calls the method with "Hello World!" as input. Take any non-.NET language: JavaScript, Python, whatever. Compute the SHA256 hash with this language and hardcode the result in your unit test. Compare.

Analyse your code (note: in TDD, you don't do that: you write tests from the use cases, not the actual code). Such analysis should give you the overall view of the flow: where are the conditions, how do you exit the loops, are there guard clauses, etc. The actual flow is easy: a sequence of calls to different methods, then a for. Aside the missing using, the sequence of calls can create a risk of using a variable while it is null. For example:

In your case, there are only constructors, so you don't have to care about null references.

for that you erroneously use (it should have been foreach) is another point to consider. Is the loop made correctly? Do you have to do i < hashedBytes.Length or i <= hashedBytes.Length? When you access the hashedBytes by index, are you sure that the index is never out of range? Could the length be bigger than int.MaxValue? What would happen?

Finally, you're using both byte.ToString and ToLower without specifying the culture. Since you don't say neither that you're expecting the InvariantCulture to be used, it may indicate that you simply forgot to clearly indicate your intent. Both methods use the current culture. This means that you have also to unit test cases where the current culture is not en-US, since the final result of your SHA256 method depends on it (or use formal methods to show that the result would be the same for any culture).

Moreover, byte.ToString("x2") already produces a lowercase string. Converting a lowercase string to lowercase is not needed. Always get rid of the code you don't need.

Conclusion:

Your assumption that you can rely on methods from .NET Framework is true. However, the way you use them should be unit tested. As you can see, in this very short method with simple logic,

there is a potential severe bug which will appear sooner or later when somebody will start to modify this method.

by not disposing objects properly, created a bug which will be very hard to debug later,

made it error prone by wrongly using for when foreach was expected,

made it potentially vulnerable by not checking your inputs (while this is a public method),

made it difficult to guess which exceptions may be thrown by this method,

made its behavior very random when it comes to edge cases (strings too long, null, etc.)

You could have avoided few of those issues with severe testing, but testing alone is not enough. Using other techniques, such as informal code reviews or static checking should be systematic and cover if not all, at least most non-CRUD code you write.

I've edit my question with the updated method. How does it look now? Is there anything in the production code that you would add to address your 'plainText is long' case?
–
Sam LeachJul 2 '13 at 12:54

1

@SamLeach: I'm writing a more complete blog article about the question. Given my tight schedule, I can't give any delay, but I'll post a comment when finished. As for your actual revised code, the only note for now that I have is that ArgumentNullException constructor takes as parameter the name of the parameter, not the message.
–
MainMaJul 2 '13 at 22:12

1

@SamLeach: I've finished the article. The link is available at the top of the answer.
–
MainMaJul 4 '13 at 21:01

In a simple function like this there aren't many tests as unit tests are defined by what output are you expecting the function to return.

You can assume that the built in functions work, but could any of those functions throw an exception based on the input you provide - is that something your function needs to handle?

Tests you might consider - is the resulting string the correct length (if someone comes along and switches SHA256 for another scheme? Are there failure states and how should the function respond to or report them?