The simple way intended to find mistakes overlooked in the initial development phase.
The goal is to improve overall quality of the code .
Practices vary from pair programming, informal walkthroughs, and formal inspection.
ONAP uses Gerrit for Code Review 
What to review?

  1. Comply to Coding Guidelines: see Java code style
  2. Comply to Intellectual Property (Copyright and license headers)
  3. Comply with Commit Process
  4. Ensure there is no Chinese Character in comments

Code reviews are expected to take place within the next 36 business hours after the code has been submitted within Gerrit.

ONAP is an Open Source project and thus anyone who is interested can provide code review.

Best candidate to ensure the code is reviewed are the “Committers” listed in project resources.

Authors are not expected to perform Code Review on their own code.  This rule is especially true for integration work.


See also Committer Best Practices

Rules

Mandatory for all projects, whatever their state.

Self-commits are not allowed

All Commits must be reviewed and integration tested.

Depending on the project state, there is a variable depending on the % of committers to perform code review

Feedback provided by voting "-1" (issue), "0" (ok, neutral, abstain), "+1" (approve)

  • Incubation: “+1” from 30% of committers (min: 2); no “-1”
  • Mature: “+1” from 60% of committers; no “-1”
  • Core: “+1” from 75% of committers; no “-1”

The code should not be accepted if there are open issues, but should as soon as the minimum threshold of reviews have come in.

7 Comments

  1. Gildas Lanilis

    Practice approved by TSC 2017-07-13 (topic #10). PDF Summary of materials presented.

  2. Dan Timoney

    2 comments on the timeframe:


    1) I think the agreement is 36 business hours, not 24.

    2) That timeframe should only apply to changes that (a) are in Verified state (meaning the automated test compile passed) and (b) have no merge conflicts.  If either of those conditions exist, the change should not be merged until the issue is cleared and the change should remain open so the develop can clear the issue.

    1. Pamela Dragosh

      I agree with Dan. We would like to have some flexibility in staging code review submissions. We don't want to lose some valuable work from someone who is going on vacation and gets work done before others. Giving time to another person/team get some other work submitted and merged before going back and fixing what the first person had submitted. We don't want to have to wait for them to come back from vacation, or have someone else be required to duplicate their work (and then they lose credit for doing it). It would be great if we can allow merge conflicts to exist, so we can better funnel changes happening. This can be documented via "reply" messages in the code review. 36 business hours seems like a long time, but occasionally this isn't possible.

      1. Gildas Lanilis

        Instead of making exception for the scenario you have described, I would rather like to enforce the "Continuous Integration" development best practices. Key practices such as "Developer is responsible for fixing the build" or "Fix it immediately or revert the change" should be in the DMA of a developer.  A merge conflict must be fixed by the developer before he leaves the office. Before a developer leaves for vacation, he should ensure his code has been integrated properly by committing frequently and not a lost minutes (at 4:55 pm on a Friday).

    2. Gildas Lanilis

      Yes, the agreement was 36 business hours. Good catch, I updated the wiki accordingly.

  3. Adam Krysiak

    I walked through few recent pull requests doing review and I see things that are done and they shouldn't be to keep quality of code on as high level as possible.

    • A lot of commented code. In case we see something is not important we should remove it, not comment it out. This causes with big and long files with no value.
    • Tests made only for code coverage. There are testcases with no assertions or with assertion on getter just after calling setter. This kind of tests are worser than no tests. It gives us false confidence that new changes are not destroying old logic. Cause everything is covered and green so it should work.
    • Injection of fields instead of injecting through constructor. It's much easier to test code that is injected through constructor and tests are fundamental for such a big project.
    • A lot of nested conditions and try-catch blocks what also complicates the code. 
    • Really long functions looking looking  like procedures. We should remember that JAVA is OO not procedural language. Methods is not something bad they have name which nicely set makes our code much more readable.
    • Usage of names that are not self descriptive.

    I thing every one should follow first 3 points. Rest is rather recommendation how to keep code readable and easy to maintain. Of course I'm aware of fact that we are not able to change everything instantly. I just want to improve quality of new code and encourage everyone to do code review even if you don't know component. Let's remember that it's an open source project and quality is one of most important factor when someone decides to join or use it.


    1. Jakub Zieba

      Adam Krysiak First and last three points should be checked in https://sonar.onap.org.