Wikimedia Apps/Team/iOS/Engineering Review/March 27 2015
(Redirected from Wikimedia Apps/Team/iOS/Code Review Meeting/March 27 2015)
 | This page is obsolete. It is being retained for archival purposes. It may document extensions or features that are obsolete and/or no longer supported. Do not rely on any information on this page. |
Topics[edit]
Makefile & external dev setup[edit]
- What is the role of a make file?
- Simple setup
- Advanced setup
What, Why, & How of Code Review[edit]
Ideally the diff, tests, and task speak for themselves. Otherwise, gerrit patch commit message (or GH pull request) should contain a high level description and pointers to the following:
- Why: Why is the change necessary? Pointers to issues in our tracker (i.e. Phabricator) should allow reviewers to get full context of a change, including requirements, screenshots, reproduction steps, etc.
- What: What was changed? The diff should be as salient as possible, with minimal unrelated changes. This doesn't preclude developers from making small refactors in files that are already touched as part of addressing the why.
- How: How does the change achieve the desired effect? Ideally the code should speak for itself, but tests are the best way to convey design and expected behavior. If it's not feasible to test a set of changes, the author should provide an adequate description or offer to pair with reviewers to brief them on the how.
Issues[edit]
- Legacy code makes understanding difficult, so extra measures need to be taken to facilitate understanding
- Gerrit literally squashes discrete history, forcing devs to tell story of "how" using other means
- Gerrit also forces the commit message to be the description of the entire changeset (for better or worse)
- Can't use a rich medium to tell the story, e.g. GitHub PR
- No tools for automated user/visual tests
- Reduce engineer testing time
- Leverage testers for "Higher Value Testing" (fuzz, exploratory, edge cases, hard-to-automate, i.e. not regression)
Actions[edit]
- Try for smaller patches w/ pointers to phab tickets (w/in sprint ideally)
- Try Dropbox approach of pairing w/ a reviewer early on in task lifecycle
- When exceptions to the best practice of testing occur, we need to flag the task so we can measure the cost of not having automated tests to cover those cases
- Create a tag project in Phabricator to tag issues lacking sufficient testing tools
- Further investigate using Github while Diffusion is being built
- Experiment with submitting a GH merge commit as a patch in gerrit