51–60 of 146 posts
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.
I'm a senior frontend engineer doing a lot of code reviews and my team loves my MR (PR) submission standard: - 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. - Require the author to attach screen recording for complex user interactions. Overall effect is that it saves everyone's time. - The author has to…
In most of my companies - no. The reason? Not practical for time constraints.
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.
Yes. I didn't used to, but that was a very big mistake. 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!" It's embarrassing to say the least. 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?
- the author to test and have a succinct PR description that includes before and after screenshots/video as appropriate
- the reviewer to test the code locally
The reviewer test is particularly important for front-end code which is where automated tests are more likely to be poorly written or absent.
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.
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.
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.
I'm a senior frontend engineer doing a lot of code reviews and my team loves my MR (PR) submission standard: - 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. - Require the author to attach screen recording for complex user interactions. Overall effect is that it saves everyone's time. - The author has to…
Yes. I didn't used to, but that was a very big mistake. 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!" It's embarrassing to say the least. 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?
Or have a machine run the code, like a CI system?
Yes. I didn't used to, but that was a very big mistake. 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!" It's embarrassing to say the least. 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?
Or have a machine run the code, like a CI system?