I think you would get your message across better if you turned this into a blog post that focused on the real issues. The video is too long, with too much fluff. The discussion of the AAA comments is silly - yes I know its late and your cranky - so why dump that on the rest of the world? And then complaining about the test naming because it follows the JP (as you called it) style instead of your own personal style seemed petty, especially since you have little company with that opinion.
I didnt think the content had any value until you got to the ChangePasswordGetReturnsView test, and that was about 10 minutes in. By that point, my spare time was used up, so I turned it off. You had legitimate complaints that someone could learn from - but theyre buried. A quick blog post that takes a few of the bad tests and discusses the issues would be much more valuable.
This is great...please continue. Unit Test are a hot topic but most people don't have a formal process. I'll buy your book!
Thanks, nice review. We want more. Well, I do anyway.
Great video, especially for a test-newbie like me. I think there's need to be more videos/podcasts/whatever that leads people like me that i've started with TDD and grasps the basic but has a hard time implementing it correctly.
Awesome practical walk through of your philosophy around unit testing. It helped me realize I've got areas I can work to improve.
Thanks!
This was fun to watch and educational. I think you've stumbled upon a great form of geek entertainment. Next time, give yourself 100 paper cuts before reviewing the tests.
Awesome video! It was really good and I hope you will create more of these!
I have to agree with Joshua, you would have been better off going to bed and sleeping, before making the video.
Which is a shame because there are things you show where people can learn a better way, writing tests is hard, and mistakes are common, I have made them in the past and probably will in the future, but please remember that pointing out an error is one thing, shooting someone for an error, even with words, is another.
As for the naming, I actually quite like JPs naming conventions, but If you were to look at his tests the class provides the context, which means that you don't get repeating contexts in the names of the tests, each test name is read with the class name as the start of a sentence, its not incorrect, but it is different to your style of naming, both are valid, it is a matter of preference, like the test framework to use.
Enjoyed the video and learned quite a bit.
I am not sure if I agree with Joshua comments, sometimes a video creates a more visual way of describing things and while going through it than a blog. And I appreciate Roy creating one the effort of it counts, even if he is cranky :) Yes dime cast might have 10 mins cast but to do code review you don't want 10 mins, might as well just use Resharper clean code, if you want something quick.
Also one has to remember that just having a blog or being loud does not make someone authoritative in it. Roy had some very valid points in the naming of the test, one form or the other (JP or RO) consistency is the main thing, as you can see some with _ (underscore) and other test name without _
One should really think about the written test code quality, its not just if the test runs everything is good, if you care about your craft one should care about the unit test they write. I have seen test that says ThisShouldBeTrue, and you just wonder about naming, what is true, why its true, etc etc.
Just my 2 cents :)
...and that sucks.
Lol! I thought it was a good video, some good tips on how to write tests.
From the comments I thought I was going to see you screaming or something. I don't think you were that harsh. If anything I'd say stop apologizing in the video. I'd bet that Scott and team have a thick enough skin to view it as well. I think it's their way of a peer review. Why on earth they don't ask (or pay) for you (or others that are experts) to do review a beforehand is way beyond me as their stuff is kind of the "authority" and you calling them out can only be good not only for them but the community as well.
Great review Roy, ashamed it was only half hour long but I guess you do have to sleep inevitably. Looking forward to you doing alot more of these!
I disagree with Joshua. You're entitled to say what you feel, and for however long you feel. And it kept me interested to the end.
Good stuff. Do get some sleep beforehand.
And I'd also agree (not with you, with the test writer) with the whole AAA comments there: it provides structure, just like the naming of the test.
Also - regarding the double assertions - I agree that a test should test only one thing, but if that one thing has several valid and invalid properties? Like, if a string must be over 4 chars long and also begin with a uppercase letter. Would you not test both assertions in one test?
Thanks for the review. I learned several things.
I am curious on what software you were using to do the focus and zooming.
LOL, Harsh but educational. I think this is an excellent format for presenting this much detail. Keep it up!
LOL, harsh but educational. I think video is a great format for diving into this much detail.
Clearly we write tests in very different ways, some of your suggestions I like. One I'm having a lot of difficulty with:
"Use only one Assert per method."
I just can't see myself ever doing that, my test code tends to walk through several uses asserting anything and everything before during and after... Is this really so bad?
I like that you criticize obscure magic strings. But you lost me when you criticized the use of values that change.
Using changing values is a way to inexpensively increase test data coverage. I would much rather have a test that sometimes fails and sometimes passes, than to have a test that is highly likely to not discover the same problems it didn't discover the first time I ran it.
Automated testing already suffers from the poverty that comes from using narrow automated oracles. Let's not compound that by reducing data coverage based on the principle of "don't discover anything new or unexpected."
If you are worried about intermittency, then use logging to preserve the exact values used in each test run.
James -
Regarding tests which sometimes fail - I believe you're wrong. I seem to remember most of the tests I wrote (or saw) which sometimes fail as "ignore if possible" (if the test doesn't break the build - ignore it, if it does - remove it).
I have a big issue with the claim that a "sometimes" test can help you find bugs. That's BS - classic unittests are meant to describe one test scenario and provide you with the safety (regression) net of having it run all the time. You should never expect it to do more, unless you have the specific intent of generating "random" inputs ("fuzz testing").
The problem with having "sometimes" tests is their irreproducibility. You NEED to be able to re-run a failed test and have it fail again in order to fix the bug, and that claim stands true for fuzz tests - you need to keep a failed input as a regression input for later tests.
I loved the video. Don't listen to the "blog about it" comments. There are tons of blogs, but how many video code reviews are there?
Makes me want to get the book.
I say, just keep picking out things from the top projects on CodePlex and go for it. Awesome.
Nicely done. I think @Chris nailed it, pure geek entertainment for me...
Good idea; poor execution.
There were some laughs "Check something for god's sake", and some good ideas, but overall I think your message got lost in a sea of sleep-deprived irritation.
A project like NerdDinner that aims to teach others should be held to the highest standards of quality. Likewise those who comment on it ought to show the highest levels of professionalism, respect and courtesy if they wish their contribution (of critique / of advice) to have a positive impact.
I do think that test reviews are worth doing. Certainly they can be entertaining! But I think you should spend a little more time upfront preparing and be careful not to let your emotions and initial reactions get in the way of the lesson.
Perhaps you could put together a series of timeboxed video reviews. Make them no more than 5 minutes each. Spend 30 minutes preparing beforehand so you can really focus on just a few concerns here and there. Be sure to highlight good or distinctive practices just as you recommend improvements.
One thing that would be really cool in a review would be to actually try to run a couple of tests on supposedly well-covered code. Then make a few reasonable and unreasonable changes to the implementation and see what kinds of things they catch. You might explore simple refactorings, fault injection and feature addition. What happens to the tests? Do they still compile and run properly? Any false positives? How about false negatives? Permissive tests are often bad. Brittle tests are usually worse. It's a tough balance.
BTW, you might be interested to know that MbUnit has two mechanisms for tolerating multple asserts in a test.
#1. Put [MultipleAsserts] on the test.
#2. Use Assert.Multiple(() => { .... }); in the test body.
In each case, the test will report assertion failures, keep running until they finish or throw some other kind of exception, then fail.
This is good for AAA-style tests where the Action part yields a composite result that requires multiple Asserts to verify completely. Of course it's important that the test still remains focused on just one scenario in a context. We don't want multiple asserts on completely unrelated behaviors!
Excellent video for test newbie!
I found this site from a link on Scott Hanselmans facebook profile - did you get any feedback from the guys that wrote the code?
your naming of tests really sux man, you can't be serious...
otherwise, nice video
Thanks for pointing out these issues. I am starting to work the test driven way now and just downloaded the NerdDinner sample application and would have made some stupid errors I am now aware of. Thanks.
Not sure if it's exactly the same as you meant but VSTS testing does let you run one test multiple times with different parameters (row test I think you called it). Check out data driven testing. In this you specify a database (which can be a CSV file), for each row in the database the test is called. This is all set up with attributes on the test and is reasonably straightforward to do
Hi Roy,
love your work.
I miss the RowTests in Visual Studio too!!
We did RowTests using Data-Driven Tests; DataSource attribute with an Input file (our rows) and then reading TestContext.DataRow
Keep it up! I loved it... informal down to earth what your opinion are.. then it is up the eachone to decide to agree or not.
I am glad you mentioned it was late and that you were talking silently because your family is a sleep... I was actually wondering if this would be the case. Now I have seen a few presentations from you and kind of got the feeling on your humor and how you think and speak so I found this video entertaining and educational. I can understand why some here got irritated and annoyed... but that is there choice. I choose to see it in a different light and a very glad someone even bother to make this type of videos so that I don't have to read yet another blog post... there are enough blogs around anyway.
Nice review.Your naming conventions are great!!!
I think I will buy your book.