A Case History: Analysing Hasura Console's code review process
Stefano Magni
Posted on July 29, 2022
How a code review process can be speeded up? What are the practices we can follow to get PRs merged as soon as possible?
Introduction: I recently joined Hasura, and along with my new team we tried to identify the existing bottlenecks and why PRs stay open for such a long time. This aims to share our internal analysis that could be used to inspire other teams to do the same.
Related to the same topic, you can read my Support the Reviewers with detailed Pull Request descriptions article.
Photo by Lala Azizli on Unsplash
TL:DR
We, as PR authors, must strive to ease the reviewer’s life as much as possible.
The problems
- Reviewing a PR takes too much time.
- As a result, releasing features takes a lot of time.
- The problem is aggravated by the number of developers that recently joined the team.
Team members and review roles
| Member | Eng. seniority | Hasura seniority | Reviews code | Reviews functionalities | Time dedicated to reviews per day |
|---|---|---|---|---|---|
| N.B.S | principal | senior | ✅ | ✅ | 2 to 5 hours (😱) |
| V.P.G. | senior | senior | ✅ | ✅ | 2 to 2.5 hours |
| S.S. | senior | senior | ✅ | ✅ | 2 hours |
| M.H. | senior | middle | ✅ | ✅ | 1 hour |
| A.S.K. | junior | middle | ✅ | ✅ | from 0.5 to 1 hour |
| V.C. | junior | middle | ✅ | ✅ | 1.5 to 2 hours |
| S.M. | senior | junior | ✅ | ❌ | 30 minutes |
| E.M. | senior | junior | ✅ | ❌ | from 0 to 0.5 hours |
| N.I. | senior | junior | ✅ | ❌ | from 0 to 0.5 hours |
| L.R. | senior | junior | ✅ | ❌ | from 0 to 0.5 hours |
| D.C. | senior | junior | ✅ | ❌ | ??? |
What do we dedicate the time spent on code reviews to?
| Activity | A.S.K. | M.H. | V.P.G. | S.S. | V.C. |
|---|---|---|---|---|---|
| Understanding what the feature is | 10% | 10-20% | 10% | 10-20% | 10% |
| Reviewing the code | 40% | 40-50% | 30% | 20-40% | 30-40% |
| Setting up the various environments | 10% | 10% | 0-20% (if pro/cloud) | 10-30% (if pro/cloud) | 5-15% |
|
Testing the functionalities |
35% | 30% | 40-60% | 40-70% (if pro/cloud) | 50% (if Storybook is not used) |
If we all spend 2 hours/day, we collectively
spend 3 hours/day setting up the environment
spend 10 hours/day reviewing the code (we can improve it through shared coding patterns and explaining the design choices we made)
spend 11 hours/day testing the functionalities (the author can list what functionalities they already tested. Company-wide, they could consider creating a QA team)
What opening a PR requires
| Activity | Responsible | Current status | Future status | |
|---|---|---|---|---|
| (pre) Describe the feature/bug | The Product Owner | ⚠️ (we must improve it) | 🚧 We're working on it | |
|
(pre) Create coding patterns |
The whole team | ❌ | ❌ Not in the roadmap | |
| (pre) Groom the features | The whole team | ❌ | 🚧 We're working on it | |
| (pre) Define a testing strategy | The whole team | ❌ | 🚧 We're working on it | |
| (pre) Pair-programming the features | The whole team | ⚠️ (we must augment it) | ✅ We can start right now | |
| (pre) Reduce the scope of a feature/bug | The whole team | ⚠️ (we must improve it) | 🚧 We're working on it | |
| (pre) Add unit/integration tests | PR's authors | ⚠️ (we do not write a lot of tests) | 🚧 We're working on it | |
| (pre) Add E2E tests | PR's author | ❌ (we do not write E2E tests at all) | 🚧 We're working on it | |
| (pre) Tidy up the commits | PR's author | ⚠️ (mostly by squashing them) | ✅ We can start right now | |
| Describe the feature/bug | PR's author | ⚠️ (some of the PRs have short descriptions) | ✅ We can improve right now | |
| Describe how to test/reproduce it | PR's author | ⚠️ (sometimes it is not described at all) | ✅ We can improve right now | |
| Describe the edge cases | PR's author | ⚠️ (some of the PRs have short descriptions) | ✅ We can improve right now | |
| Describe the chosen solutions and motivations | PR's author | ⚠️ (some of the PRs have short descriptions) | ✅ We can improve right now | |
| Describe the discarded solutions and motivations | PR's author | ❌ | ✅ We can improve right now | |
| Describe the codebase limitations | PR's author | ❌ | ✅ We can improve right now | |
| Add screenshots/videos | PR's author | ⚠️ (not always done) | ✅ We can improve right now | |
| Add screenshots of the original graphic layouts | PR’s author | ❌ | ✅ We can improve right now | |
| Correctly fill up the PR template | PR's author | ⚠️ (we must improve it) | ✅ We can improve right now | |
| Comment out when starting the review | PR's reviewer | ✅ | ✅ We can improve right now | |
| Review the code | PR's reviewer | ✅ | ✅ We can improve right now | |
| Speeding setting up the Console in all modes | The whole team | ❌ | 🚧 We're working on it | |
| Review the graphic layouts alignment | PR's reviewer | ⚠️ (not always done) | ✅ We can improve right now | |
| Review the functionalities | PR's reviewer | ⚠️ (sometimes require double check, sometimes not done properly on all the versions of the Console) | ✅ We can improve right now | |
| Describe what has been tested in the review | PR's reviewer | ❌ | ✅ We can improve right now |
What we should do
- Anticipate and share the problems as soon as possible (through the grooming sessions)
- Share what we will do, what we are doing, and what we did with other team members
- Move part of the time dedicated to the code review to organize and describe the PR, by asking ourselves
- Have I described the feature enough? Is there anything I could do to get it more understandable?
- Are my intentions clear enough?
- Does my code reflect the intentions?
- Have I tested the feature the best possible way? Have I described what I tested?
- Spread the knowledge of the product and its edge cases to the new team members
In one word: tell!
Posted on July 29, 2022
Join Our Newsletter. No Spam, Only the good stuff.
Sign up to receive the latest update from our blog.
Related
August 31, 2023