[lhnz]

  > 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]

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.

[lhnz]

It doesn't seem possible to me that "role separation" could be more efficient.

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

[pixel_tracing]

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

[ljm]

It's a lot easier to do in an application that's easy to boot up locally. A lot of software in web-startup world grows out of that stage once the k8s/distributed architecture earworm burrows in.

[WorldMaker]

It can be an automation thing. With tools like (Github) Codespaces there are places today that have the ability to deploy every random PR branches and do PRs in a full code editor backed with a running deployment that you can browse. Admittedly, that's a lot easier to do when what you are deploying is web-based and automating spinning up new URLs (even just proxy forwarded localhost URLs in the case of Codespaces) is easy and things are a lot harder to automate with for instance mobile apps.

[siva7]

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.

[BobbyJo]

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.

[ozim]

Efficient role separation?

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.