CodeReviews

Differences between revisions 1 and 4 (spanning 3 versions)
⇤ ← Revision 1 as of 2012-10-25 14:41:52 →
Size: 1984
Editor: 91
Comment:
← Revision 4 as of 2012-10-25 15:09:54 → ⇥
Size: 3010
Editor: 91
Comment:
Deletions are marked like this. Additions are marked like this.
Line 4: Line 4:

Line 7: Line 5:
 * 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, ...).  * 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, ...).
Line 28: Line 26:
   * [[https://wiki.ubuntu.com/HelpOnMoinWikiSyntax|Feature Freeze]]    * [[https://wiki.ubuntu.com/FeatureFreeze|Feature Freeze]]
Line 30: Line 28:
 * 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 42:
 * 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?)		  * 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)
 * 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

NOTE: This is work in progress

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

  • 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
  • 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)
  • 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)