Review guidelines for code in 0 A.D.

This document is a review guidelines with two aims:

  • 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.
  • Giving contributors the opportunity to become a part of the review process.

It relies on our code-review tool Phabricator (you are strongly encouraged to read the wiki page).

Being a reviewer

Please 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.

Please 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.

If 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. Instead 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).

A 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.

What makes a review

Reviewing is:

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

No review needed

In 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).

The known situations where a review is not needed are:

  • Obvious string typos (typically a an to a fix, missing plurals, faulty grammar, etc)

If you think something should be added here, please discuss it in the team forums.

Committing and what happens next

You 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.

Don't forget to close the revision and the Trac ticket if any.

After 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:

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

Automated testing

The 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.

The 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.

  • Building (in release and debug mode) on GCC, clang and MSVC
  • Unit tests (run in release and debug mode) on Unix and Windows
  • Perl scripts for art files and entity templates
  • Linting for enforcing our Coding_Conventions automatically
  • Using SpiderMonkey tools for more analysis of the JS code
  • Running a game with AIs and catching warnings and errors
  • When #3392 is implemented, generating a commands file from AIs and run serialization tests on it
  • Fuzzing our file reading, input parsing, (de)serialization code
  • Look for memory leakage during a game by using the command file from AIs
  • Static analysis of the engine code

For more information on the current setup, read the page JenkinsSetup.

A 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.

Last modified 9 months ago Last modified on Jan 23, 2017, 1:04:04 PM