10 ms·

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

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 verify that the feature works anyway, so taking screenshots / recordings doesn't take much addtional efforts. - The reviewer can focus on code itself instead of checking whether the code works. - Easy for both parties to spot design and UX issues/regressions visually.

lazypenguin 5y ago
I’ve found this works extremely well for visual changes and can’t believe when it’s not the default behavior!
Folcon 5y ago
This is great, do you do this in github? Or elsewhere? Just curious if you had a template to hand that myself and others could use =)...
- x86a 5y ago
  "MR" so probably Gitlab
  - Folcon 5y ago
    Ah didn't know that, less familiar with gitlab.
- paradite 5y ago
  The PR/MR template itself is pretty generic. You can make one yourself quickly by referring to GitHub markdown table documentation: https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/organizing-information-with-tables https://docs.github.com/en/get-started/writing-on-github/wor...
dinkleberg 5y ago
This is an excellent idea for a submission standard, will have to store this in the back of my mind.
mnutt 5y ago
You can even automate this with something like Percy that takes screenshots and does visual diffs at selected points in the test suite. You just have to be careful about including random/date-based data in your tests or you’ll get more diffs than expected. Also not quite a substitute for someone calling out “this was a major UX change”.
- paradite 5y ago
  Interesting to hear that there are automation solutions around this. In your experience / expertise, how much work can be automated by such tools? 80%? 99%?
BossingAround 5y ago
But you still have to test the actual functionality, don't you? What if the author makes a gif of clicking a button, but doesn't record if the browser back button still works? I'd think that for frontend, you'd have CI/CD pipeline that deploys the code into a staging server, where I can test it myself.
- stephen_cagle 5y ago
  I remember a few companies ago we had a system that would deploy every branch to a separate subdomain (same name as the PR) that you could access. It was fantastically useful for just seeing what the deployed code would look like. I think (for UI things at least) this is a very reasonable solution. Wish I could remember the github service that did this?
  - julienfr112 5y ago
    the neame is review app, it's on heroku since forever, also in gitlab ops today.
  - heygarrison 5y ago
    Vercel and Netlify do PR reviews these days. Was it in the JS space? I think render.com is going to I introduce this soon too.
  - klohto 5y ago
    There is already Vercel, Netlify, etc. that support this feature. You can also easily do your own version with Cloudfront and S3 or just a custom nginx config.
- ryan_lane 5y ago
  Isn't this what automated tests are for? Manually testing every change is a huge burden, requires most of the automation necessary for automated testing (from the infrastructure point of view), and actually discourages people from writing automated tests.
- paradite 5y ago
bobobob420 5y ago
The fact that people think this is an excellent idea.....screen recording the proof that the feature works wtf what is the alternative you are getting? People are submitting features that don't work? Just fire them or teach them your set of standards. If my boss asked me to do this I would laugh and find a new job. I truly feel sorry for you but also please stop ruining our industry with this nonsense.
- paradite 5y ago
  Requiring screen recording isn't about lack of trust or lack of competence. Rather it's a way of eliminating information asymmetry between the author and the reviewer: - The author might have checked that the code works fine (A) or haven't done it (B). - The reviewer might decide to run the code locally (1) or not (2). Combination A1 results in duplicate effort. Combinations A2 and B1 are perfect. Combination B2 results in potential bugs. The MR standard merely eliminating the combinatorial possibilities by sharing information between the author and the reviewer automatically. The end result is that both parties know A2 is the process that the other person follows.
  - bobobob420 5y ago
    All you have to do in a code review is review the code. You do not have to run the code nor should you be unless it is a very tiny project. It is the developers responsibility to test the functionality and get it ready for the QA process. If there is a dedicated QA that can test their functionality then the QA is responsible for testing on that front. The code reviewer is not supposed to be testing the feature works as intended for product. The manager of the team or product or client or stakeholder would be the ones testing yhe functionality and giving it a go ahead. It is not the job of the code reviewer to do that. It means you have not taught your developers the set of standards and practices to follow. If you have to get a screen recording of a feature working (probbaly in local or dev so its as useful as a unit test) then you or whoever is responsible has failed at creating a proper software development process
    - paradite 5y ago
slaymaker1907 5y ago
Word is actually really great for this stuff. You get a portable file and can add notes to the screenshots. There is even a screenshot button built into Word! For recordings, if you use Windows you can bring up the game bar with Win+G. It has a very easy to use screen capture tool meant for gaming, but perfectly fit for small software demos. It even captures sound which is sometimes useful.
- tomatohs 5y ago
  I'm working on an "instant replay for developers" desktop app for exactly this case. Create desktop instant-replays to embed in pull requests: http://replayable.io/ http://replayable.io/

