Skip to content

Instantly share code, notes, and snippets.

@sleepyfox

sleepyfox/2020-10-18-preview-driven-development.md

Last active February 15, 2022 19:37
Show Gist options
  • Star 0 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save sleepyfox/53ce08ab81515cdc5494f1358f609401 to your computer and use it in GitHub Desktop.
Save sleepyfox/53ce08ab81515cdc5494f1358f609401 to your computer and use it in GitHub Desktop.
Download ZIP
Preview-driven development
Raw
2020-10-18-preview-driven-development.md 
author: @sleepyfox
title: Preview-Driven Development
date: 18 October 2020
preamble: Doing the PR review before the code is written is more helpful, just like TDD

Preview-Driven Development1

Reader's note: these notes were originally written to support a SimplyBusiness tech talk, so is written with some embedded context which I hope to explain or remove in a future edit - my apologies for any confusion that this may cause the reader.

Premise

Github Pull Request (PR) reviews would play more nicely with e.g. 1-day branches if they had a code preview to guide them.

Hypothesis: Allowing the review process to start before coding starts rather than waiting till after coding is finished will improve team effectiveness. N.B. The review still needs to be finished before the work is 'done'. Perhaps, q.v. FutureLearn example. I'd rather not conflate the two things here though.

Story

I finish my work, the tests run locally, I make my last commit and push to origin, create a new PR and ask for reviews. It's coming up to lunch time so I decide to break early for lunch to give people time to pick up the PR.

I return from lunch, vaguely satisfied by my avocado, spout and cashew sandwich. No reviews as yet. I think about what I should do in the meanwhile, I read some emails, catch up on Slack, read a linked tech blog. Still no review comments or better yet an approval. I look at the backlog - ideally there'd be something small and quickie to finish up, some dependency updates, or a change of base image or something, updating documentation perhaps. No luck, so you start on a new story, digging into the new code, new functionality.

Tomorrow morning you log on to find a stream of github emails, your PR has been reviewed! Great, work your way through the comments, fix this, resolve that, no change required there, no - you got that wrong, now what the hell did that comment mean? You ask for clarification on a couple of items. git commit, push, wait for Chopin to build, but there's a build queue on Jenkins, so reluctantly you put down the PR and go back to the new story.

You work through the afternoon and another review comes in, raising new issues. The reviewer suggests that a change you've made to the auth subroutines needs to be signed off by Infosec, you write a slack message on their channel asking if there's an issue that needs dealing with here or whether you can proceed. You head back to the PR and see the dreaded innocent comment at the bottom: "did you consider X when starting work on this PR? It seems like there's a lot of crossover with the gem that another team produced last month to solve exactly this kind of issue".

You could blow them off with a "Yeah, our team considered it but we felt that our case was sufficiently different to warrant a fresh approach", but you'd know that it would be a brush off, and really you should make sure that indeed that your case is different enough that you can justify the comment. You follow the link and look at the code, and with a sinking heart you realise that indeed their work does indeed solve a significant amount of the problem you were dealing with. Of course, it takes a different approach to the one that you took, and it isn't a straightforward swap, there's a fair chunk of work to retrofit their gem to your code.

Looks like the other story's going to have to wait a while.

Meanwhile no reply from infosec.

You spend the next couple of days retrofitting the gem, and resubmit your PR. Code reviews have to start over as there's basically almost nothing left from your original approach. You're late, and the rest of the team has already started on a major piece of work and could do with your help. Meanwhile another team is waiting on your fix.

You incorporate another round of PR comments, change, rebut, resolve. You have an approval. Cheer! But before you can hit 'squash and merge' there's that outstanding sign-off from infosec. You ping them on slack. The person handling your review is off sick, no problem, someone else will step in to help out, you're lucky they're not overrun right now like usual. Maybe it'll even be before end of day.

You go back to your other story - what was that about? Where had you got to? What does this code you wrote even do? You spend the rest of the afternoon getting your head back into the space that is was in at the beginning of the week.

Next morning, sign-off from InfoSec, hoorah! No changes required, even better! Must sacrifice a chicken to the PR gods, or like maybe like a courgette or something, you're a veggie after all. Time to merge! OH NOES! Merge conflict. You head to the kitchen to get a coffee, this could be bad. You've got the other team on your back because they're still waiting for your fix that you said would be done last week, someone's got a flakey test in the build queue in front of you and the queue's blocked, and suddenly it's looking like it's going to be a late night...

Pull request review issues

PR reviews suffer from a number of issues at the moment:

  • A PR review (especially when there is design-misalignment) blocks deployment, for anything from half-an-hour to days. This causes a number of knock-on issues:
    • Context switching costs (both for new work and for original PR task)
    • Merge-skew in deployment2
  • You cannot comment on code that has not been changed in a PR, meaning it cannot be used as an effective tool for code review, because you're not looking at a complete picture. Seeing code outside changed lines is time-consuming and actively discouraged by the tool (greyed-out text, needing to continually click 'expand' for each 20 lines).
  • Although PR reviews seek to be a 'harness the collective intelligence' type of tool, in reality the collective is not engaged, as each reviewer is (for the most part) independent, and scope for discussion is limited due to the low-effectiveness3 of text communication.
  • Few bugs are caught by PR review, especially when compared to code inspection
  • Even fewer "showstoppers" are caught by PR review
  • PR review is mostly 'bikeshedding' and small incremental improvement over the likes of naming things or code style.
  • The focus on 1-day branches and on Velocity stats means that PRs are getting smaller and smaller, the inevitable consequence of this is that logical changes that should be considered together are split over many individual PRs, and the systemic change is not and can not be reviewed or discussed ('obfuscated change', 'death by 1000-cuts' or 'cannot see the wood for the trees' anti-patterns).
  • Smaller PRs exacerbates problems with application deployments because they take so long, meaning that we are actively blocked from implementing solutions like BORS4 that implement a merge queue as a solution to force deterministic merges.

Explanation

For any two process where B follows A, in which both:

  • the result of B can involve rework in A (and corresponding downstream rework in B), and;
  • the probability of that is above some low threshold

it will be more efficient to invert the order of A and B5, so we do B first. This is how we have:

  • TDD - write the test before the implementation;
  • Walking skeleton - implement CI before we have components that need integrating;
  • BDD - implement acceptance tests before we have an application.

Experiment

To fix this, we propose a simple experiment: Code Preview

Open a DRAFT PR once work on a task begins;

Start with a description of:

  1. The scope, both the current state and the goal of the change;
  2. The constraint;
  3. How the change will be made i.e. the design;
  4. Any other factors for consideration;
  5. A link to the Trello card for the work.

If the PR would get merged (probably not), put a date and person to talk to about it.

Select reviewers;

Start work.

Review then proceeds contemporaneously with the work, just like TDD or BDD.

We expect that this will not be something that 'just works' without learning some discipline and ways of working that are not immediately obvious, hence the value of trying this as an experiment and figuring out what we need to do differently in order to realise the benefits. Use your retrospective here.

Footnotes

  1. It would be nice to give this a better name at some point, perhaps RDRR? But that will have to wait. ↩

  2. Merge-skew definition https://bors.tech/essay/2017/02/02/pitch/ ↩

  3. Effectiveness of communication modes, Scott Ambler and Alistair Cockburn, http://agilemodeling.com/essays/communication.htm ↩

  4. Bors, https://bors.tech/ ↩

  5. Reference to J.B. Rainsberger's article on queuing theory and TDD: https://blog.jbrains.ca/permalink/how-test-driven-development-works-and-more ↩

@scips
Copy link

scips commented Feb 15, 2022

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment