{"id":977,"date":"2010-02-02T00:55:02","date_gmt":"2010-02-02T05:55:02","guid":{"rendered":"http:\/\/mikeconley.ca\/blog\/?p=977"},"modified":"2023-12-20T16:25:18","modified_gmt":"2023-12-20T21:25:18","slug":"pre-commit-code-review-in-markus-development","status":"publish","type":"post","link":"https:\/\/mikeconley.ca\/blog\/2010\/02\/02\/pre-commit-code-review-in-markus-development\/","title":{"rendered":"Pre-Commit Code Review in MarkUs Development"},"content":{"rendered":"<p>So, for my Master&#8217;s thesis, I&#8217;ve pretty much set my heart on code review as my research area.\u00a0 Here&#8217;s why:\u00a0 <em>it works<\/em>.\u00a0 It makes your code better.\u00a0 It helps you find bugs.\u00a0 And I&#8217;m not just quoting something overheard in a pub &#8211; <a href=\"http:\/\/mikeconley.ca\/blog\/2009\/09\/14\/smart-bear-cisco-and-the-largest-study-on-code-review-ever\/\">there&#8217;s evidence to back these claims up<\/a>.<\/p>\n<p>And then there&#8217;s my own experience to boot &#8211; <a href=\"http:\/\/www.markusproject.org\">the MarkUs team<\/a> has been using <a href=\"http:\/\/www.reviewboard.org\/\">ReviewBoard<\/a> as our pre-commit code review tool since last summer, and I wouldn&#8217;t ever go back.\u00a0 If I ever have to work in a shop that doesn&#8217;t perform code reviews, I&#8217;ll campaign my butt off.<\/p>\n<p>Having said all that, <strong>pre-commit<\/strong> reviews  certainly <a href=\"http:\/\/stackoverflow.com\/questions\/246319\/peer-review-code-before-or-after-check-in\">aren&#8217;t for everyone<\/a>.\u00a0 Some downsides of pre-commit over post-commit:<\/p>\n<ul>\n<li>It goes against &#8220;<a href=\"http:\/\/www.codinghorror.com\/blog\/archives\/001165.html\">check in  early, check in often<\/a>&#8220;<\/li>\n<li>&#8220;The major downside of pre-checkin code review is that it puts a  major  bottle neck on getting changes into the system for other  developers to  integrate with early enough.&#8221; (<a href=\"http:\/\/stackoverflow.com\/questions\/246319\/peer-review-code-before-or-after-check-in\">from this link<\/a>)<\/li>\n<li>For some applications, testing takes hours on end.\u00a0 Why wait?\u00a0  Might as well toss it into the repo, let the Continuous Integration build it, and just  see what happens.<\/li>\n<\/ul>\n<p>There are probably more.<\/p>\n<p>My response:\u00a0 at least for MarkUs, pre-commit code reviews are working just fine, thank you  very much.\u00a0 At least we&#8217;re reviewing it &#8211; and any review is better than no review.\u00a0 But to continue my response, here are a couple of advantages of pre-commit code review for the MarkUs development team:<\/p>\n<ol>\n<li>Since most students working on MarkUs are doing it for half-credits, this means there&#8217;s a lot of turnover every semester.\u00a0 ReviewBoard lowers the chance of our new hotshot developers\u00a0 accidentally slipping something ridiculous into the repository and having to do that neat Subversion trick of pulling it out again.\u00a0 This is the obvious one.<\/li>\n<li>It helps all developers keep track of what everyone else is doing.\u00a0 This is true for post-commit reviews too, but it&#8217;s certainly worth the mention.\u00a0 It sure beats reading SVN log messages&#8230;<\/li>\n<li>It&#8217;s a great arena for new developers to ask questions.\u00a0 Our new developers this semester have been very active on our ReviewBoard, asking plenty of questions about things that are showing up in the diffs under review.\u00a0 Sometimes, &#8220;theoretical&#8221; code is posted to demonstrate how something would be done.\u00a0 Post-commit does not support this nicely.<\/li>\n<li>It&#8217;s an excellent way of showing how you&#8217;re coming along with a task, without the embarrassment of breaking the build.\u00a0 MarkUs developers sometimes post up &#8220;sneak previews&#8221; just to give everybody a taste of how their particular task is coming.\u00a0 This &#8220;sneak preview&#8221; gives the opportunity for other developers to critique the direction that the submitter is going in, and offer pointers in case they seem to be heading off in a hazardous direction.<\/li>\n<\/ol>\n<p>Yep, there&#8217;s just something so satisfying about seeing all of those little green &#8220;ship-it&#8217;s&#8221;, and then firing off your code into the repository&#8230; it&#8217;s positive reinforcement for code reviews.\u00a0 And it&#8217;s strangely addictive to me.<\/p>\n<h3>Another Idea to Augment This Process<\/h3>\n<p>A little while ago, I wrote about what I consider to be one of the <a href=\"http:\/\/mikeconley.ca\/blog\/2009\/10\/08\/the-achilles-heel-of-light-weight-code-review\/\">Achilles&#8217; Heels of Peer Code Review<\/a>.\u00a0 Here&#8217;s another one: at least for ReviewBoard, during a review <em>all you&#8217;re looking at is the code<\/em>.\u00a0 That&#8217;s all fine and dandy if you want to <em>look <\/em>at the logic of the code&#8230;but what if you want to try it?\u00a0 Does trying it out help find more bugs than just looking at it?<\/p>\n<p>Well, at least for MarkUs, it&#8217;s helped.\u00a0 I&#8217;ve recently started checking out a fresh copy of MarkUs every time a review request is put up, and splat a copy of the diff under review on top.\u00a0 I run the test suites, and if they pass, I drive it around.\u00a0 I try out the new features that the diff supposedly adds, or try to recreate the bug that the diff supposedly fixes.<\/p>\n<p>And I&#8217;ve caught a few bugs this way.\u00a0 This is because ReviewBoard is good at showing me what is in the code, but is bad at telling me what is <em>not <\/em>there.\u00a0 And that&#8217;s perfectly understandable &#8211; <a href=\"http:\/\/mikeconley.ca\/blog\/2009\/10\/27\/code-reviews-and-predictive-impact-analysis\/\">it&#8217;s not psychic<\/a>.<\/p>\n<p>So here&#8217;s an idea &#8211; how about writing a little script that checks ReviewBoard for new review requests.\u00a0 When it finds one, it checks out a brand new copy of MarkUs, splats down the diff under review over top, runs the tests, and then posts back as a ReviewBoard reviewer how many tests passed, how many failed, etc. If we wanted to get fancy, the script could even do some commenting on the code &#8211; maybe using <a href=\"http:\/\/github.com\/martinjandrews\/roodi\">Roodi<\/a>, <a href=\"http:\/\/ruby.sadi.st\/Flog.html\">Flog<\/a>, <a href=\"http:\/\/ruby.sadi.st\/Flay.html\">Flay<\/a>, or some of those other <a href=\"http:\/\/ruby.sadi.st\/Ruby_Sadist.html\">sadistically named Ruby tools<\/a> to say things about the diff.\u00a0 The script would be another reviewer.<\/p>\n<p>And then the kicker &#8211; the script posts a link in its review where developers can try out a running instance of MarkUs with the applied diff.<\/p>\n<p>Want a fancy name to kick around the office?\u00a0 Call it <em>pre-commit continuous integration.<\/em> I just checked &#8211; <a href=\"http:\/\/www.google.ca\/search?q=%22pre-commit+continuous+integration%22\">it&#8217;s not a common term<\/a>, but <a href=\"http:\/\/people.canonical.com\/~ianc\/papers\/community-agile\/community-agile.html#pre-commit-continuous-integration\">I&#8217;m not the first to use it<\/a>.\u00a0 Again, so much for being cutting edge.<\/p>\n<p>Would this be useful?\u00a0 It&#8217;s possible that the Roodi\/Flog\/Flay stuff would bring too much noise to the review process &#8211; that&#8217;s something to toy with later.\u00a0 But what about the link to the running instance?\u00a0 Will that little feature help catch more bugs in MarkUs?\u00a0 How about for <a href=\"basieproject.org\">Basie<\/a>?<\/p>\n<p>I&#8217;m curious to find out.<\/p>\n<p>Unfortunately, <a href=\"http:\/\/www.mail-archive.com\/reviewboard@googlegroups.com\/msg00860.html\">ReviewBoard doesn&#8217;t let me download diffs through its API just yet&#8230;<\/a>if schoolwork lets up for a few days, <a href=\"http:\/\/github.com\/reviewboard\/reviewboard\">I&#8217;ll look into changing that.<\/a><\/p>\n<p>I&#8217;d love to hear your thoughts.<\/p>\n","protected":false},"excerpt":{"rendered":"<p>So, for my Master&#8217;s thesis, I&#8217;ve pretty much set my heart on code review as my research area.\u00a0 Here&#8217;s why:\u00a0 it works.\u00a0 It makes your code better.\u00a0 It helps you find bugs.\u00a0 And I&#8217;m not just quoting something overheard in a pub &#8211; there&#8217;s evidence to back these claims up. And then there&#8217;s my own [&hellip;]<\/p>\n","protected":false},"author":4,"featured_media":0,"comment_status":"open","ping_status":"open","sticky":false,"template":"","format":"standard","meta":{"jetpack_post_was_ever_published":false,"_jetpack_newsletter_access":"","_jetpack_dont_email_post_to_subs":false,"_jetpack_newsletter_tier_id":0,"_jetpack_memberships_contains_paywalled_content":false,"_jetpack_memberships_contains_paid_content":false,"footnotes":"","jetpack_publicize_message":"","jetpack_publicize_feature_enabled":true,"jetpack_social_post_already_shared":false,"jetpack_social_options":{"image_generator_settings":{"template":"highway","default_image_id":0,"font":"","enabled":false},"version":2}},"categories":[454],"tags":[588,589,501,1218,550,587,586,585,504],"class_list":["post-977","post","type-post","status-publish","format-standard","hentry","category-code-reviews","tag-basie","tag-basieproject","tag-code-review","tag-code-reviews","tag-markus","tag-markusproject","tag-post-commit","tag-pre-commit","tag-reviewboard"],"jetpack_publicize_connections":[],"jetpack_featured_media_url":"","jetpack_shortlink":"https:\/\/wp.me\/prmTy-fL","jetpack_sharing_enabled":true,"_links":{"self":[{"href":"https:\/\/mikeconley.ca\/blog\/wp-json\/wp\/v2\/posts\/977","targetHints":{"allow":["GET"]}}],"collection":[{"href":"https:\/\/mikeconley.ca\/blog\/wp-json\/wp\/v2\/posts"}],"about":[{"href":"https:\/\/mikeconley.ca\/blog\/wp-json\/wp\/v2\/types\/post"}],"author":[{"embeddable":true,"href":"https:\/\/mikeconley.ca\/blog\/wp-json\/wp\/v2\/users\/4"}],"replies":[{"embeddable":true,"href":"https:\/\/mikeconley.ca\/blog\/wp-json\/wp\/v2\/comments?post=977"}],"version-history":[{"count":13,"href":"https:\/\/mikeconley.ca\/blog\/wp-json\/wp\/v2\/posts\/977\/revisions"}],"predecessor-version":[{"id":3202,"href":"https:\/\/mikeconley.ca\/blog\/wp-json\/wp\/v2\/posts\/977\/revisions\/3202"}],"wp:attachment":[{"href":"https:\/\/mikeconley.ca\/blog\/wp-json\/wp\/v2\/media?parent=977"}],"wp:term":[{"taxonomy":"category","embeddable":true,"href":"https:\/\/mikeconley.ca\/blog\/wp-json\/wp\/v2\/categories?post=977"},{"taxonomy":"post_tag","embeddable":true,"href":"https:\/\/mikeconley.ca\/blog\/wp-json\/wp\/v2\/tags?post=977"}],"curies":[{"name":"wp","href":"https:\/\/api.w.org\/{rel}","templated":true}]}}