DEV Community

Discussion on: Good Code Reviews

Collapse
collinbarrett profile image
Collin M. Barrett • Edited on

Great piece, Mike. We've had a rocky start to implementing C.R.s in the last six months with our team of 3. One is a long-time veteran of the application and pretty resistant to change/transparency. But, it's healthy.

We do not currently do testing as a part of code reviews, though. We have no unit testing yet (which would help), but testing would take significant more time. We have been relying on the original developer and the QA team to catch bugs via testing... How do you justify the time for the extra layer of testing during the code review process?

Collapse
anibalvelarde profile image
Anibal Velarde

In my team, we see the creation of good unit tests as part of our culture. At first it was hard to get on that discipline harness. Now, in our most complex library we have over 800 UT that run within 20 seconds (93% code coverage). The feedback on any change is great. It took some time but it is worth the work. Check out Michael Feathers' book "Working Effectively with Legacy Code"

Collapse
iamshercoder profile image
Pardeep Singh

Great to hear more people writing unit tests. :)

In my team, we have a git-hook set for pre-commit where we run lint and unit tests. If either of those fail or the coverage threshold was not met, then the dev will have to fix the issues before trying again to commit. We also use a tool called jest-coverage-ratchet, which updates our coverage threshold after a successful commit.

Our team and organization goal at Build.com has been to reach 80% branch coverage. I have personally set a goal for myself as well, or more like a rule that I will write enough unit tests to meet the 80% branch coverage for any file I touch when working on a particular feature/bug. For non-complex files, I usually try to meet 100%.

Collapse
mporam profile image
Mike Oram Author

Hi Collin, tthis all depends on your development process. We dont have a dedicated QA team so rely on the developers and our account team for QA. We find that developers who are familiar with the code anticipate bugs better and catch them earlier. This means that by the time the account team see any work it is generally bug free and so saves them time. I think this can be the case with a QA team also, you want to foster an environment where your developers strive to not produce any bugs at QA stage, the team should work together to ensure the work is bug free before going to QA. C.R should be seen as a way for your developers to help improve each other and the system. That is, after all, their job.

It is not uncommon to get push back from senior developers on this, but if the rest of your team are behind it and drive it. It becomes part of the process and they won't have a choice. We integrate it into our definition of done, so no piece of work goes outside the dev team until it is fully reviewed, tested and unit tested.