5 ms·

"Approving a pull request without even testing the code it’s very dangerous" Well; code review is not QA. My approval means - I'm OK with how the code layer is

by V-2 5y ago

"Approving a pull request without even testing the code it’s very dangerous" 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.

fredrb 5y ago
> 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'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.
- deleted 5y ago
  [deleted]
kashug 5y ago
I agree that code review is not QA. But I read it as the writer of the post comes from a team where they do QA on pull-requests. And QA is assumed to be done on a per PR basis. But here I think different teams will have different approaches and pipelines. 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.
lhnz 5y ago
> 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. You will be able to do more substantiative comments if you pull down their code and test it. It will improve the quality of your review, since it can be situated in the context of how the program works instead of how the code is abstracted. Your reviews should consider the practical workings of a program, and not just consistency with patterns or superficial bugs. 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.
- siva7 5y ago
  It’s a matter of efficient role separation. Most teams i’m aware of do not manually test a PR in the same cycle as a code review. It is absolutely expected that the PR author already tested his changes sufficiently. The manual test is usually reserved for the product or QA team.
  - pixel_tracing 5y ago
    Good luck getting junior or mid level engineers to do this properly each and every single time :) hell I’ve seen even seniors do this improperly
    - BobbyJo 5y ago
      If you can get people to test in review, I don't see why you couldn't get them to test before review. If you can't get your developers to test appropriately, then you've hired bad developers.
    - siva7 5y ago
      If there are engineers on the team wo assign PR for review that aren’t tested by themselves or don’t contain automated software tests (if the project has such standard) it’s time for the manager to have a 1on1 discussion about professional standards. Sometimes juniors aren’t aware of these shortcomings or are coming from a shop with bad development practices. They just need sometimes to learn professional practices and there is nothing lost.
wokwokwok 5y ago
Bluntly, this is the problem with code reviews: at the review stage it’s too late. 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-as-a-gateway/ https://blog.jetbrains.com/upsource/2017/01/18/code-review-a..., etc. google “code review as gateway”.
NotAnOtter 5y ago
A submitted code review comes with the following assumptions 1. The submitter approves of the code being merged into master 2. The submitted has tested the happy path and everything seems to work

by V-2 5y ago

fredrb 5y ago

> 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'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.

deleted 5y ago
[deleted]

kashug 5y ago

I agree that code review is not QA. But I read it as the writer of the post comes from a team where they do QA on pull-requests. And QA is assumed to be done on a per PR basis. But here I think different teams will have different approaches and pipelines. 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.

lhnz 5y ago

> 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. You will be able to do more substantiative comments if you pull down their code and test it. It will improve the quality of your review, since it can be situated in the context of how the program works instead of how the code is abstracted. Your reviews should consider the practical workings of a program, and not just consistency with patterns or superficial bugs. 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.

siva7 5y ago
It’s a matter of efficient role separation. Most teams i’m aware of do not manually test a PR in the same cycle as a code review. It is absolutely expected that the PR author already tested his changes sufficiently. The manual test is usually reserved for the product or QA team.
- pixel_tracing 5y ago
  Good luck getting junior or mid level engineers to do this properly each and every single time :) hell I’ve seen even seniors do this improperly
  - BobbyJo 5y ago
    If you can get people to test in review, I don't see why you couldn't get them to test before review. If you can't get your developers to test appropriately, then you've hired bad developers.
  - siva7 5y ago
    If there are engineers on the team wo assign PR for review that aren’t tested by themselves or don’t contain automated software tests (if the project has such standard) it’s time for the manager to have a 1on1 discussion about professional standards. Sometimes juniors aren’t aware of these shortcomings or are coming from a shop with bad development practices. They just need sometimes to learn professional practices and there is nothing lost.

wokwokwok 5y ago

Bluntly, this is the problem with code reviews: at the review stage it’s too late. 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-as-a-gateway/ https://blog.jetbrains.com/upsource/2017/01/18/code-review-a..., etc. google “code review as gateway”.

NotAnOtter 5y ago

A submitted code review comes with the following assumptions 1. The submitter approves of the code being merged into master 2. The submitted has tested the happy path and everything seems to work