CodeReviews

Differences between revisions 1 and 8 (spanning 7 versions)
⇤ ← Revision 1 as of 2012-10-25 14:41:52 →
Size: 1984
Editor: 91
Comment:
← Revision 8 as of 2013-01-22 15:09:48 → ⇥
Size: 3186
Editor: 198
Comment:
Deletions are marked like this. Additions are marked like this.
Line 1: Line 1:
= NOTE: This is work in progress =
Line 4: Line 2:

Line 7: Line 3:
 * You wrote a test. The lower level the better (1. unit test, 2. autopilot test, 3. manual test). Manual tests are strongly discouraged and for visual stuff only (layout, rendering, ...).
 * If there is an autopilot test add Thomi ([[https://launchpad.net/~thomir|~thomir]]) or Chris ([[https://launchpad.net/~veebers|~veebers]]) as a reviewers		  * A test is added. The lower level the better (1. unit test, 2. autopilot test, 3. manual test). Manual tests are strongly discouraged and for visual stuff only (layout, rendering, ...).
 * If there is an autopilot test add Thomi ([[https://launchpad.net/~thomir|~thomir]]) or Chris ([[https://launchpad.net/~veebers|~veebers]]) as a reviewers. Ping them on IRC to make sure they see the review request!
Line 27: Line 23:
 * ubuntu processes are followed. This primarily means you are aware of:
   * [[https://wiki.ubuntu.com/HelpOnMoinWikiSyntax|Feature Freeze]]		  * Ubuntu processes are followed. This primarily means you are aware of:
   * [[https://wiki.ubuntu.com/FeatureFreeze|Feature Freeze]]
Line 30: Line 26:
 * Design review is requested when it is needed
   *
 * If a packaging change is needed add integration team as a reviewer (timo/lukasz)		  * Design review is requested when it is needed. It is needed when either
   * A visual change has been made to the interface and/or
   * A change has been made to the user facing functionality
 * If a packaging change is needed add integration team as a reviewer -- either [[https://launchpad.net/~timo-jyrinki|~timo-jyrinki (mirv)]] or [[https://launchpad.net/~sil2100|~sil2100]]
Line 42: Line 40:
 * Does it have tests
 * Make sure it builds and tests are passing (ideally just check jenkins results)
 * Was it tested by MP creator
 * If there is autopilot code -- was Chris/Thomi added as reviewers?
 * If design review is needed -- is design review requested?
 * If there is a packaging change -- did popey's team approve?
 * Are the bugs linked?
 * ubuntu processes are followed (is this a new feature and are we past feature freeze/UIf?)		   * ...
 * Does the merge proposal contain unrelated changes?
 * Check if test was added. The lower level the better (1. unit test, 2. autopilot test, 3. manual test). Manual tests are strongly discouraged and for visual stuff only (layout, rendering, ...).
 * Make sure it builds and tests are passing (ideally just check jenkins results; jenkins votes on the MP automatically) -- verify 'make check' passes if available, since jenkins does not always run these tests
 * Check if it was tested by MP creator (check the description)
 * Check if there is an autopilot test and if Thomi ([[https://launchpad.net/~thomir|~thomir]]) or Chris ([[https://launchpad.net/~veebers|~veebers]]) were added as reviewers
 * Check if design review is requested when it is needed. It is needed when either
   * A visual change has been made to the interface and/or
   * A change has been made to the user facing functionality
 * Check for a packaging change and whether [[https://launchpad.net/~timo-jyrinki|~timo-jyrinki (mirv)]] or [[https://launchpad.net/~sil2100|~sil2100]] were added as reviewers
 * Check if all the bugs are linked to this branch
 * Make sure ubuntu processes are followed. This primarily means you are aware of:
   * [[https://wiki.ubuntu.com/FeatureFreeze|Feature Freeze]]
   * [[https://wiki.ubuntu.com/UserInterfaceFreeze|UI Freeze]]
Line 51: Line 55:
 * Don't forget about the global state  * Don't forget about the global state of the MP if you are the last reviewer

Responsibilities of Merge Proposal (MP) creator

  • Test the functionality before creating MP (you were actually running your branch in a real environment)
  • A test is added. The lower level the better (1. unit test, 2. autopilot test, 3. manual test). Manual tests are strongly discouraged and for visual stuff only (layout, rendering, ...).

  • If there is an autopilot test add Thomi (~thomir) or Chris (~veebers) as a reviewers. Ping them on IRC to make sure they see the review request!

  • All the bugs you fixed are linked to your branch
  • Commit message in the launchpad Merge Proposal is set
  • Merge proposal description is matching the following template: 

= Problem description =

<describe the problem being fixed, or refer to linked bugs>

= The fix =

<describe how the code changes in the diff fix the problem described>

= Test coverage =

<What tests confirm correct behaviour>

  • Ubuntu processes are followed. This primarily means you are aware of:
  • Design review is requested when it is needed. It is needed when either
    • A visual change has been made to the interface and/or
    • A change has been made to the user facing functionality

  • If a packaging change is needed add integration team as a reviewer -- either ~timo-jyrinki (mirv) or ~sil2100

Responsibilities of reviewer

  • Coding style
  • Code correctness
    • flaws in the code
    • performance
    • ...
  • Does the merge proposal contain unrelated changes?
  • Check if test was added. The lower level the better (1. unit test, 2. autopilot test, 3. manual test). Manual tests are strongly discouraged and for visual stuff only (layout, rendering, ...).
  • Make sure it builds and tests are passing (ideally just check jenkins results; jenkins votes on the MP automatically) -- verify 'make check' passes if available, since jenkins does not always run these tests
  • Check if it was tested by MP creator (check the description)

  • Check if there is an autopilot test and if Thomi (~thomir) or Chris (~veebers) were added as reviewers

  • Check if design review is requested when it is needed. It is needed when either
    • A visual change has been made to the interface and/or
    • A change has been made to the user facing functionality

  • Check for a packaging change and whether ~timo-jyrinki (mirv) or ~sil2100 were added as reviewers

  • Check if all the bugs are linked to this branch
  • Make sure ubuntu processes are followed. This primarily means you are aware of:
  • Check commit message
  • Don't forget about the global state of the MP if you are the last reviewer

Unity/CodeReviews (last edited 2013-01-22 15:09:48 by 198)