3 ms·

"every bug fix must be accompanied with a test that demonstrates the bug/fix" is just dogma IMNSHO. (also see "closing the barn doors after the horses have esc

by YZF 7y ago

"every bug fix must be accompanied with a test that demonstrates the bug/fix" is just dogma IMNSHO. (also see "closing the barn doors after the horses have escaped" or "lightning doesn't strike twice at the same place") The typical rationale is the tests will now catch a re-introduction of the bug or regression. The reality is that often the tests bloat the code base, they cost in future maintenance making the code harder to refactor, they may introduce their own flakiness (the test itself might have bugs), and the bug that's fixed is generally unlikely to recur. You want good tests- not just a random collection of tests that reflect the history of bugs in the code. I often end up writing a test as part of trying to understand or reproduce the bug, and that test will often stay... But not always. Sometimes it's the right thing to do. Sometimes it's not. I.e. subjective. The lines aren't always so bright. To complicate things further it's very hard to gauge the effectiveness of various processes. Is following those specific guidelines a net positive or negative? who knows. I think there's consensus that code reviews in general are a good idea so probably just by having a second person read the code, have a discussion with the author, with no guidelines, you're getting most of the benefit (obvious issues surface, more than one person is familiar with the code etc.). My gut feel is that the rest is in the noise. I do agree hiring is critical ;) yet another subjective process there. Ideally you'd only hire people with a track record of producing good software in your area of business, most other indicators are probably pretty random. If you're looking to build a first person shooter you should hire John Carmack. Hope he likes your code review guidelines though. Another random side comment is that IMO design issues should probably be identified before code is written. EDIT: random semi-related trivia: https://kotaku.com/the-exceptional-beauty-of-doom-3s-source-code-5975610 https://kotaku.com/the-exceptional-beauty-of-doom-3s-source-...

bt848 7y ago
https://mobile.twitter.com/dvyukov/status/1169544167871131653 https://mobile.twitter.com/dvyukov/status/116954416787113165... This tweet perfectly captures why all bug fixes must have tests. Linux has no tests and no testing culture and it is 17 million lines of juicy hot garbage. Google is mostly tests and it is one of the largest and most successful C++ projects in history of our industry. I feel justified in standing my ground when reviewing under-tested code.
- YZF 7y ago
  > Google is mostly tests and it is one of the largest and most successful C++ projects in history of our industry. I'm not quite sure what you mean here? Are you talking about Search? Ads? Cloud? Self driving cars? Google has various products of varying business and technical success levels. Some stuff I feel is a good example of quality software (let's say Go for example or maybe Maps) and some stuff seems to just be very poor software (let's say Google Hangouts Chat or Google Sheets). Some stuff is really good business and some gets cancelled (with little apparent correlation to quality). Google is so rich that perhaps it is succeeding despite some practices and not because of them. It's initial success probably predates all these practices and there's plenty of quality software in the history of the industry that has been produced using a variety of other practices. Even if you are right and Google is the most successful C++ code base ever that's still not evidence that some particular practice is the cause of that. There is no way to measure any of this and "feeling" doesn't really cut it. Some practices apply more to certain kinds of software and possibly less to others. Doesn't Google use Linux? For juicy hot garbage it's done pretty well. With a budget that's a fraction of what Google has. It's also not C++. re-EDIT: Another thing to note is that Google has a large number of engineers developing internal tools, build systems, test engineers etc. With all the automation around dealing with flaky tests and testing in general at scale perhaps this works better for Google. Other companies who do not have the luxury of having 100's of engineers work on their build or test systems may run into different problems when they try to do things the Google way. Because Google is overall fairly opaque (the open bits are exceptions) it's hard to really come to a conclusion about how well they do software. I've heard different stories from different sources and I'll bet there's lots of internal variation as well. The success as a company is undeniable but there's more to that than software.
enneff 7y ago
If the added test makes your code hard to refactor, then the test is too low-level. Trigger the bug by exercising the higher level abstractions, then refactoring won't care about the test (and in fact, you'll be glad the test is there when you refactor). WRT "brittle", sometimes a test fails when you're doing maintenance; you can look at the test and sometimes it makes sense to delete it, sometimes it shows you that your maintenance has unintended consequences. You can't make that judgment call if you didn't write/keep the test.
cr1222 7y ago
Including a regression test is an excellent heuristic. I also like to extend that to testing closely related things where there are test gaps. The benefit is not just that it will catch the exact same bug recurring, but that you get more tests in areas of the code with higher bug density and more test gaps. It's not a perfect heuristic but it's an easy one to get a team aligned on. IMO the biggest challenge in scaling a team and maintaining code health is setting standards like this that are easy to explain and easy to maintain. There's an unfortunate human tendency to cut corners in the absence of clear rules here (even the best engineers will do this in many circumstances). You can always exercise judgement on the cost/benefit of adding the test though, it's just a good default.

