Code review guidelines

See the Code_Review documentation on how to be an effective code reviewer, and to how to prepare patches effectively as an author.
Also see Eric Rose's presentation on constructive code review https://air.mozilla.org/erik-rose-constructive-code-review/
For authors:
- If possible, please include a try run with your patches for review
- If you are working on a high priority bug, please include your work in progress patches on the bug at the end of your workday so that people in other timezones can pick work on it during their day if required.

Taskcluster

Testing changes to the decision task

From http://gecko.readthedocs.io/en/latest/taskcluster/taskcluster/taskgraph.html

"The decision task is the first task created when a new graph begins. It is responsible for creating the rest of the task graph.

The decision task for pushes is defined in-tree, in .taskcluster.yml. That task description invokes mach taskcluster decision with some metadata about the push. That mach command determines the optimized task graph, then calls the TaskCluster API to create the tasks.

Note that this mach command is not designed to be invoked directly by humans. Instead, use the mach commands described below, supplying parameters.yml from a recent decision task. These commands allow testing everything the decision task does except the command-line processing and the queue.createTask calls."

Taskcluster creates a large graph of tasks, some which depend on each other. Each task has different properties. Depending on the code tree that the task runs on, different tasks may run. This decision task will vary depending on the code tree it is running on. For every push to a code tree, a Gecko Decision Task runs which generates several files associated with it, include parameters.yml which includes metadata for that push.

Comparing the list of tasks after a patch is applied

You can download the parameters.yml file associated with a push on a particular tree. This file can be used locally with the mach taskgraph command.
Documentation on using mach taskgraph http://gecko.readthedocs.io/en/latest/taskcluster/taskcluster/taskgraph.html

For example, if you have a local clone of a repo, in this case mozilla-beta, you can output the list of tasks to a file. Then apply your patch, output the list of tasks to a file, and then diff to ensure your patch resulted in the expected changes. In this case, the patch is to enable talos jobs for linux64 devedition builds.

Kims-MacBook-Pro:mozilla-beta kmoir$ pwd
/Users/kmoir/hg/mozilla-beta
Kims-MacBook-Pro:mozilla-beta kmoir$ ./mach taskgraph target -p ~/Downloads/parameters-m-b.yml &> old
apply patch
Kims-MacBook-Pro:mozilla-beta kmoir$ hg diff
diff --git a/taskcluster/ci/test/test-platforms.yml b/taskcluster/ci/test/test-platforms.yml
--- a/taskcluster/ci/test/test-platforms.yml
+++ b/taskcluster/ci/test/test-platforms.yml
@@ -71,16 +71,17 @@ linux64-pgo/opt:

 linux64-devedition/opt:
     build-platform: linux64-devedition-nightly/opt
     test-sets:
         - common-tests
         - web-platform-tests
         - opt-only-tests
         - desktop-screenshot-capture
+        - talos
         - awsy

 linux64-asan/opt:
     build-platform: linux64-asan/opt
     test-sets:
         - common-tests
 
 # Stylo builds only run a subset of tests for the moment. So give them
