MediaWiki

Code Health Group/projects/CodeReview/meeting notes/20190709

< Code Health Group | projects/CodeReview

2019-07-09 - Kickoff meeting

edit

Attendees: Jean-Rene Branaa, Lars Wirzenius, Andre Klapper, Gergő Tisza, Holger Knust, James Forrester, Kosta Harlan, Evan Prodromou, Niklas Laxström, Mukunda Modell, Will Doran

Introductions

edit

In the event that we don't already know each other

Logistics

edit
  • Meeting frequency, schedule, and duration
    • Proposal: Once a week for a month, then re-evaluate, at 07:00 SF / 14:00 UTC on Tuesdays.

Scope

edit

Problem Statement(s)

edit

What do we perceive the problem(s) to be?


Andre: [mostly taken from https://www.mediawiki.org/wiki/User:AKlapper_(WMF)/Code_Review ]

  • Weak review culture
  • Lack of enough skillful, available, confident reviewers and mergers
  • Hard to identify good reviewer candidates


Plus:

1.2        No culture to improve changesets by other contributors

2.1        Unstructured review approach

2.2        Unhelpful reviewer comments

2.3        Poor quality of contributors' patches

3.2        Under-resourced or unclear responsibilities

3.3        Workload of existing reviewers

4.2        Hard to realize a repository is unmaintained

4.3        Hard to find related patches

4.4        Lack of synchronization between teams


Mukunda: releng did a survey which collected a lot of comments about this subject already https://www.mediawiki.org/wiki/Developer_Satisfaction


Kosta:

  • Technical: Post screenshots in Gerrit, but that's less critical
  • Main issue is time allocation, recognizing and supporting
  • Every team has its own triage process

Gergő:

  • Missing organizational buy-in / valueing
  • Tooling: No CI support for new devs (apart from previously they got basic PHP linting, but that was killed two months ago)
  • Lack of culture of improving code review / how to [not] give CR feedback


JamesF:

Some friction by design, e.g. "harder" to get patches in because we've got a system that allows chains of patch dependencies (e.g. across repos)


Mukunda:

History: Deployed code (production) / branches far away from master


Lars:

Protecting production shall be better with new CI system


Evan:

Relying more on CI, Integration testing, unit testing should make CR process more pleasant and maybe better


Lars:

  • Scientific papers: Code review works (but only up to 200 lines of a diff); automated testing might work but less proven by science
  • Paper blogged by Greg Wilson, but link missing. Blog is http://neverworkintheory.org/
  • Let humans focus on what humans are better at doing (as it relates to reviews)


Will:

Hygiene important, what the work of the code review should be (purpose), knowledge sharing in our team, pairing within team, onboarding to areas of code, some anxiety

Andre:


Lars:

Should go: does this change make sense, does this architecture make sense, does this code look ok. Actually goes: here is 5000 line patch.


JamesF:

Gerrit allows to say +1 without merging, appreciated. Using -1 for experienced patch authors; sometimes fixing a typo and merging for new patch authors. People should not be afraid of giving CR and giving it with appropriate language tailored to the recipient, it's their job


Lars:

Some don't do as much code review. Some don't know how to do it well. How about having monthly training session.


JamesF:

  • In 2011–2 there was a policy that staff developers did one day of volunteer code review each week (and 4 days regular work): https://www.mediawiki.org/wiki/Wikimedia_engineering_20%25_policy . It failed.
  • Code review often does not happen because there isn't anybody comfortable to review that area of code. Code reviewers avoid certain areas of code in afraid of breaking things.


Mukunda:

I might see a simple patch to review, but I am not the person who should make the decision whether it is a good idea. I don't know who would be that person.


Holger:

Code review vs. runtime behavior. Any checklist for code review and quality target for certain area of the code?


James:

  • Not codified in process documents
  • Currently a conflict(?) over policy that new features should not be merged into deployed code without an owner. Volunteers surprised, where is that written?


<chat>Suggestion to put documentation/help/how to get review links inline in gerrit</chat>


Gergő:

We are a missing "intent to merge" feature. Currently you get notified when patch is created, and then when it is merged already.


Evan:

Can we set up a metric to track and use that to test our interventions on the process? See https://www.cast-safety.org/apex/f?p=102:1 for example of successful approach.


======= Not discussed in 2019-07-09 meeting as we ran out of time: =======

Is it measurable?

edit
  • Median patch queue time
  • Oldest unreviewed patch waiting for review
  • Volunteer developer retention (does the contributor make more patch requests)
  • Bugs in production (as a braking metric)
  • Sentiment analysis on comments (friendly, helpful responses)
  • Developer happiness surveys?
    • Good, BUT: these take a long time, they're infrequent, and self-reported numbers are less reliable than behaviour (making contributions)
  • Patch cycle times, split out by contributor type?


What we want to accomplish as a group

edit

What do we think we can do to solve the above mentioned problem(s)?

Other Info:

edit
Retrieved from "https://www.mediawiki.org/w/index.php?title=Code_Health_Group/projects/CodeReview/meeting_notes/20190709&oldid=5236674"