by YZF 7y ago

bt848 7y ago

https://mobile.twitter.com/dvyukov/status/1169544167871131653 https://mobile.twitter.com/dvyukov/status/116954416787113165... This tweet perfectly captures why all bug fixes must have tests. Linux has no tests and no testing culture and it is 17 million lines of juicy hot garbage. Google is mostly tests and it is one of the largest and most successful C++ projects in history of our industry. I feel justified in standing my ground when reviewing under-tested code.

YZF 7y ago
> Google is mostly tests and it is one of the largest and most successful C++ projects in history of our industry. I'm not quite sure what you mean here? Are you talking about Search? Ads? Cloud? Self driving cars? Google has various products of varying business and technical success levels. Some stuff I feel is a good example of quality software (let's say Go for example or maybe Maps) and some stuff seems to just be very poor software (let's say Google Hangouts Chat or Google Sheets). Some stuff is really good business and some gets cancelled (with little apparent correlation to quality). Google is so rich that perhaps it is succeeding despite some practices and not because of them. It's initial success probably predates all these practices and there's plenty of quality software in the history of the industry that has been produced using a variety of other practices. Even if you are right and Google is the most successful C++ code base ever that's still not evidence that some particular practice is the cause of that. There is no way to measure any of this and "feeling" doesn't really cut it. Some practices apply more to certain kinds of software and possibly less to others. Doesn't Google use Linux? For juicy hot garbage it's done pretty well. With a budget that's a fraction of what Google has. It's also not C++. re-EDIT: Another thing to note is that Google has a large number of engineers developing internal tools, build systems, test engineers etc. With all the automation around dealing with flaky tests and testing in general at scale perhaps this works better for Google. Other companies who do not have the luxury of having 100's of engineers work on their build or test systems may run into different problems when they try to do things the Google way. Because Google is overall fairly opaque (the open bits are exceptions) it's hard to really come to a conclusion about how well they do software. I've heard different stories from different sources and I'll bet there's lots of internal variation as well. The success as a company is undeniable but there's more to that than software.

enneff 7y ago

If the added test makes your code hard to refactor, then the test is too low-level. Trigger the bug by exercising the higher level abstractions, then refactoring won't care about the test (and in fact, you'll be glad the test is there when you refactor). WRT "brittle", sometimes a test fails when you're doing maintenance; you can look at the test and sometimes it makes sense to delete it, sometimes it shows you that your maintenance has unintended consequences. You can't make that judgment call if you didn't write/keep the test.

cr1222 7y ago

Including a regression test is an excellent heuristic. I also like to extend that to testing closely related things where there are test gaps. The benefit is not just that it will catch the exact same bug recurring, but that you get more tests in areas of the code with higher bug density and more test gaps. It's not a perfect heuristic but it's an easy one to get a team aligned on. IMO the biggest challenge in scaling a team and maintaining code health is setting standards like this that are easy to explain and easy to maintain. There's an unfortunate human tendency to cut corners in the absence of clear rules here (even the best engineers will do this in many circumstances). You can always exercise judgement on the cost/benefit of adding the test though, it's just a good default.