I've been having a debate about whether code should be checked out and run locally (or on a staging environment) as part of code review. Personally, I find it easier to review a change after I've run it locally, but perhaps that's because I'm not very good at reading code.<p>I'm interested to hear what other people think. Do you run code as part of code review?
I'm a senior frontend engineer doing a lot of code reviews and my team loves my MR (PR) submission standard:<p>- Require the author to attach screenshots of the feature in a markdown table format showing the before and after comparison. This is enforced using a MR template.<p>- Require the author to attach screen recording for complex user interactions.<p>Overall effect is that it saves everyone's time.<p>- The author has to verify that the feature works anyway, so taking screenshots / recordings doesn't take much addtional efforts.<p>- The reviewer can focus on code itself instead of checking whether the code works.<p>- Easy for both parties to spot design and UX issues/regressions visually.
Yes.<p>I didn't used to, but that was a very big mistake.<p>Just wait until something you signed off on doesn't run and folks ask, "But didn't you review this code and say that everything 'looked good'? It doesn't even run!"<p>It's embarrassing to say the least.<p>If you're doing a code review, run the darn code. What would you think of a mechanic who didn't make sure the car was able to turn on after claiming it was fixed?
No. That's the job of unit tests, integration tests and your CI/CD pipeline or alternatively the test environment (dev, staging, quality control, you name it...). Moreover, the person having written the code is supposed to have checked that it runs and meets business requirements - although the latter should be ultimately checked by your Product Owner or client.
This should be something agreed within a team, so that review standards are consistent across team members.<p>In my previous team, the reviewer was the main responsible for the code they were approving. They were expected to test locally and should actively hunt for potential issues, such as checking in the logs that the ORM was building correct SQL.<p>In my current team, the developer is the main responsible for the code they're pushing, and is expected to do all the tests needed. Reviews are more about ensuring code standards, finding possible oversights, requirements mismatches, and so on.<p>No one strategy is right or wrong, as long as expectations are set and everyone knows their responsibilities and expectations.
Most of the time I would expect the code to be already exercised by automated tests.<p>Sometimes, if it adds a new feature or something 'interesting', I've checked it out locally to see what the user-facing behaviour is, but this is very rare.
If you want to approach the highest quality code your team can manage, you want:<p>- the author to test and have a succinct PR description that includes before and after screenshots/video as appropriate<p>- the reviewer to test the code locally<p>The reviewer test is particularly important for front-end code which is where automated tests are more likely to be poorly written or absent.<p>It goes without saying that there should be targeted automated tests, but I find that in practice the front-end is where this tends to fall short.
Historically, I have rarely tried running code under review, but I have a coworker who routinely does this and by doing so, he has caught my mistakes a couple of times.<p>I should probably learn from him.
It depends on:<p>- what’s in the change; if I’m reasonably sure I understand it, and that it won’t have side effects, and that it’s the right thing to do, I may skip running it<p>- how familiar I am with the area of the codebase; even if the above is satisfied, I’ll pull it and explore around the change to look for potential side effects I’ve missed, or other approaches that might be better for maintenance… and generally to improve my familiarity<p>- how confident I am in the other contributor’s (and reviewers’) thoroughness; I generally scrutinize even trivial changes if I don’t think others have, because surprising things sometimes reveal themselves
Hell no. Also if this is your expectation you’re absolutely insane if this isn’t automated…also this kind of sounds like QA to me. My presumption is the base functionality requirements are met, I’m looking for potential performance issues, code standards, sound architecture, etc.<p>I am always amazed at how many repetitive tasks folks want to load up developers with. I think there’s a tendency among developers-turned-managers to see their job as crafting the perfect “program” of processes for their developers to follow, instead just automating these tasks. Like they say, the cobbler’s children have no shoes.
So far everyone is saying, “it depends”, and that’s my experience as well. Depending on the complexity of the change, how impactful it will be in production, what the test suite is like, etc. I may or may not pull and run it.
For me personally, code review is the place where you take a look at the code structure, architecture and if it is using the correct ways to do things. It's not the place to check if business requirements are met.<p>Code compilation (and checks) & tests must be requirement as part of each PR
Depends on the context and scope.<p>If it's something simple I can grok by reading the code and tests I usually don't.<p>If it's a complex piece of code, I find it valuable to run it locally, to setup some breakpoints, debug it and step it to understand it better.
We have a server automatically deployed for every branch, so most of the time if I want to see it running I can just check out that server.<p>Though sometimes I'll check out their branch, open up a repl and run a few functions or something on my own.
We have a CICD pipeline for every branch. We use squash.io which spins up a VM and runs the docker compose to create a per-branch QA env. The process however is that a developer does a code review on just the code first, then a tester tests the feature branch, then we merge and do some regression testing (manual and auto), then release.
Usually I don't, because the CI bot does it. I do read the code thoroughly though.<p>Only if something really draws my attention do I pull and run a particular PR (CR, CL, whatever yo call it). Once I did that to illustrate that the change introduces a security issue, so I had to write an exploit against it, and demonstrate how it works. (It helped.)
Yes we started doing this around 6 months ago. We started using [Github PR templates](<a href="https://docs.github.com/en/communities/using-templates-to-encourage-useful-issues-and-pull-requests/creating-a-pull-request-template-for-your-repository" rel="nofollow">https://docs.github.com/en/communities/using-templates-to-en...</a>) to prompt for instructions on how to run and test the PR. We are seeing following benefits :<p>- We are finding deficiencies in our testing/development methods. The more people have to test/run code the more we are able to find bottlenecks and eliminate them.<p>- Knowledge sharing. We found that team members had knowledge/tricks in their heads which are being shared more.<p>- The overall reliability of our systems(measured via SLOs) has greatly increased.
I only do it if I suspect that the code will not work. Sometimes I can think of edge cases that I suspect are not covered; so I check out the code, add the edge case as a unit test, and if it fails I mention it in the code review for the other dev to add and fix.<p>Other than that, no. I am reviewing the coding standard, architecture, and algorithms used. It's up to the dev to ensure that the code works and runs. The CI builds as well as the QA testers will test the actual functionality works as expected (obviously if I can't find the code that is supposed to fix/implement the PR item, I will question it).
Varies by company/project/team.<p>In most of my companies - no. The reason? Not practical for time constraints.<p>To be honest - this is mostly how you find out if a company values quality or speed. Most I’ve been at valued speed more than anything.
Only very rarely. Because we have a dedicated QA team (normally an antipattern but for complex enough software you some times end up building software you literally can’t run which is - yes - weird and difficult).
For us the developer is assumed to have run it, the CI checks that it builds and loads, code review is more for the decisions and any gotchas.<p>Bugs introduced will be caught and fixed in general testing usually.
Yes, absolutely. One time, my team was left without QA personnel, so we had to check features manually for a while. Even after this no longer became mandatory, running the code almost always reveals aspects worthy of a discussion as a part of code review. Obviously, this has a drawback of longer reviews, especially if testing a particular scenario involves quite a bit of effort. Would recommend, certainly beats longer feedback loop when the same issues are revealed during the QA acceptance phase.
Yes, UI tests are very hard to maintain on Android so I always pull and run the code to check the actual behavior.
As said by others compilation and linting are done by the CI.
I work on a frontend team, you usually want at least 1 person on the team who reviews code by pulling and running it. They can find unique bugs you didn’t think of while coding. For me, if I’m not busy I try to run it, if I’m busy I probably only give it a quick read. I try to maintain one development server whose purpose is to host my in review CR’s code so that reviewers can just click it and try it instead of having to pull it.
For smaller PRs we usually review without running locally unless there is something specific to check. And then we do separate release reviews where we install and run it locally as part or testing. This is for ML projects though, we are not doing any kind of continuous integration or continuous releases, and it's generally small repositories. It must really depend on the nature of the project and how the reviews fit in.
I think this is a good practice to think about - especially from the perspective of "if not, why not, and how can we make it easier." Some systems are "just complex" and will always be that way, but often this thought exercise can expose some low cost investments in developer tooling or staging environments which can lift all ships.<p>As for my own process, I have no hard and fast rule, but generally for user-impacting and functional UI changes I strongly encourage this on my teams. At companies with a functioning tooling setup, pushing to a remote staging environment is also useful because other stakeholders like Product and Design can make changes before the feature lands.<p>I do also advocate a fairly strict rule around screenshots before/after and a regression test for bug fixes. This prevents engineers from pushing speculative "I totally know what's going on" fixes which seem intuitively correct to a reviewer as well and then simply don't work. I'm tempted to do this all the time and even some really basic process does a good job at dissuading it.
In my experience the idea that you will catch all bugs by just reading the code is just wishful thinking - I saw plenty of bugs merged into the development branch on codebase I worked on at Google despite somewhat strict PR review process.<p>Now that I’m in startup land, we use Heroku’s Review App feature, which is by far my favorite Heroku feature, and helps us catch bugs all the time
I think this depends on the scale and type of the project, but it's good to get in the habit of codifying tests and setting up CI ASAP. It's basically scaling you pulling down code and running it. Now with containers, there isn't much of a reason to not have automated build checks and testing. Even when you have changes in a distributed environment, things like docker-compose make it much easy to create mocked CI environments with dbs, caches, etc. Also, CI helps me prioritize PRs when I have many to review. Being able glance at github to see which ones are mergeable immediately vs which ones may have other dependencies causing CI to break is great.<p>If you're working on some legacy codebase and don't have these luxuries, I totally get running it locally first. I am lucky to work with people who I do trust deeply to not waste others' precious time by testing first so there is probably also a human element for me.
While I think the author has the most responsibility to test changes, it's too easy to commit only some of the changes or otherwise make a change that's dependent on your development environment. Running tests and building code on CI prevents some, but not all of these mistakes.<p>Risk also needs to be taken into account. It's not necessary to spin up the application locally for tweaking error message wording or a small CSS change with no real responsiveness or accessibility implications. But for most fixes and features, I think at least a quick check is worth it. And if you're mentoring an intern or very junior developer, you should probably check every change, at least for the first few months.<p>Finally, the rest of your testing makes a big difference. If you have a formal QAer who tests your application before release or a community of people running nightlies, it's less urgent.
Mostly we depend on the CI actions (running automated tests and linters), but every PR also creates a feature branch on the testing/staging environments so anybody can run the app _as of that branch_ online. The URL schema is kind of like<p><a href="https://app-staging.foo.com/feature/add-sorting/" rel="nofollow">https://app-staging.foo.com/feature/add-sorting/</a><p>compared to the original<p><a href="https://app-staging.foo.com/" rel="nofollow">https://app-staging.foo.com/</a><p>given you have created a PR pointing from branch named feature/add-sorting to master.<p>This also makes it easier for our QA engineers to test things before they get merged to master.
I used to pull the code and run it for each PR, as I believed it just wasn't possible to read code adequately. Then I worked with one guy who clearly could read code very well - he found quite subtle issues just by reading it, and having seen it was possible, I put in the effort and learned to do it too. So in my case, my reason for always pulling and running the code was, as you suggest, that I simply wasn't very good at reading code. I still won't hesitate to pull and run code if I need to, but I'm pretty suspicious of code I can't read these days, and usually request it be made clearer.
In my last project at the current job, I've had one team member that always pulled the code and check it locally. And that from my experience was best code review that I've ever had. He sometimes found a bug in my code, because of something that I didn't tested, even before go to dev env and tested properly by QA.<p>No one else in the team was doing the same including myself. I'm not exactly sure but maybe part of the reason was that he was not much familiar with the code base, and checking if it works was the way to test if the code is correct.
No almost never. That’s the job of the CI build. If the code doesn’t even run then it should be caught by the tests. If there are no tests that would catch it then they must be written before passing code review.
Very rarely. We have presubmit actions that confirm everything is building and passing tests. And I insist on at least some degree of testing in all cases. Running it locally would be of little help for our case because the systems I work on are large enough that it's not trivial to examine behavior through local commands.<p>Think of an rdbms, and I'm reviewing changes that optimize or allow new schema options. It would not be productive for me to design new schemas each time I review code. Instead, the person sending the review should already have done that work in tests.
Every time I skipped that part and the change wasn't trivial, I missed something important, so yes.<p>I'm a big proponent of "trial by fire" reviews. The idea is to setup a dumpster environment, deploy the change there and give it for a spin to someone who's keenly interested in seeing some progress - sometimes it's the PM, other times the sales guy - anyone who has some time in between meetings and would appreciate a little bit of variety in their work life.<p>Such people are amazingly talented at finding catastrophic edge cases.
Yes, I read the code and also run the code locally. Reading the code is for strategic larger picture thinking and running the code is to find tactical errors in the implementation.
it depends on application size, if you have dedicated testers etc.<p>currently at a place with extremely large code base and with dedicated testers, I don't run the code.<p>Sometimes even with dedicated testers etc. I will run the code if I think something might be problematic and I want to double check. I tend to only comment on code though if I think it could really be improved, if you have named a variable something I wouldn't I probably won't care.
I wish there was a better workflow for this. We have code across ~10 repos at work, and I'm usually working on something else. So checking out somebody's code for pull requests is pretty disruptive. We generally consider "that the code runs" not to be part of code review, and have a separate testing phase before codes goes into production. Which works, but it definitely could be a smoother process.
this really depends on many factors like maturity of the tests and static code analysis, size and seniority distribution of the team and also if part of the pr acceptance is accepting the feature or if the team has a proper product management where the feature is accepted independent from the more code focused PR. i would totally agree with many comments, that a project that is way past mvp phase with proper feature acceptance and integration tests as part of the CI/CD pipeline should not require you to run the code for pr review, there are many phases in early projects where this might be necessary. eg when i review prs from junior devs where something just does not feel right, but change requests are too complex for simple comments on the pr i always use codespaces to run and manually test the code and understand what exactly they did and how things should be. also for a complex and critical feature i could not understand the PR just by reading the diff, its sometimes necessary to run and see the code in context to really understand it.
I tend not to run stuff locally due to the complexity of setting up whatever is being tested. This goes especially for bug fixes and 3rd party integrations.<p>I do however check out code locally. Online code review only shows what's been changed and I often find subtle issues in the code that wasn't changed. In short I'm trying to ask myself: What's missing in implementation?
I don't have any particular rule for it, it depends entirely on the PR. If I feel confident that I can review the change without running it locally, I don't do it. But if I feel that my review might be compromised by not seeing the behaviour first hand or that I need to step through some part of the code myself to properly understand it, I'll run it.
It depends. If looking at the diffs isn’t enough to really understand the change, then yeah, I’m pulling it down. I can more easily navigate using static analysis tools that way. I can also play with it too to find edge cases.<p>I certainly wouldn’t make it a requirement of reviewers. I leave it to them to make their own determination on whether a patch is satisfactory.
Every person saying "no" is probably part of a full, competent team, that has automated tests for everything, QA folks - that truly understant what they are doing and what are the business requirements, etc, etc.<p>If you are missing stuff like this, than yes. I'm surprised at the amount of time developers deliver something that doesn't even boot
I made a little service called quarantest[0] for web apps that you can point at a GitHub repo. For every PR/push it does a fresh build and makes the app available at a url based on the commit hash.<p>[0]: <a href="https://github.com/anderspitman/quarantest" rel="nofollow">https://github.com/anderspitman/quarantest</a>
I use Emacs so yes of course[1]. Not only do I get a nice editing environment for editing comments, I can also take advantage of language server features to make navigating and understanding the change easier.<p>[1] <a href="https://github.com/magit/forge" rel="nofollow">https://github.com/magit/forge</a>
Where i work ehen a PR is created, a copy of the environment is spinned with the changes automatically by the CI peocess. Mainly so that QA can test the PR.<p>So people doing code review can poke at the resulting running program if they like. But generally code review is used for the "how" instead of the "does it work?"
I imagine this is particular to company/team policy. In my case, the team does 2 reviews before a deployment. The first validates architecture and gerneral structure, what is considered not be a code review. The second is a "functional" validation, where the person must run and test the use cases.
it varies. I work on mobile applications, the last place I worked did not afford us much/any time to really do quality unit testing.<p>If it's code that is really changing behavior significantly then I'll run it on my device and just see if I notice anything. I've found some surprisingly gnarly bugs this way.
I almost never run the code locally unless I see something questionable and can’t reason about what it will do because I’m unfamiliar with something in it. For that, I tend to use a REPL for just the snippet of questionable code.<p>However, if the code is a large enough change, and it needs testing anyways, I will run it.
No.<p><pre><code> 1. PRs should target development, which is an *unstable* branch, so no biggie if something breaks
2. CI should catch things like syntax errors, failing tests, or poor test coverage
3. I trust my colleagues to do their work properly and fix things if they've made a mistake</code></pre>
It depends.<p>Your CI should be robust enough to catch "does it run" type issues. That's not always possible for one reason or another. If there are things that your CI can not currently check, you should be verifying local. You should also be striving to close those gaps in your CI.
I run data pipelines using some
Docker as a function pattern and if the ELT is small o kickoff to see if it works and then I start the review. Takes a bit more time but at the end of the day everyone has a bit more security to review a “compiled” piece of code for our ELT.
I've worked mostly on back-end stuff and "works for me/this" is asking for trouble. Making sure it builds is literally the minimum review you can do. You learn whether it actually compiles and whether it requires changes to the development environment.
Usually no. There are situations in which I want to understand better how something is working or if an edge case was taken into account, or if I want to suggest a change and I wanna check my suggestion against the code first.<p>In those cases I’ll pull the code
Sometimes I do, esp. when CI is behind and doesn't have the latest compilers, sanitizers and not enough warnings turned on.<p>or as in the latest case when the developer has the newest msvc compiler with ' as digit separators, but the CI not.
Not generally, and I prefer not to. The code being clear enough to read is one of our review criteria.<p>But... our system is a large pile of legacy crap, and too often the only way to check something properly is to step through it in a debugger.
Code that I haven't tested is not code I can give a thumbs up on. Yes, we pull and test. This is enforced for any PRs that actually change some code (with the obvious exceptions of comments, formatting, or similar PRs)
Depending on the change and my understanding of it, I will: (a) review the changes, (b) browse the version of the files on GitHub, (c) checkout the version locally and browse in IDE, (d) run stuff (app, tests, console).
We have bird slots like: Canary, Eagle, Owl etc. When there is a PR according to its order it gets deployed to one of them.<p>Then we have preflight.com UI tests that run on those environments. Would love to tell you my experience.
Sometimes, mostly for important features or for cases when I am not sure if it will work or how it works. On the other side I run code locally whenever I do PR, this gives me more confidence in changes I made.
Sometimes. It was worth it every time, too. "I thought this was working"–well, it wasn't.<p>Most of the time I pull the code to easily navigate it in context. The diff viewer will only get you so far.
Generally, no, it’s CI’s job to build and run tests on every PR. On some occasions when tests do not cover some condition, I ask to add the test, instead of running it myself.
I expect automated tests to run, and where practical a demo link with the change to be provided for larger changes that aren't captured by automated testing.
once in a while, a new feature (or fix) can mess up with another new feature (or fix). And running those branches separately would be ok by themselves. But when both merge, then stuff goes wrong.<p>So it might be useful to run the thing before AND after merging, but that is very very tedious..