Changes between Initial Version and Version 1 of ReviewingPatches


Ignore:
Timestamp:
Jan 23, 2017, 1:04:04 PM (7 years ago)
Author:
Itms
Comment:

New review guidelines

Legend:

Unmodified
Added
Removed
Modified
  • ReviewingPatches

    v1 v1  
     1= Review guidelines for code in 0 A.D. =
     2
     3This document is a review guidelines with two aims:
     4* Having a common and transparent way to review patches and changes from everyone (both contributors and team members), so the code quality stays as high as possible, without blocking the evolution of the game.
     5* Giving contributors the opportunity to become a part of the review process.
     6
     7It relies on our code-review tool [wiki:Phabricator] (you are strongly encouraged to read the wiki page).
     8
     9== Being a reviewer ==
     10
     11Please perform your reviews and put all your comments on Phabricator. A review over IRC can sometimes be quicker, but it is then lost in the logs and useless.
     12
     13Please ignore all new revisions that were not built successfully by Jenkins yet. It's a waste of time to apply a patch and compile it, just to find an issue that can be found automatically. You can of course give hints to newcomers, should the build fail.
     14
     15If you are assigned a revision for review, you should perform that review as soon as you can. That means there is no pressure on you to do it fast, but nothing will come out of that revision proposal if you don't take care of it.
     16Instead of letting the patch rot, you are free to resign as reviewer for a revision, but in that case, you should find another suitable reviewer. That means either somebody with knowledge of the code, either somebody who agrees to take the task after you make sure they have the basic knowledge to do the review (and learn to know that code better).
     17
     18A reviewer does not write any code. If you want to modify a contributor's patch, you're the author of the new version and you need a review from somebody else. You can use the "Commandeer Revision" action on Phabricator, which makes you the author, and automatically makes the former author a reviewer.
     19
     20== What makes a review ==
     21
     22Reviewing is:
     23- Checking the fix or the feature proposed actually works. If applicable, read the corresponding Trac ticket, and check that the proposal takes into account whatever discussion happened there. First check you can reproduce the issue without the patch, then test the basic situation, then try to find edge cases that the patch could miss;
     24- Making sure the issue is as covered as possible by automated tests. Request new unit tests from the author whenever possible (see Automated testing). For a bug fix, add a test detecting that bug. For a feature, make sure there is a basic testing of the feature, covering the most common edge cases. No need to be too extensive, if subtle bugs are detected, we will add tests for them over time. Unit tests are not the cherry on top of a commit, it's a blocking item on your review checklist;
     25- Reading the code carefully in order to find design flaws or subtle issues. Of course you might miss things, but '''never be afraid of reviewing'''! There is no "bad" review, if a bug slips through, it will be caught later and you will learn something. The more you review, the better you get at it. If you find no issue at all but feel like your review is incomplete, you can also just drop a comment. That doesn't "accept" the revision or anything, it's just a note that you spent some time testing without finding an issue. The reviewers can then focus on things you didn't test thoroughly, and the contributor is happy their patch seems to work;
     26- Posting your entire review on Phabricator. If you miss this last step, your review is useless. A review over IRC did not happen (a lot of possible reviewers won't read it and there is no way to make sure all your comments are addressed).
     27
     28=== No review needed ===
     29In some very specific situations, if you are a team member, you can commit things without review through Phabricator, in order to avoid inefficiency. You can also use the Paste tool in Phabricator to share snippets and get comments without going through the complete review process (for instance if you need a comment from a native English speaker about a string).
     30
     31The known situations where a review is not needed are:
     32* Obvious string typos (typically a `an` to `a` fix, missing plurals, faulty grammar, etc)
     33
     34If you think something should be added here, please discuss it in the team forums.
     35
     36== Committing and what happens next ==
     37
     38You can commit a revision that is "accepted" if you are the author, or if the author is not a team member and you are one of the reviewers; and if the build tests completed successfully. However, don't jump on the commit button as soon as a revision is accepted. Somebody outside the list of reviewers (especially if the list has length 1) might notice something and request changes. Just let the "accepted" patch on standby for a bit, commit it next time you're around.
     39
     40Don't forget to close the revision and the Trac ticket if any.
     41
     42After the commit, if it breaks something, it needs an Audit. Comment on the commit inside Diffusion in Phabricator, and use the "Raise Concern" option. The committer will then get a notification. However, it is not always their responsibility to write a fix, but the author's. There are two possible situations:
     43- The author committed themselves. In that case it's their responsibility to fix it, as Phabricator will remind them. Reviewers are not directly concerned, but they are likely to review the fix since they know the code.
     44- A reviewer committed because the patch was from an occasional contributor. In that case they will be notified by Phabricator, and they are responsible for getting it fixed, i.e. start by contacting the author and make them fix it. This prevents them from wasting their time writing a patch themselves, and it allows us to see whether the contributor is willing to take care of the issues they introduced. If the author doesn't try to fix anything, the committer should become the author of a fix. If somebody else wants to handle it, the original committer stays responsible for getting the fix in.
     45
     46== Automated testing ==
     47
     48The review guidelines above rely on an efficient way of testing patches and commits. Testing should be as automated as possible in order to free time and brain capacity to find design flaws and other subtle issues in a piece of code.
     49
     50The following things can be automatically tested on patches by Jenkins, and can also be run post-commits. Some of them are already tested, others should in the future. For every item it would be nice to use as many alternative pieces of software as possible. This is a work-in-progress list and suggestions are welcome.
     51 - Building (in release and debug mode) on GCC, clang and MSVC
     52 - Unit tests (run in release and debug mode) on Unix and Windows
     53 - Perl scripts for art files and entity templates
     54 - Linting for enforcing our [wiki:Coding_Conventions] automatically
     55 - Using SpiderMonkey tools for more analysis of the JS code
     56 - Running a game with AIs and catching warnings and errors
     57 - When #3392 is implemented, generating a commands file from AIs and run serialization tests on it
     58 - Fuzzing our file reading, input parsing, (de)serialization code
     59 - Look for memory leakage during a game by using the command file from AIs
     60 - Static analysis of the engine code
     61
     62For more information on the current setup, read the page [wiki:JenkinsSetup].
     63
     64A lot of things cannot be tested automatically right now, but if you notice that a common class of bugs is never caught by our workflow and creates "concerning" commits, we should think about an automated way of blocking them.