Code reviews and Unit tests
-
Here we use manual unit testing - no fancy automated tools, and it's too late in the project to change anything now. For some reason unknown to me, it has been decided that every time we check in a file, it has to be code reviewed and unit tested. The frustrating thing is I've just completed some cosmetic changes (spelling errors in comments, incorrect indentation etc...) resulting from a previous code review, which I now have to run 4 unit tests and a code review for. Frustrating since I didn't actually touch any of the code - it was all formatting changes. And yes, the previous code review has already been signed off by my tech lead verifying that my changes are correct. Please somebody tell me this is not a ridiculous situation...
Ryan
"Punctuality is only a virtue for those who aren't smart enough to think of good excuses for being late" John Nichol "Point Of Impact"
You have to run four whole unit tests?! You poor thing. You should retain a lawyer at once and get started on the lawsuit. Matt Gerrans
-
Here we use manual unit testing - no fancy automated tools, and it's too late in the project to change anything now. For some reason unknown to me, it has been decided that every time we check in a file, it has to be code reviewed and unit tested. The frustrating thing is I've just completed some cosmetic changes (spelling errors in comments, incorrect indentation etc...) resulting from a previous code review, which I now have to run 4 unit tests and a code review for. Frustrating since I didn't actually touch any of the code - it was all formatting changes. And yes, the previous code review has already been signed off by my tech lead verifying that my changes are correct. Please somebody tell me this is not a ridiculous situation...
Ryan
"Punctuality is only a virtue for those who aren't smart enough to think of good excuses for being late" John Nichol "Point Of Impact"
Ryan Binns wrote: run 4 unit tests Wow! How do you guys end up writing so many tests? I see dead pixels Yes, even I am blogging now!
-
You have to run four whole unit tests?! You poor thing. You should retain a lawyer at once and get started on the lawsuit. Matt Gerrans
Matt Gerrans wrote: You have to run four whole unit tests?! You poor thing. I've run these four unit tests before. Last time, it took two days, and everything passed. It just seems a bit pointless having to run them again, knowing they're going to pass because the only difference in the code between this time and last time is a bit of indentation and a couple of spelling errors in comments. If I had actually changed any code, then I wouldn't have any problems with running them. Normally, I wouldn't mind too much. It's just these four that are painful.
Ryan
"Punctuality is only a virtue for those who aren't smart enough to think of good excuses for being late" John Nichol "Point Of Impact"
-
Here we use manual unit testing - no fancy automated tools, and it's too late in the project to change anything now. For some reason unknown to me, it has been decided that every time we check in a file, it has to be code reviewed and unit tested. The frustrating thing is I've just completed some cosmetic changes (spelling errors in comments, incorrect indentation etc...) resulting from a previous code review, which I now have to run 4 unit tests and a code review for. Frustrating since I didn't actually touch any of the code - it was all formatting changes. And yes, the previous code review has already been signed off by my tech lead verifying that my changes are correct. Please somebody tell me this is not a ridiculous situation...
Ryan
"Punctuality is only a virtue for those who aren't smart enough to think of good excuses for being late" John Nichol "Point Of Impact"
Actually, once you've been programming long enough, nothing surprises you any more as a source of bugs. That includes changing comments and formatting - I've seen it happen. More to the point, why aren't your unit tests automated? That's the real issue. It'd be so easy to write something that monitors your source control system, and run the unit tests every time something changes (or at least once a day). I've heard that computers are really good at repetitive, boring tasks :-) It never ceases to amaze me how reluctant people who write software to solve problems for a living are to write software to solve their own problems :-)
Lets be honest, isn't it amazing how many truly stupid people you meet during the course of the day. Carry around a pad and pencil, you'll have twenty or thirty names by the end of the day - George Carlin Awasu 2.1.1 [^]: A free RSS reader with support for Code Project.
-
Actually, once you've been programming long enough, nothing surprises you any more as a source of bugs. That includes changing comments and formatting - I've seen it happen. More to the point, why aren't your unit tests automated? That's the real issue. It'd be so easy to write something that monitors your source control system, and run the unit tests every time something changes (or at least once a day). I've heard that computers are really good at repetitive, boring tasks :-) It never ceases to amaze me how reluctant people who write software to solve problems for a living are to write software to solve their own problems :-)
Lets be honest, isn't it amazing how many truly stupid people you meet during the course of the day. Carry around a pad and pencil, you'll have twenty or thirty names by the end of the day - George Carlin Awasu 2.1.1 [^]: A free RSS reader with support for Code Project.
Taka Muraoka wrote: More to the point, why aren't your unit tests automated? I have no idea. The people who originally wrote the software (who have all since left) didn't allow for it in the design. The classes are now so incredibly entangled (our dependency graph is something to marvel at ;)) that it would be impossible to attempt to separate them out to unit test them separately. We attempted to do it for a couple of classes last year to see how much effort it would take, and gave up in despair. I agree that it would definitely be the best solution though. I've had to implement a few new classes, and every one of them is written with the ability to unit test them automatically. Unfortunately, we just can't do this for all the classes. As for this situation, our diff program ignores whitespace, and it reports only a single line changed in the before and after files - that line is purely a single-line comment. The output from the C preprocessor is identical for both files - a standard diff picks up no differences at all. I can guarantee that the functionality is identical based on that alone.
Ryan
"Punctuality is only a virtue for those who aren't smart enough to think of good excuses for being late" John Nichol "Point Of Impact"
-
Taka Muraoka wrote: More to the point, why aren't your unit tests automated? I have no idea. The people who originally wrote the software (who have all since left) didn't allow for it in the design. The classes are now so incredibly entangled (our dependency graph is something to marvel at ;)) that it would be impossible to attempt to separate them out to unit test them separately. We attempted to do it for a couple of classes last year to see how much effort it would take, and gave up in despair. I agree that it would definitely be the best solution though. I've had to implement a few new classes, and every one of them is written with the ability to unit test them automatically. Unfortunately, we just can't do this for all the classes. As for this situation, our diff program ignores whitespace, and it reports only a single line changed in the before and after files - that line is purely a single-line comment. The output from the C preprocessor is identical for both files - a standard diff picks up no differences at all. I can guarantee that the functionality is identical based on that alone.
Ryan
"Punctuality is only a virtue for those who aren't smart enough to think of good excuses for being late" John Nichol "Point Of Impact"
I feel your frustration, but surely it's good to have such a rigid safety net between you and your production environment? Cheers, Simon sig ::
"Don't try to be like Jackie. There is only one Jackie.... Study computers instead.", Jackie Chan on career choices.
article :: animation mechanics in SVG blog:: brokenkeyboards
"Most of us are programmers, but a few use VB", Christian Graus -
Taka Muraoka wrote: More to the point, why aren't your unit tests automated? I have no idea. The people who originally wrote the software (who have all since left) didn't allow for it in the design. The classes are now so incredibly entangled (our dependency graph is something to marvel at ;)) that it would be impossible to attempt to separate them out to unit test them separately. We attempted to do it for a couple of classes last year to see how much effort it would take, and gave up in despair. I agree that it would definitely be the best solution though. I've had to implement a few new classes, and every one of them is written with the ability to unit test them automatically. Unfortunately, we just can't do this for all the classes. As for this situation, our diff program ignores whitespace, and it reports only a single line changed in the before and after files - that line is purely a single-line comment. The output from the C preprocessor is identical for both files - a standard diff picks up no differences at all. I can guarantee that the functionality is identical based on that alone.
Ryan
"Punctuality is only a virtue for those who aren't smart enough to think of good excuses for being late" John Nichol "Point Of Impact"
I was talking about the unit tests you have now. Why aren't *they* automated? Automatate them and it gives you a framework within which you can start to write some more. As for running a diff, you're probably right in that there's no mistake. I just wish I had a dollar for every time I've heard a dev say "there's NO WAY this could possibly cause any problems..." :-)
Lets be honest, isn't it amazing how many truly stupid people you meet during the course of the day. Carry around a pad and pencil, you'll have twenty or thirty names by the end of the day - George Carlin Awasu 2.1.1 [^]: A free RSS reader with support for Code Project.
-
Matt Gerrans wrote: You have to run four whole unit tests?! You poor thing. I've run these four unit tests before. Last time, it took two days, and everything passed. It just seems a bit pointless having to run them again, knowing they're going to pass because the only difference in the code between this time and last time is a bit of indentation and a couple of spelling errors in comments. If I had actually changed any code, then I wouldn't have any problems with running them. Normally, I wouldn't mind too much. It's just these four that are painful.
Ryan
"Punctuality is only a virtue for those who aren't smart enough to think of good excuses for being late" John Nichol "Point Of Impact"
Ryan Binns wrote: I've run these four unit tests before. Last time, it took two days, and everything passed A unit test that takes two days is not a unit test. What you're running is an Acceptance Test. Your lead should know the difference. A unit test should take milliseconds to run, and each unit test tests one and only one very specific function of one specific method in one class. Now, you may have tens of thousands of unit tests eventually, but that's a different matter. Ryan Binns wrote: the only difference in the code between this time and last time is a bit of indentation and a couple of spelling errors in comments. I remember making a change to a comment, or thinking that's what I did, and something actually broke in the code. While it was entirely my fault, I learned that I cannot trust myself when I say "I only changed the comments". :) Marc MyXaml Advanced Unit Testing YAPO
-
I was talking about the unit tests you have now. Why aren't *they* automated? Automatate them and it gives you a framework within which you can start to write some more. As for running a diff, you're probably right in that there's no mistake. I just wish I had a dollar for every time I've heard a dev say "there's NO WAY this could possibly cause any problems..." :-)
Lets be honest, isn't it amazing how many truly stupid people you meet during the course of the day. Carry around a pad and pencil, you'll have twenty or thirty names by the end of the day - George Carlin Awasu 2.1.1 [^]: A free RSS reader with support for Code Project.
Taka Muraoka wrote: I was talking about the unit tests you have now. Why aren't *they* automated? Automatate them and it gives you a framework within which you can start to write some more. It would be incredibly difficult. We're talking about a distributed system running across 3 machines, where our unit tests involve pushing buttons on one machine to verify as much of a module running on the second machine as possible, through looking at the results sent to external subsystems via the third machine and the GUI located on the first machine. I'd like to see someone attempt to automate that without rewriting half the software, especially since half the modules that I need to test (the second machine is my domain) are inexplicably linked to 10 or 20 others. As Marc indicated above, they're probably not technically unit tests, but they're the closest to it that we do. Taka Muraoka wrote: I just wish I had a dollar for every time I've heard a dev say "there's NO WAY this could possibly cause any problems..." Ditto. I've said that many times, but at least I always test the code thoroughly before I check it in ;) Just as an aside, I'm looking at taking a new job at a company that actually does unit testing properly... :~
Ryan
"Punctuality is only a virtue for those who aren't smart enough to think of good excuses for being late" John Nichol "Point Of Impact"
-
Ryan Binns wrote: I've run these four unit tests before. Last time, it took two days, and everything passed A unit test that takes two days is not a unit test. What you're running is an Acceptance Test. Your lead should know the difference. A unit test should take milliseconds to run, and each unit test tests one and only one very specific function of one specific method in one class. Now, you may have tens of thousands of unit tests eventually, but that's a different matter. Ryan Binns wrote: the only difference in the code between this time and last time is a bit of indentation and a couple of spelling errors in comments. I remember making a change to a comment, or thinking that's what I did, and something actually broke in the code. While it was entirely my fault, I learned that I cannot trust myself when I say "I only changed the comments". :) Marc MyXaml Advanced Unit Testing YAPO
Marc Clifton wrote: A unit test that takes two days is not a unit test. What you're running is an Acceptance Test. Your lead should know the difference. Yeah, we all know that. This is the closest we've got. We've tried to separate out modules/methods to be able to test them automatically, but it was practically impossible, and we gave up after a week or attempting to extract a single method. The original design was, well, non-existant, and we just have to live with it unfortunately. We change things as we can, and any new code that is introduced is written with the ability to do automated testing built in, but we just don't have the time or budget to go changing everything. It's a five year old project, and there have been more major design changes than developers on the project :~ Marc Clifton wrote: I remember making a change to a comment, or thinking that's what I did, and something actually broke in the code. While it was entirely my fault, I learned that I cannot trust myself when I say "I only changed the comments". I've done that before as well, which is why this time I actually verified the preprocessor output (it's C++) before and after using a diff, and they were identical. If someone can explain to me how identical code can have different behaviour, I'd love to hear it :) I never trust my changes, so I always perform a diff on code changes I make, just to make sure what I thought I changed is what I actually changed.
Ryan
"Punctuality is only a virtue for those who aren't smart enough to think of good excuses for being late" John Nichol "Point Of Impact"
-
Marc Clifton wrote: A unit test that takes two days is not a unit test. What you're running is an Acceptance Test. Your lead should know the difference. Yeah, we all know that. This is the closest we've got. We've tried to separate out modules/methods to be able to test them automatically, but it was practically impossible, and we gave up after a week or attempting to extract a single method. The original design was, well, non-existant, and we just have to live with it unfortunately. We change things as we can, and any new code that is introduced is written with the ability to do automated testing built in, but we just don't have the time or budget to go changing everything. It's a five year old project, and there have been more major design changes than developers on the project :~ Marc Clifton wrote: I remember making a change to a comment, or thinking that's what I did, and something actually broke in the code. While it was entirely my fault, I learned that I cannot trust myself when I say "I only changed the comments". I've done that before as well, which is why this time I actually verified the preprocessor output (it's C++) before and after using a diff, and they were identical. If someone can explain to me how identical code can have different behaviour, I'd love to hear it :) I never trust my changes, so I always perform a diff on code changes I make, just to make sure what I thought I changed is what I actually changed.
Ryan
"Punctuality is only a virtue for those who aren't smart enough to think of good excuses for being late" John Nichol "Point Of Impact"
Ryan Binns wrote: We've tried to separate out modules/methods to be able to test them automatically, but it was practically impossible, Yeah, I read that in another post you made after I made my comment. Sounds like an awful situation! To answer your original question, I guess that yes, I agree with you, given your situation, it seems a bit excessive to enforce unit testing especially when you can prove that the binary is identical and your diff's show only comment changes, not code changes. Someone is being a bit too obsessive! Marc MyXaml Advanced Unit Testing YAPO
-
Matt Gerrans wrote: You have to run four whole unit tests?! You poor thing. I've run these four unit tests before. Last time, it took two days, and everything passed. It just seems a bit pointless having to run them again, knowing they're going to pass because the only difference in the code between this time and last time is a bit of indentation and a couple of spelling errors in comments. If I had actually changed any code, then I wouldn't have any problems with running them. Normally, I wouldn't mind too much. It's just these four that are painful.
Ryan
"Punctuality is only a virtue for those who aren't smart enough to think of good excuses for being late" John Nichol "Point Of Impact"
FACTS: - you know they're going to pass - last time it took 2 days SOLUTION: - muck around with something else for 2 days, then back to the test just check the relevant checkboxes ;)
-
Ryan Binns wrote: I've run these four unit tests before. Last time, it took two days, and everything passed A unit test that takes two days is not a unit test. What you're running is an Acceptance Test. Your lead should know the difference. A unit test should take milliseconds to run, and each unit test tests one and only one very specific function of one specific method in one class. Now, you may have tens of thousands of unit tests eventually, but that's a different matter. Ryan Binns wrote: the only difference in the code between this time and last time is a bit of indentation and a couple of spelling errors in comments. I remember making a change to a comment, or thinking that's what I did, and something actually broke in the code. While it was entirely my fault, I learned that I cannot trust myself when I say "I only changed the comments". :) Marc MyXaml Advanced Unit Testing YAPO
exactly. I've done this a few times where I thought I just changed a comment or made a simple change and BAM a unit test tells me something broke. That's the beauty of unit tests and why like you say they need to be short, quick and only test a specific part of the code.
-
Here we use manual unit testing - no fancy automated tools, and it's too late in the project to change anything now. For some reason unknown to me, it has been decided that every time we check in a file, it has to be code reviewed and unit tested. The frustrating thing is I've just completed some cosmetic changes (spelling errors in comments, incorrect indentation etc...) resulting from a previous code review, which I now have to run 4 unit tests and a code review for. Frustrating since I didn't actually touch any of the code - it was all formatting changes. And yes, the previous code review has already been signed off by my tech lead verifying that my changes are correct. Please somebody tell me this is not a ridiculous situation...
Ryan
"Punctuality is only a virtue for those who aren't smart enough to think of good excuses for being late" John Nichol "Point Of Impact"
We don't do any testing to speak of (unit, or otherwise), and we release every 8 weeks or so (wth minor patches between releases). Scary part - program contains over 450,000 lines of code in two executables and approximately 20 DLLs, and sometimes, large swaths of this code are changed between releases. For example... Right now, I'm completely changing the command handling structure - building the menu and toolbar based on the contents of a database, and changing the command ID of EVERY menu click or toolbar button in the program. This change will affect every binary we distribute and was necessary to implement the design. Basically, I'm using the database an a sort of resource template. :) So far, I've elminated almost 1000 lines of code with the new design, and relieved us of almost all the reliance we have of the XTToolkit (an ancient 5-year old version, no less). Fewer lines of code means easier maintenance later on. BTW, I plan on posting an article describing how I dynamically built the menu and toolbar. Granted, it's done in VC6, but it shouldn't be too hard to move it to .NOT. ------- sig starts "I've heard some drivers saying, 'We're going too fast here...'. If you're not here to race, go the hell home - don't come here and grumble about going too fast. Why don't you tie a kerosene rag around your ankles so the ants won't climb up and eat your candy ass..." - Dale Earnhardt "...the staggering layers of obscenity in your statement make it a work of art on so many levels." - Jason Jystad, 10/26/2001
-
We don't do any testing to speak of (unit, or otherwise), and we release every 8 weeks or so (wth minor patches between releases). Scary part - program contains over 450,000 lines of code in two executables and approximately 20 DLLs, and sometimes, large swaths of this code are changed between releases. For example... Right now, I'm completely changing the command handling structure - building the menu and toolbar based on the contents of a database, and changing the command ID of EVERY menu click or toolbar button in the program. This change will affect every binary we distribute and was necessary to implement the design. Basically, I'm using the database an a sort of resource template. :) So far, I've elminated almost 1000 lines of code with the new design, and relieved us of almost all the reliance we have of the XTToolkit (an ancient 5-year old version, no less). Fewer lines of code means easier maintenance later on. BTW, I plan on posting an article describing how I dynamically built the menu and toolbar. Granted, it's done in VC6, but it shouldn't be too hard to move it to .NOT. ------- sig starts "I've heard some drivers saying, 'We're going too fast here...'. If you're not here to race, go the hell home - don't come here and grumble about going too fast. Why don't you tie a kerosene rag around your ankles so the ants won't climb up and eat your candy ass..." - Dale Earnhardt "...the staggering layers of obscenity in your statement make it a work of art on so many levels." - Jason Jystad, 10/26/2001
Sounds like fun :) Ours is just over a million lines of code running over a maximum of 12 machines running Solaris. Our "unit" testing consists of pushing buttons on one of 8 clients, to validate functionality of a single module on one of two redundant processing servers, by observing the output sent by one of two redundant I/O servers. Not exactly true unit testing, huh? :)
Ryan
"Punctuality is only a virtue for those who aren't smart enough to think of good excuses for being late" John Nichol "Point Of Impact"