lazypenguin 5y ago

I’ve found this works extremely well for visual changes and can’t believe when it’s not the default behavior!

Folcon 5y ago

This is great, do you do this in github? Or elsewhere? Just curious if you had a template to hand that myself and others could use =)...

x86a 5y ago
"MR" so probably Gitlab
- Folcon 5y ago
  Ah didn't know that, less familiar with gitlab.
paradite 5y ago
The PR/MR template itself is pretty generic. You can make one yourself quickly by referring to GitHub markdown table documentation: https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/organizing-information-with-tables https://docs.github.com/en/get-started/writing-on-github/wor...

dinkleberg 5y ago

This is an excellent idea for a submission standard, will have to store this in the back of my mind.

mnutt 5y ago

You can even automate this with something like Percy that takes screenshots and does visual diffs at selected points in the test suite. You just have to be careful about including random/date-based data in your tests or you’ll get more diffs than expected. Also not quite a substitute for someone calling out “this was a major UX change”.

paradite 5y ago
Interesting to hear that there are automation solutions around this. In your experience / expertise, how much work can be automated by such tools? 80%? 99%?

BossingAround 5y ago

But you still have to test the actual functionality, don't you? What if the author makes a gif of clicking a button, but doesn't record if the browser back button still works? I'd think that for frontend, you'd have CI/CD pipeline that deploys the code into a staging server, where I can test it myself.

stephen_cagle 5y ago
I remember a few companies ago we had a system that would deploy every branch to a separate subdomain (same name as the PR) that you could access. It was fantastically useful for just seeing what the deployed code would look like. I think (for UI things at least) this is a very reasonable solution. Wish I could remember the github service that did this?
- julienfr112 5y ago
  the neame is review app, it's on heroku since forever, also in gitlab ops today.
- heygarrison 5y ago
  Vercel and Netlify do PR reviews these days. Was it in the JS space? I think render.com is going to I introduce this soon too.
- klohto 5y ago
  There is already Vercel, Netlify, etc. that support this feature. You can also easily do your own version with Cloudfront and S3 or just a custom nginx config.
ryan_lane 5y ago
Isn't this what automated tests are for? Manually testing every change is a huge burden, requires most of the automation necessary for automated testing (from the infrastructure point of view), and actually discourages people from writing automated tests.
paradite 5y ago

bobobob420 5y ago

The fact that people think this is an excellent idea.....screen recording the proof that the feature works wtf what is the alternative you are getting? People are submitting features that don't work? Just fire them or teach them your set of standards. If my boss asked me to do this I would laugh and find a new job. I truly feel sorry for you but also please stop ruining our industry with this nonsense.

paradite 5y ago
Requiring screen recording isn't about lack of trust or lack of competence. Rather it's a way of eliminating information asymmetry between the author and the reviewer: - The author might have checked that the code works fine (A) or haven't done it (B). - The reviewer might decide to run the code locally (1) or not (2). Combination A1 results in duplicate effort. Combinations A2 and B1 are perfect. Combination B2 results in potential bugs. The MR standard merely eliminating the combinatorial possibilities by sharing information between the author and the reviewer automatically. The end result is that both parties know A2 is the process that the other person follows.
- bobobob420 5y ago
  All you have to do in a code review is review the code. You do not have to run the code nor should you be unless it is a very tiny project. It is the developers responsibility to test the functionality and get it ready for the QA process. If there is a dedicated QA that can test their functionality then the QA is responsible for testing on that front. The code reviewer is not supposed to be testing the feature works as intended for product. The manager of the team or product or client or stakeholder would be the ones testing yhe functionality and giving it a go ahead. It is not the job of the code reviewer to do that. It means you have not taught your developers the set of standards and practices to follow. If you have to get a screen recording of a feature working (probbaly in local or dev so its as useful as a unit test) then you or whoever is responsible has failed at creating a proper software development process
  - paradite 5y ago

slaymaker1907 5y ago

Word is actually really great for this stuff. You get a portable file and can add notes to the screenshots. There is even a screenshot button built into Word! For recordings, if you use Windows you can bring up the game bar with Win+G. It has a very easy to use screen capture tool meant for gaming, but perfectly fit for small software demos. It even captures sound which is sometimes useful.

tomatohs 5y ago
I'm working on an "instant replay for developers" desktop app for exactly this case. Create desktop instant-replays to embed in pull requests: http://replayable.io/ http://replayable.io/