Well; code review is not QA. My approval means - I'm OK with how the code layer is knitted. It doesn't mean I've tested the changes.
Of course, if there's some logical problem with the implementation (such as the author seemingly failing to handle an edge case), any careful reviewer should catch this out.
Still, this is a situation where the overlook could be detected through theoretical code analysis, so to speak: which is what a review is. That's not testing.
"some 2~3 line changes may not need to be tested, but those are the exception, not the rule."
Generally speaking everything needs to be tested (not necessarily manually), but that's beyond the point. Testing is not reviewing, and vice versa.
On my current team, we want most tests to be automatic. But the manual regression testing that might be needed is mostly done by other developers. A separate test-environment is created for each PR, and the link posted in the pull-request. So when looking at a pull-quest we expect people to do both a code-review and QA.
In my team approving a PR means you approve for the code to go to production. So you need to consider both code quality and whatever QA is needed. But thats just our way t do it - other teams do things differently. And I think thats OK, there is no silver bullet.
Changes from this type of review are usually fine details (“use a constant for this”, “consider this edge case”, “prefer this style…”) that can be done easily by the reviewer with no context.
If you’re lucky, you might get meaningful feedback (“consider using this approach instead…”), but many people just use code reviews as a gateway to merging code (1).
This is the pragmatic approach; just trust the other developers and doing their jobs and do a light pass check to ensure that everyone is aligned on approach and style.
..but, that’s not what you were tasked with.
You were asked to review the code, not the syntax.
You cannot review code you do not understand.
I’m sorry, but your mental parser does not run code. It does not render ui, update databases or generate concurrent race conditions. You may be able to approximate some of those, but most people can only do all of those things by actually running the code.
So… you may get some value from looking at code; but I question, in almost all cases, if teams contributed significant value by doing so.
[1] - see https://blog.jetbrains.com/upsource/2017/01/18/code-review-a..., etc. google “code review as gateway”.
I'm 100% on-board with this. However, I understand were OP's mindset comes from. A few years ago I was working at a development shop in a fairly large project, where we had little to no automated tests. We frequently had PRs that would break main features. At some point developers were required to perform smoke tests alongside code reviews.
> Well; code review is not QA. My approval means - I'm OK
> with how the code layer is knitted. It doesn't mean
> I've tested the changes.
I know that more experienced people are able to notice bugs or edge cases due to having a lot of experience and understanding of prior art, but noticing incorrectly designed/implemented logic/behaviour is much easier if your review includes execution of their code.
If you leave this to some later QA stage or QA team then you're just making your reviews less effective and increasing the size of the feedback loop.
If you wait until code is merged into `master` before doing further testing, you're saving yourself 15 minutes of time, but when a problem is found everybody will be disconnected from the source of the bug.
If you, as a software engineer, pull down the branch and try running the code then you'll be able to find bugs, edge-cases, misunderstandings of the design (etc) and then literally point out where they occurred in the source code to your colleagues at the point of the review.
It saves a bunch of time to do it this way, because it only takes <15 minutes to pull down a branch and execute the code, and finding problems at this point reduces everybody's feedback loop and allows you to give meaningful feedback in relation to the diff of the source code. (Note: I'd not do this with trivial changes.)
(I think the only reason that people rest on "role separation" here is because they've never actually tried doing anything deeper so can't comprehend that it helps to find deeper problems.)
When devs would only write code and never click anything around in the system how can they implement anything good?
I don't believe in "efficient role separation" that is 100%. Devs still need to click the system around and still need to attend meetings that give business context. The same for QA, they should understand basic exceptions and know where to find logs to make good reports and not just make screenshot and say "not passed" and drop all troubleshooting on the devs or ops.
Best outcomes I see are when you have multidisciplinary team where team members work together. Of course you have Dev or QA specialists but you have to have people who don't throw stuff over the wall and are focusing on delivering togheter.
1. The submitter approves of the code being merged into master 2. The submitted has tested the happy path and everything seems to work
If you want to enforce standards, automate it (there are multiple highly configurable tools for that). If you want good solutions, pair program. If you want to maintain good codebase, review it periodically (codebase, not some changes to it contained in pull request) and refactor. Use TDD / BDD to ensure the app still works.
This implies the need for smaller pull requests maybe?
If work is broken into smaller chunks, addressing peer feedback isn't as costly, because there's time to back out of poor decisions if the peers get alerted to them early on.
Changing the course once the implementer has indeed invested lots of time and effort into a flawed solution is ineffective, true; but that might just mean the feedback was asked for too late.
"If you want to enforce standards, automate it"
Agreed, as long as you're thinking standards as in indentation, naming rules etc. All the simple stuff can be done with code inspections, and employing humans to enforce these "mechanical" standards is a waste of time. No argument there!
However, there are higher-level standards that can't really be automated though, or at least not effectively. Eg. various architectural conventions and the like.
"If you want to maintain good codebase, review it periodically (codebase, not some changes to it contained in pull request) and refactor"
Well, I can see how it could work in a small, lean project... but in some multi module behemoth with millions of LOC and history spanning back years?
"The code was already written. Time/money spent" rings much more strongly in that scenario that in reference to "regular" code review.
This makes me feel like perhaps pull requests shouldn't always pull/merge the changes directly, but instead there should be something like the GitLab "Start a review" functionality, where you can queue up code comments, before submitting all of them at once, but for pull requests.
As a Jira analogy, imagine the totality of the changes that you want to merge as an issue, but the individual steps/smaller requests as sub-tasks. The entire task would only be considered done after all of the sub-tasks are done, similarly, the changes should only be merged after all of the sub-requests have been successfully reviewed. Thus, development could happen more iteratively, with feedback sooner.
Otherwise you end up with something like the following being separate requests:
- FEATURE-123 add data models for the feature
- FEATURE-123 add service logic for the feature
- FEATURE-123 add logging and validations for the feature
- FEATURE-123 add unit tests for the feature
- FEATURE-123 add REST endpoints for the feature
- FEATURE-123 add front end code to forms
- FEATURE-123 add front end logic to use the REST endpoints
- FEATURE-123 add integration tests for headless browser
To me, all of this seems like a problem that stems from developers viewing code review as something to only be done at the moment when you want to merge the feature to your main branch. Even the tooling that we use is built with this in mind. It feels like there are better alternatives.
EDIT: So, you'd need something like the following:
- (WIP, 3/8 reviewed) FEATURE-123 add new feature // feature-123 branch, cannot be merged into main yet, review the below first
- (reviewed) add data models for the feature // feature-123-data-models branch, will be merged into previous
- (reviewed) add service logic for the feature // feature-123-service-logic branch, will be merged into previous
- (reviewed) add logging and validations for the feature // feature-123-logging-and-validations branch, will be merged into previous
- (in review, 9/13 comments resolved) add unit tests // feature-123-unit-tests branch, will be merged into previous
- (registered) add REST endpoints for the feature // feature-123-rest-endpoints branch, will be merged into previous
- (registered) add front end code to forms // feature-123-front-end-code branch, will be merged into previous
- (registered) add front end logic to use the REST endpoints // feature-123-front-end-rest branch, will be merged into previous
- (registered) add integration tests for headless browser // feature-123-integration-tests branch, will be merged into previous
You can technically do the above already, by having a main feature branch and functionality specific feature branches all of which get merged into the main feature branch, before then being merged into the main branch after the feature is complete. It's just that people usually live with the view of: "One merge request == one feature", which i don't really think should be the case.
I see no problem with that if it is a small change that can de deployed safely. (for instance hidden behind a feature-toggle where applicable).
We tend to strive for small, short lived branches. We usually don't want PRs with more than a couple of days work - to keep them small, easy to test. It also makes the amount of changes going to production at any time small.
The best would be if any specific feature is small enough to only be a few days work. But out experience is that it is not allways easy to do that - and sometimes some features will take weeks before they are done.
Yes, alone they don't provide actual value, but they are small (easy to review) and can be trivially / automatically rolled back (database migrations aside) and can be deployed (without being used) safely, and even when they are exercised, it may be via a switch, itself toggled with a pull request which itself can be rolled back in isolation.
Whatever you think is a small change, think even smaller. Think creating a class and stubbing out many of the methods with TODOs.
Delaying the merge until all commits required for the feature wouldn't work in a large monorepo - just keeping small commits up to date can be work when there's enough people working around the clock. There's no feasible way to sustain continuously rebasing in a feature branch model in a large monorepo.
True - and it sounds like a textbook scenario for introducing a feature branch... this approach has worked well for me.
The idea of queueing up "sub-requests" is interesting, but it comes at the cost of creating another layer of complexity (in my view).
Code reviews are in my opinion still a good tool. It works well in other industries as well. You want to bounce stuff from the other people before pushing it further.
Where it goes wrong is people - so it is people problem not the tool problem. If you do not have good people you won't fix them with code reviews.
It is the same with agile, managers tend to think they can just follow some agile coach and can hire anyone to the team. But the truth is, I see bad teams going bad with or without agile and good teams agile or not going strong.
I don't agree 100%, but I am on-board with the sentiment. I've seen systemic problems of under-communication pre-review which results in me looking at a PR and seeing a fundamental issue, but not knowing how to approach tackling it without hurting feelings. I don't think eliminating reviews is the solution, but I think it's much less of a critical step than it's often made out to be. My take is that if reviews are actually preventing major issues with released code, they're almost certainly including work that should've happened much earlier in the development process.
If the management rebukes, you move employer.
Quality is not something the customer or your manager is responsible for. It's entirely on us as professional developers.
That said - code review is not the only way to ensure quality. Do what works. Pairing generally works even better for getting quality review time in.
I'm building CodeApprove (https://codeapprove.com) to be a much more powerful, enjoyable, and efficient code review tools for teams on GitHub. If you're interested in trying our Alpha email me.
Doing the reviews myself:
A lot of simple code splintered in 100 files -- I and all others that I know only review the diffs.
if(THIS_COERNER_CASE){do this ; do that}; xml configs relating to this or that business case
There's nothing to review, really, I don't know the business case, the code looks okish.
I really don't care about unused imports (unless you make a special commit called "code cleanup" that pollutes history and I hate you for that), lines too long, if you used nested ifs or returns, unless it's really nasty code (which I very rarely stumbled upon, usually written by some coding style nazzi in his youth);
Feedback for my own/others code:
- this or that "coding style" issues by some coding style nazzi; almost fine, it improved my inner compiler/code formatter
- "don't use this language feature, because I don't like it" from the highly oppinionated nutjob that leaves the company in a couple of months anyway to join a smaller company where his genius is appreciated more
- "why did you do this and that, you should have done...." half an hour later in person/over the phone conversation... "oh, yeah, I guess that actually makes sense"
- "whaaaaaat, you expect me review this monster commit? Why didn't you break it in multiple smaller commits?" -- because I care about the convenience of the person that is actually interested in investigating a feature/bug 5 years from now and not of the lazy code reviewer now
- "How could this security bug have slipped us, we have CODE REVIWS, don't we?"
Doesn't seem like such a big problem to make a PR with multiple commits, and then squash and merge into 1 commit after the review.
Like "migrated this JMS queue to kafka", that can span across tens of files.
Just because the commit is big doesn't mean it's not also logical and consistent.
In fact the reverse can be said -- if you split the commit in smaller commits (and your build is still green with each commit) -- each commit can seem contrieved, and good luck later on, when you try to understand how a piece of code works and how this xml/drl/properties file is connected to this piece of code if you have multiple commits.
If it's a single commit, you can usually narrow down your search tremendously and can easily see the logical deoendencies.
So for me, if it's a JIRA issue, I usually squash my commits before I push.
I fundamentally disagree here. If you're reviewing code, the code that is pushed should be what is reviewed, not a draft of what is reviewed..
- "you are right, i completely forgot about that. thank you for catching this, i am changing the code and asking you to review it again after the fix."