diff --git a/taskcluster/taskgraph/transforms/job/mozharness_test.py b/taskcluster/taskgraph/transforms/job/mozharness_test.py
--- a/taskcluster/taskgraph/transforms/job/mozharness_test.py
+++ b/taskcluster/taskgraph/transforms/job/mozharness_test.py
@@ -22,16 +22,17 @@ ARTIFACTS = [
     ("public/test", "artifacts/"),
     ("public/test_info/", "build/blobber_upload_dir/"),
 ]

 BUILDER_NAME_PREFIX = {
     'linux64-pgo': 'Ubuntu VM 12.04 x64',
     'linux64': 'Ubuntu VM 12.04 x64',
     'linux64-nightly': 'Ubuntu VM 12.04 x64', 
+    'linux64-devedition-nightly': 'Ubuntu VM 12.04 x64',
     'linux64-asan': 'Ubuntu ASAN VM 12.04 x64',
     'linux64-ccov': 'Ubuntu Code Coverage VM 12.04 x64',
     'linux64-jsdcov': 'Ubuntu Code Coverage VM 12.04 x64',
     'linux64-stylo': 'Ubuntu VM 12.04 x64',
     'macosx64': 'Rev7 MacOSX Yosemite 10.10.5',
     'android-4.3-arm7-api-15': 'Android 4.3 armv7 API 15+',
     'android-4.2-x86': 'Android 4.2 x86 Emulator',
     'android-4.3-arm7-api-15-gradle': 'Android 4.3 armv7 API 15+',
Kims-MacBook-Pro:mozilla-beta kmoir$ ./mach taskgraph target -p ~/Downloads/parameters-m-b.yml &> new
Kims-MacBook-Pro:mozilla-beta kmoir$ diff old new
....
> test-linux64-devedition/opt-talos-chrome
> test-linux64-devedition/opt-talos-chrome-e10s
> test-linux64-devedition/opt-talos-dromaeojs
> test-linux64-devedition/opt-talos-dromaeojs-e10s
> test-linux64-devedition/opt-talos-g1
> test-linux64-devedition/opt-talos-g1-e10s
> test-linux64-devedition/opt-talos-g2
> test-linux64-devedition/opt-talos-g2-e10s
> test-linux64-devedition/opt-talos-g3
> test-linux64-devedition/opt-talos-g3-e10s
> test-linux64-devedition/opt-talos-g4
> test-linux64-devedition/opt-talos-g4-e10s
> test-linux64-devedition/opt-talos-other
> test-linux64-devedition/opt-talos-other-e10s
> test-linux64-devedition/opt-talos-svgr
> test-linux64-devedition/opt-talos-svgr-e10s
> test-linux64-devedition/opt-talos-tp5o
> test-linux64-devedition/opt-talos-tp5o-e10s

The diff indicates that new talos jobs are added. You should include a diff like this when asking for review of your taskcluster patches on the bug or pull request.

Note: You can also specify the branch of the tree instead of downloading the paramaters.yml - see Ability to use latest parameters.yml file from given tree (https://bugzilla.mozilla.org/show_bug.cgi?id=1378212)

Generating the decision task locally in JSON

You can also output the JSON of the taskgraph that will be generated. This is a useful troubleshooting tool when you are changing the dependencies of the tasks within a graph, or the payload

./mach taskgraph target  --json -p ~/Downloads/parameters-m-b.yml > task.json
more task.json
{
  "android-api-15-gradle-dependencies": {
    "attributes": {
      "build_platform": "android-api-15-gradle-dependencies",
      "build_type": "opt",
      "kind": "android-stuff",
      "run_on_projects": [
        "all"
      ]
    },
    "dependencies": {},
    "kind_implementation": "taskgraph.task.transform:TransformTask",
    "label": "android-api-15-gradle-dependencies",
    "task": {
      "created": {
        "relative-datestamp": "0 seconds"
      },
      "deadline": {
        "relative-datestamp": "1 day"
      },
      "expires": {
        "relative-datestamp": "1 year"
      },
.....

Changing the target

You can also modify the properties of your parameters.yml file to reflect a different target then the default one to look at the jobs generated for this target task. Target tasks are defined in tree here taskcluster/taskgraph/target_tasks.py. For instance, here I change the default target to nightly_linux from the default push to see the tasks generated for that target.

Kims-MacBook-Pro:mozilla-central kmoir$ cat ~/Downloads/parameters-m-c.yml 
base_repository: https://hg.mozilla.org/mozilla-central
build_date: 1494018407
filters:
- check_servo
- target_tasks_method
head_ref: ff83fde8be946eabcf27ea97d4676f601c122194
head_repository: https://hg.mozilla.org/mozilla-central/
head_rev: ff83fde8be946eabcf27ea97d4676f601c122194
include_nightly: false
level: '3'
message: ' '
moz_build_date: '20170505210647'
optimize_target_tasks: true
owner: kwierso@gmail.com
project: mozilla-central
pushdate: 1494018407
pushlog_id: '31771'
#target_tasks_method: default
target_tasks_method: nightly_linux
triggered_by: push

Kims-MacBook-Pro:mozilla-central kmoir$ ./mach taskgraph target -p ~/Downloads/parameters-m-c.yml &> default

Buildbot

Setup buildbot environment

In your localhost use this script, it magically does everything for you.

List builders added/removed

You can read the blog post in here.

Download your patch and run it against list_builder_differences.sh. It will create a clean buildbot environment under $HOME/.mozilla/releng and determine what your patch adds/removes.

NOTE: This only works with buildbot-configs patches.

hg clone http://hg.mozilla.org/build/braindump
braindump/community/list_builder_differences.sh -j path_to_patch.diff

vimdiff

Did you know vim has a diff mode? Try running vimdiff file1 file2

Most of the suggestions below combine nicely with vimdiff, or some other diff utility. Get a dump of the output prior to your changes, and then a dump of the output after your changes and run them through vimdiff or your favourite diff utility. Since the diff lines can be lengthy, "wdiff -3" can be helpful to hilight not just what lines changed, but exactly what changed on each line.

test-masters.sh

buildbot-configs includes a script called test-masters.sh. It will iterate through the list of buildbot master configurations and run buildbot checkconfig on each.

NOTE: You want your PYTHONPATH to point to the path that contains your buildbotcustom checkout and lib/python inside of the tools repo. For example:

export PYTHONPATH=$PYTHONPATH:~/repos:~/repos/tools/lib/python

where ~/repos is where buildbotcustom and tools are checked out.

Run the tests like this:

./test-masters.sh

If you don't have buildbot on your path, then you should do

./test-masters.sh -b /path/to/buildbot

eg .../bin/buildbot in a master you already have set up (see below).

Please run this before asking for review, or landing changes!

config.py is executable!

You can run config.py in the mozilla/ or mozilla-tests/ directories and have it spit out the complete configuration for all branches. e.g.

ln -sf production_config localconfig.py
python config.py > orig.txt
hg qpush # Apply my awesome changes
python config.py > new.txt
vimdiff orig.txt new.txt

setup one master and output the steps for it

cd ~/repos/buildbot-confgs
# run ./setup-master.py --list to see the list of masters
rm -rf master_dir; mkdir master_dir; python setup-master.py master_dir pm01-builder
cd master_dir
python ~/repos/releng/braindump/buildbot-related/dump_master.py master.cfg > old
# apply your changes
python ~/repos/releng/braindump/buildbot-related/dump_master.py master.cfg > new
# compare old and new

and here is the changes I do to brainddump:

diff --git a/buildbot-related/dump_master.py b/buildbot-related/dump_master.py
--- a/buildbot-related/dump_master.py
+++ b/buildbot-related/dump_master.py
@@ -31,4 +31,5 @@ print "Builders:"
 for b in g['c']['builders']:
     print b['name'], b['factory'].__class__.__name__
+'''
     for s in b['factory'].steps:
         step_class, args = s
@@ -65,2 +66,3 @@ for s in g['c']['status']:
             d[a] = getattr(s, a)
     print format_objs(s, d)
+'''

builder_list.py / dump_master.py

Our braindump repo has a few useful scripts to help with testing: builder_list.py and dump_master.py.

builder_list.py will take a buildbot master.cfg file as input and output the list of all builders with steps to stdout.

For example:

export PYTHONPATH=/builds/buildbot/kmoir/test2:/builds/buildbot/kmoir/tools/lib/python:/builds/buildbot/kmoir/test2:/builds/buildbot/kmoir/tools/lib/python
source ./bin/activate
/builds/buildbot/kmoir/test2/braindump/buildbot-related/builder_list.py master/master.cfg > new
hg up -C
/builds/buildbot/kmoir/test2/braindump/buildbot-related/builder_list.py master/master.cfg > old
diff -pU 8 old new > differences

dump_master.py will take a buildbot master.cfg file as input and output the list of all builders with steps, all change sources, all schedulers, and all status plugins to stdout.

To be really thorough, you can run dump_masters.sh, which is like test-masters.sh, except instead of doing just a buildbot checkconfig, it runs dump_master.py on each master configuration. This can take a while to run (over a minute), but is a great way to make sure your changes don't have unintended side-effects.

This is an example on how to use it:

cd ~/repos/buildbot-configs
~/repos/releng/braindump/buildbot-related/dump_masters.sh > new
hg up -C
~/repos/releng/braindump/buildbot-related/dump_masters.sh > old
diff -pU 8 old new > differences
# or
vimdiff old new

Be not afraid!

... of hacking things to make testing easier. Here's an example of a simple script to output the command for certain status objects. I used this to make sure that my patch was only changing the command for a subset of the status objects.

import re

# Load the buildbot master config
execfile("master.cfg")

for s in c['status']:
    # We only care about status plugins with a 'command' attribute here
    if hasattr(s, 'command'):
        # Strip out 'instance at 0x12345678', it changes between runs
        s = "%s %s %s" % (s, s.command, s.builders)
        s = re.sub("instance at 0x[0-9a-f]+", "", s)
        print s

ReleaseEngineering:TestingTechniques

Contents