Apps-android-commons: Reduce cost of reviews - Screenshot

[akaita]

In #829 @misaochan said 40% of the time spent in maintenance was about reviewing PRs.
A good way to reduce this asking for Before&After screenshots and/or videos of PRs.

This simple technique has multiple effects:

• forces the author to run the app in order to take the screenshot
• forces the author to review his own work once he is done
• provides a way to review the results at a glance
• limits the scope of the PR and makes its goal clear
• people who might not be reviewing that PR can still see the what's going on

This would need from us:

• [ ] Create instructions about how to create a Before&After table in markdown
• [ ] Create instructions about how to take a screenshot
• [ ] Create instructions about how to take a video and convert it to gif

Does that sound good?

[misaochan]

I like the idea, @akaita . A video might be too much work or sound too intimidating, but a screenshot (especially an After screenshot) would be great. :) That would definitely be a huge help for the reviewers! And you're right that it "forces" (I prefer to use the term "encourages", haha) the author to run the app and test it, which does prevent the rare case of people submitting completely unworkable PRs.

Even if we only require an After screenshot though, I worry that it might put new contributors off, especially if other beginner-friendly repos don't require one. But it might be a worthwhile tradeoff.

Of course, we would need to stipulate that certain types of PRs don't require screenshots, e.g. those that fix lint issues etc.

[neslihanturan]

I also like the idea. I can add some additional things that we can add to readme. Of course it is not required for all types of PR's but,

• If PR requires some shared preferences
  
  
  
  • Testing it after clearing app data also
  
  
• If it is login task related,
  
  
  
  • Testing it with logging-out and logging-in again.
  
  
  • With fresh installation of the app
  
  
• If it is nearby realted
  
  
  
  • Testing it with location-on,off
  
  
• For all kinds of PR's
  
  
  
  • Testing them with at least different test devices with different versions
  
  

would be very helpful for reviewers. Even if it is not possible to do all of them on each PR, PR owner can note tests that are done to description. So that reviewer doesn't need to do them again.

Please feel free to add new items to my test list.

[neslihanturan]

Actually it can be nice if reviewers also note what they have tested so far in comments with test device versions, so that other reviewers can finish remained tests and merge.

[janpio]

Great idea. The documentation of this itself should live on the wiki, but also think about additionally creating a pull request template that users will see when creating a PR. This can include checkboxes to fill out and of course also links to the wiki explanation of what is expected with a PR.

First step is probably to create the docs in the wiki and then build a pull request template based on that.

[neslihanturan]

Agree with checkboxes and PR template @janpio .

[akaita]

Having a template for PRs sounds like a good one.
We just have to make sure we point as few requirements as possible. Each requirement decreases the time spent in reviews, but it also increases the entry-barrier for authors.

We should find a good balance.
I would propose that we add requirements iteratively:

1. add one requirement
2. if PRs still take too much maintenance effort, add another requirement
3. if PR amount decreases under an adequate level, re-evaluate existing requirements

I propose we add the Before&After screenshot requirement to the PR template, only to be used if the PR modifies the UI.

Then, the checkboxes listed by @neslihanturan sound to me like something that can require knowledge about the app's inner-workings. Maybe it is something that a maintainer/contributor has to manually add?

[misaochan]

My suggestion for the initial PR requirements (we can add more requirements later if needed, as @akaita suggested):

1. "After" screenshot for all PRs that modify the UI. I personally do not think the "Before" screenshot is absolutely essential - it would be nice but I'm not sure the onus should lie with the contributor to document the existing state of the repo. If we do mention a "Before" screenshot it could possibly be listed as "recommended but not mandatory"

2. As @neslihanturan mentioned, all PRs should require the contributor to mention what tests they have carried out, with what API level and device. At minimum there should be at least 1 test with the target API level. Additional tests with other API levels or devices could, again, be "recommended but not mandatory"

[neslihanturan]

I have an additional idea to reduce review costs:

-Removing PRs (or making requested change ourselves) when requested changes are still waiting during more than 30 days.
What do you think?

[nicolas-raoul]

@neslihanturan I believe that a sleeping PR is better than no PR at all.
Volunteers come and go, and most do it for fun.
Let's say Alice implemented a feature and send a PR because she had a few hours on a rainy Sunday. Meanwhile master has changed a bit and the PR must be rebased. Rebasing is everything but fun, and it might be months before the next rainy Sunday. Deleting the PR would lead to the Alice's branch being forgotten, which would be a loss. I believe the right thing to do would be to link to the PR on the issue page and explain what is left to do, and maybe assign the "beginner-friendly" label if most of the work is done.

[misaochan]

I believe the right thing to do would be to link to the PR on the issue page and explain what is left to do, and maybe assign the "beginner-friendly" label if most of the work is done.

+1 if there is anything else needed besides rebasing... IMO if only a rebase is needed we could just do it ourselves, since it's less work than creating new issues, assigning etc., and doesn't really benefit a newcomer to do it.

I personally wouldn't go with removing PRs @neslihanturan , unless there's a very good reason for it (like the instant Android Studio codestyle change with lots of conflicts, where it could be reimplemented with the click of a button). If you are finding your workload too heavy it's OK to leave them for some time. :)

[neslihanturan]

Actually my concern was issues being forgotten with their sleeping PRs. No one wants to work on them since they think Alice will come back one day. I liked @nicolas-raoul 's idea about encouraging related issue. Can we decide a minimum day limit to re-promote the issue? And this minimum day limit can be used while answering "Can I work on this issue" requests. Most of the times, I don't want to take initiative and leave the question someone else to answer:)
Besides, sleeping PRs may seem like no one doesn't care with this repo for newcomers.

[nicolas-raoul]

It is true that before contributing to a repository, I always look at the number of open PRs, and sometimes give up if there are too many compared to the size of the project, as it make it feel like nobody processes PRs. Too bad Github does not show a few recently closed PRs on the same screen.

[misaochan]

@neslihanturan Ah, yes, good point. Maybe after 30 days, we can re-promote the issue and suggest that anyone is free to work on the issue, but that they preferably try and build on the last PR instead of starting from scratch?

[neslihanturan]

Including me, we sometimes don't completely fit in with decisions taken under this discussion. So I wrote this comment to up this topic as a reminder:)

[maskaravivek]

Now that we have PR templates in place and contributors are mostly uploading a Screenshot, can we close this issue. Reviewing all the open PRs still takes considerable time but the process has become much more streamlined in the past few months. :)

[misaochan]

Sounds good to me. :)