Code Review Checklists

juju
1

Contents

Introduction

This document contains checklists to use when reviewing changes to Juju core. They have been synthesised from various documents written over Juju’s history as well as issues that have been raised on actual reviews. These checklists don’t cover every possible thing that a reviewer should be thinking of when looking at a proposed change. The focus is on the biggest wins that can be practically included in a checklist.

The hope is that the checklists will help to ensure a base level of quality and assist reviewers to be in the right frame of mind when evaluating pull requests.

These checklists are not set in stone. They will evolve as Juju and the team’s development practices evolve. Any significant changes to the checklists should be proposed via the juju-dev mailing list of the Juju tech board.

This document is being migrated from the old location. We’re also gardening and modernising the rules as we bring them across. Please transfer points that you think are valuable. If you think something needs more history or explanation, add a collapsible details section with more information (which might include quizzing someone else if there’s context you don’t know).

Using the checklists

  • For each pull request at least one reviewer must go through the checklists relevant to the pull request.
  • The reviewer should indicate that they have gone through the checklist(s). Any issues found via the checklist(s) should be raised on the review.
  • The “General” list applies to all pull requests.
  • There a checklists which apply to specific areas of the code which should be applied as applicable.

As a reviewer, a reasonable approach seems to be to read through a proposed change once, and then apply the checklists once you have it in your head. YMMV.

General

  • Does the pull request description justify the change?

  • If the pull request is a bug fix, does the description link to the relevant ticket?

  • Have QA steps been defined?

    • Or justification why external QA is not possible

  • Is all state non-global?

    • There should be no global variables/state.
    • Package level logging is a valid exception.

  • Are there unit tests with reasonable coverage?

  • Are the unit tests actually unit tests?

    • Wherever possible, unit tests should not involve functionality outside of the unit being tested.
    • JujuConnSuite should not be used for new unit tests.

  • Do the test exercise behaviour, not implementation?

    • Correct behaviour is far more important than how it is done.

  • Are tests isolated from the host machine’s environment?

    • Start with IsolationSuite as a base but know that it only addresses some isolation concerns.

  • Are all test suites registered with gocheck? (gc.Suite(...))

  • Do tests prefer AddCleanup over TearDownTest or TearDownSuite?

  • Are external dependencies passed in?

    • Use interfaces!
    • Patching in tests should be kept to a minimum.

  • Do structs, methods and funcs each have just one, clear job?

    • Sprawling, complex units of code are bug prone and hard to reason about.

  • Do all exported structs, fields, methods and funcs have docstrings?

  • Do new packages have documentation? (in doc.go)

    • These should explain why the package exists with references to the important parts.

  • Has the documentation for preexisting packages been updated to match the changes made?

  • Are non-obvious parts of the implementation explained with comments?

  • Are non-obvious parts of the implementation as clear as they could be?

    • Sometimes, restructuring the code or choosing better names for things can make the code much clearer.

  • Are inputs validated?

    • Don’t trust anyone or anything.

  • Have sensible names been used?

    • Say what something does, not how it works.
    • Don’t use names which may confuse or mislead.

  • Are channels used in favour of sharing memory wherever possible?

    • Primitives from the sync package should be used sparingly.
    • Sync package primitives such as Mutex are ok to protect state internal to a type but should never be exposed externally.
    • Combining mutexes and goroutines/channels makes code exceptionally difficult to reason about.

  • Are all created resources, cleaned up later?

    • There must be a something in place to clean up each API connection, mongo session, goroutine and worker.
    • If you start it, you must stop it.

  • Are all channel reads and writes abortable?

    • All channel reads and writes must be performed using a select which also involves some other channel which can be used to abort the read or write.
    • The only exception to this is in tests where it can be useful to write into a buffered channel.

  • Are channels private wherever possible?

    • <-chan struct{} channels which just get closed are probably fine.

  • Are interfaces as small as they can be?

    • Consider whether larger interfaces could be split up.

  • For code that deals with filenames, OS specific errors or low level operations, will it work on all platforms we care about?

    • Paths work differently on different platforms
    • Errors should not be examined for specific text as these can vary by platform.

  • Are errors being created and wrapped using the juju/errors package?

  • Is juju/clock.Clock used instead of time.Now or time.After?

  • Is there a useful and tasteful amount of logging?

    • Not too little; not too much

  • Have appropriate log levels been used for each logged message?

    • INFO and up for the user
    • DEBUG and down for developers
    • Use the right level within those ranges

state

apiserver

Example: https://github.com/juju/juju/tree/develop/apiserver/facades/client/sshclient

api

Example: https://github.com/juju/juju/tree/develop/api/migrationflag

worker

Example: https://github.com/juju/juju/tree/develop/worker/hostkeyreporter

CLI & hook tools

Example: