Hey all! My team at work is struggling with growing pains of getting into a formalized review process, so I was wondering if any of you guys have some things to live or die by in your code reviews. How much of it is manual, or how much is just static code analysis + style guide stuff, etc?
We’ve got 20 or so devs and some infrequent contributors commiting to a pair of mono-repos, with some extra steps between them.
Our process looks like this:
- develop on a feature branch
- get two or more reviewers, sometimes devs that you’ve been talking with about the design, but if you don’t know who we have a list of devs for different product areas.
- only our newest stuff has auto-linting, otherwise style and static code analysis is all manual, but we’re trying to automate as we go
- need at least one approval to merge, not by any got rules, just by convention
All the code reviews are asynchronous, we’re a distributed team so we don’t like sit down in a room to talk about it, just comments on the PR.
Sometimes however you find a fix so small, you just commit and push to master. I’m not really in favor of that, but it happens.
Midwestern b tier company:
-
Test your code on its own branch. Make sure it’s not fucked
-
Pr to dev, do code review with devs that call you out on your bullshit (were all lazy sometimes)
-
Whip up QA doc, send the ticket to the QA queue
-
Confirm with BU that all their bases are covered and nothing is missing
-
Repeat steps for inevitable wishy washy scope creep from BA who wants to have inevitable meetings that could be done in emails
-
Complete merge to dev, merge dev to master, and tell devops that it’s ready to go
-
this is almost exactly how my company does it as well
except merges are typically handled by specific people and they only reach out for reviews if they aren’t sure of how to handle a potential conflict
Ours is pretty intense - large bank, 60 or so iOS engineers actively contributing to a mono-repo:
- We have about 15 CI steps that pick up on anything from basic linting to security concerns (SonarQube). Unit tests, UI tests, etc.
- We have a template that PR authors follow to add descriptions, test plans, devices tested on.
- Reviewers are automatically assigned using a round robin system
- Reviewers obviously review the code, but also execute the test plan, which includes accessibility testing.
- All PRs require 2 approvals.
- A bunch of other stuff (uploading artefacts, generating gRPC protos) that probably isn’t worth going into detail.
It’s intense, and PRs on average take a week or so to get merged. In saying that, it is the highest quality and most well-architected codebase I have ever worked on.
If I were in your situation I’d push for the following:
- all PRs have one approval, preferably two depending on team size
- code is tested by someone else before being merged to main
- use linters, Danger, etc to pick up on trivial shit
- a few manual checks like ensuring code is unit tested
- a Github PR Reviewer guide describing common issues to look for, tone of messaging when leaving comments (“be nice”, “make it clear when you are adding optional nit-picks”, etc)
- encourage authors to add review comments to their own PRs for any bit of code that isn’t immediately obvious
- stretch goal: look into generating code coverage reports on your PRs, add quality gates
This is super interesting, thank you for the info! Do you guys find a week for a merge too long?
Yeah it’s probably one of our more hotly discussed issues at eng. catch-ups. A few newer starters come from more startup backgrounds and find it super frustrating.
Pros & cons as with any process 🤷
SonarQube
Triggered
We’re a small team of 6 and follow Git Flow for branching. Every change that wants to go into the develop branch needs an approval on the pull request by one of the two senior developers on the team.
We have (virtual) meetings for any changes that require discussion; some things are difficult to quickly communicate over pull request comments.
When it comes to code style, our standard is “format it using IntelliJ”. Same with warnings – if the IDE calls it out, it’s fair game in the review. Personally, I check out every branch I’m reviewing so that I can navigate easily and enable my own warnings, which are generally strict.
Most PR comments start as questions because sometimes questionable looking code can be correct, then things go from there.
At my work we use trunk based development, with mandatory review before merging (enforced by tooling). Part of the review is ensuring proper test coverage and it’s enhanced by static code analysis.
have you looked at Git Flow? Its pretty solid.
My team has a develop branch from which we branch feature branches. On it we commit our stuff and when we think its feature complete we build a snapshot version of it so that our QA can test it. Once that test was successful, and the code has been peer reviewed, it will be merged back onto develop.
PRs will be auto built so that the feature can be integrated and automated tested.
There is trunk based way. Although I have not used it heavily at work. https://trunkbaseddevelopment.com/
My team is very small (3 people). We mostly trust each other on just merging away without PR reviews. Although we ask for reviews when in doubt during development, not when ready to merge. Mostly for asking ideas on where to put stuff.
On my previous work, we were like a 15+ dev team, doing mandatory PR reviews before merging and doing the shotgun request (ping @review_channel and pray). I hated it.
So we use that, and that works well. What does your peer review process look like? Is it pretty loosy-goosy, or do you have hard and fast guidelines and checklists?
Its pretty much loosy-goosy with only a limit of how many checks are required (including a fully built build) to be able to merge.
Do you have automated builds for every snapshot that needs to be tested or is QA running them locally?
We have a pipeline (realised on Jenkins with jenkinfiles) that is triggered by changes on a Branch a PR was made for. Another pipeline then is responsible for providing the Application for QA testing.
We use a version of Git Flow for branching (since everyone is talking about branching strategies here). But technically, you asked specifically about code review process. Every ticket is it’s own branch against the
developmentbranch, and when complete is merged by PR into the development branch. We’re a small team, so our current process is:- Merges to the development branch require one approval
- Merges to the main branch for a release require two approvals
- If the changes are only code, any developer can review and approve
- If there are “significant” SQL changes a DBA approval is required.
- “significant” means a new entity in the DB, or…
- an inline/Dapper query with a join
As we grow we’ll probably have to silo more and require specific people’s approval for specific areas.
A lot of what we do is “cultural”, like encouraging readability, avoiding hard-coded values, and fixing issues near the altered code even when not related to the original ticket. The key is to be constructive. The goal is better code, not competition. So far we have the right people for that to work.
I’m a senior at a large tech company - I push all the teams I work with to automate the review process as much as possible. At minimum, teams must have a CI hook on their pull request process that runs a remote dryrun build of the changed packages. The dryrun makes sure the packages compile, pass unit tests and meet linter rules. A failed build blocks the pull request from being merged.
I try to encourage developers to turn the outcome of every code style discussion into a lint rule that fails the dryrun build when its violated. It saves time by automating something devs were doing manually anyway (reading every line of code to look for their code style pet peeves) and it also makes the dialogue healthier - devs can talk about the team standards and whether the code meets them, instead of making subjective comments about one another’s code style.
Small startup - Here is our process
- Create your branch
- Implement feature
- Test independently of other components (with unit tests or otherwise)
- Test directly with other componenets
- Work with other devs to ensure stability on dev branch, make any small bug fixes directly in dev branch
- Push to prod
I work on an old monolithic system (20 years ish). It’s a case management system. We’ve been through many iterations of our workflow over the years I’ve worked there. Our current workflow is,
- Create a branch from main
- Create a PR against main when ready to merge
- Every night a new build of the program is automatically built and pushed to a testing environment
- Every Wednesday night we deploy a new version. We deploy the version that has been fully testet. So for instance, if we built v 9.101 on Tuesday night, but that build still has untested features, we deploy v 9.100 which was built Monday night, etc.
- If there is a major bug in production, we
- temporarily deactivate merging.
- fix the issue in main.
- build a new version, push it out to a test environment
- our product owners test the fix along with other new features that have found their way into the build.
- Once testet ok, we decide if the fix is important enough to take the system down during working hours, or if it can wait for a night-time deploy.
- We open up merging again.
The obvious down side to this way of working is that the product owners might have quite a few features to test if there is a hotfix, and other features have been merged since deploy. This has almost never been an issue though. We almost always deploy the very latest version on Wednesdays, and if there is a major issue, it’s usually discovered and reported before 11 am on Thursday. So the number of other features which have found their way into the code is usually 1 or 2 at the most. Each feature is usually quite small in scope, so it’s actually worked quite well for us.
How many developers do you have that you have to disable merging? Or is it more a safety-net?
Purely a safety net to avoid mistakes.
Merging straight from feature -> master is gutsy
Well. I told a slight lie. We go against develop. However, builds are created from develop. So we don’t actually use main anymore. We used to, but our workflow changed over time untill we got to the point were we didn’t use main anymore. I’m not even sure what we would use it for if we’d tried.
Current place:
- Work is done on a feature branch on a personal fork of the repo
- Codebase requires 100% functional coverage, and you’re responsible for writing the tests for your code and including that in the same PR
- Run pre-commit hooks for style auto-formatters before you can commit and push your code to your origin fork
- Ideally run local tests as well
- Create a PR to pull feature branch into the upstream repo’s main branch, which triggers the CI pipeline for style and tests
- At least 1 other person must review the code before a PR can be approved and merged into upstream main
- There’s a separate CI pipeline for testing of publishing the packages to TestPyPI
- Releases to PyPI are currently done manually
