Bug 1425708 - Document advice for working on Marionette. r?whimboo,maja_zf,automatedtester draft
authorAndreas Tolfsen <ato@sny.no>
Mon, 18 Dec 2017 17:16:29 +0000
changeset 713111 d8cb2067dff5e32d8bea43beeebb317fb60c63c0
parent 713110 47c61d02fa29c92a57ba7cba02ba9c586b7fc2d1
child 744239 0c76b0c4db9cfbe7f6af163a418fc1bf1bed2c2c
push id93540
push userbmo:ato@sny.no
push dateTue, 19 Dec 2017 15:46:13 +0000
reviewerswhimboo, maja_zf, automatedtester
bugs1425708
milestone59.0a1
Bug 1425708 - Document advice for working on Marionette. r?whimboo,maja_zf,automatedtester There are many gotchas to be aware of when working on Marionette source code. It would be a good idea to collect this in a single document that could serve as documentation in onboarding new developers. DONTBUILD MozReview-Commit-ID: DuVqckA7wLP
testing/marionette/doc/CodeStyle.md
new file mode 100644
--- /dev/null
+++ b/testing/marionette/doc/CodeStyle.md
@@ -0,0 +1,261 @@
+Style guide for source code
+===========================
+
+Like other projects, we also have some guidelines to keep to the code.
+For the overall Marionette project, a few rough rules are:
+
+  * Make your code readable and sensible, and don’t try to be
+    clever.  Prefer simple and easy solutions over more convoluted
+    and foreign syntax.
+
+  * Fixing style violations whilst working on a real change as a
+    preparatory clean-up step is good, but otherwise avoid useless
+    code churn for the sake of conforming to the style guide.
+
+  * Code is mutable and not written in stone.  Nothing that
+    is checked in is sacred and we encourage change to make
+    testing/marionette a pleasant ecosystem to work in.
+
+
+JavaScript
+----------
+
+Marionette is written in [XPCOM] flavoured JavaScript and ships
+as part of Firefox.  We have access to all the latest ECMAScript
+features currently in development, usually before it ships in the
+wild and we try to make use of new features when appropriate,
+especially when they move us off legacy internal replacements
+(such as Promise.jsm and Task.jsm).
+
+One of the peculiarities of working on JavaScript code that ships as
+part of a runtime platform is, that unlike in a regular web document,
+we share a single global state with the rest of Firefox.  This means
+we have to be responsible and not leak resources unnecessarily.
+
+JS code in Gecko is organised into _modules_ carrying _.js_ or _.jsm_
+file extensions.  Depending on the area of Gecko you’re working on,
+you may find they have different techniques for exporting symbols,
+varying indentation and code style, as well as varying linting
+requirements.
+
+To export symbols to other Marionette modules, remember to assign
+your exported symbols to the shared global `this`:
+
+	this.EXPORTED_SYMBOLS = ["PollPromise", "TimedPromise"];
+
+When importing symbols in Marionette code, try to be specific about
+what you need:
+
+	const {TimedPromise} = Cu.import("chrome://marionette/content/sync.js", {});
+
+The [linter] will complain if you import a named symbol that is
+not in use.  If however you _need_ to import every symbol, you can:
+
+	const wait = {};
+	Cu.import("chrome://marionette/content/sync.js", wait);
+
+	wait.sleep(42);
+	await wait.TimedPromise(…);
+
+We prefer object assignment shorthands when redefining names,
+for example when you use functionality from the `Components` global:
+
+	const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
+
+When using symbols by their own name, the assignment name can be
+omitted:
+
+	const {TYPE_ONE_SHOT, TYPE_REPEATING_SLACK} = Ci.nsITimer;
+
+In addition to the default [Mozilla eslint rules], we have [our
+own specialisations] that are stricter and enforce more security.
+A few notable examples are that we disallow fallthrough `case`
+statements unless they are explicitly grouped together:
+
+	switch (x) {
+	  case "foo":
+	    doSomething();
+
+	  case "bar":  // <-- disallowed!
+	    doSomethingElse();
+	    break;
+
+	  case "baz":
+	  case "bah":  // <-- allowed (-:
+	    doCrazyThings();
+	}
+
+We disallow the use of `var`, for which we always prefer `let` and
+`const` as replacements.  Do be aware that `const` does not mean
+that the variable is immutable: just that it cannot be reassigned.
+We require all lines to end with semicolons, disallow construction
+of plain `new Object()`, require variable names to be camel-cased,
+and complain about unused variables.
+
+For purely aesthetic reasons we indent our code with two spaces,
+which includes switch-statement `case`s, and limit the maximum
+line length to 78 columns.  When you need to wrap a statement to
+the next line, the second line is indented with four spaces, like this:
+
+	throw new TypeError(
+	    "Expected an element or WindowProxy, " +
+	    pprint`got: ${el}`);
+
+This is not normally something you have to think to deeply about as
+it is enforced by the [linter].  The linter also has an automatic
+mode that fixes and formats certain classes of style violations.
+
+If you find yourself struggling to fit a long statement on one line,
+this is usually an indication that it is too long and should be
+split into multiple lines.  This is also a helpful tip to make the
+code easier to read.  Assigning transitive values to descriptive
+variable names can serve as self-documentation:
+
+	let location = event.target.documentURI || event.target.location.href;
+	log.debug(`Received DOM event ${event.type} for ${location}`);
+
+On the topic of variable naming the opinions are as many as programmers
+writing code, but it is often helpful to keep the input and output
+arguments to functions descriptive (longer), and let transitive
+internal values to be described more succinctly:
+
+	/** Prettifies instance of Error and its stacktrace to a string. */
+	function stringify(error) {
+	  try {
+	    let s = error.toString();
+	    if ("stack" in error) {
+	      s += "\n" + error.stack;
+	    }
+	    return s;
+	  } catch (e) {
+	    return "<unprintable error>";
+	  }
+	}
+
+When we can, we try to extract the relevant object properties in
+the arguments to an event handler or a function:
+
+	const responseListener = ({name, target, json, data}) => { … };
+
+Instead of:
+
+	const responseListener = msg => {
+	  let name = msg.name;
+	  let target = msg.target;
+	  let json = msg.json;
+	  let data = msg.data;
+	  …
+	};
+
+All source files should have `"use strict";` as the first directive
+so that the file is parsed in [strict mode].
+
+Every source code file that ships as part of the Firefox bundle
+must also have the following copying header:
+
+	/* This Source Code Form is subject to the terms of the Mozilla Public
+	 * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+	 * You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+New xpcshell test files _should not_ have a license header as all
+new Mozilla tests should be in the [public domain] so that they can
+easily be shared with other browser vendors.  We want to re-license
+existing tests covered by the [MPL] so that they can be shared.
+We very much welcome your help in doing version control archeology
+to make this happen!
+
+The practical details of working on the Marionette code is outlined
+in [CONTRIBUTING.md], but generally you do not have to re-build
+Firefox when changing code.  Any change to testing/marionette/*.js
+will be picked up on restarting Firefox.  The only notable exception
+is testing/marionette/components/marionette.js, which does require
+a re-build.
+
+[XPCOM]: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM
+[strict mode]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Strict_mode
+[our own specialisations]: https://searchfox.org/mozilla-central/source/testing/marionette/.eslintrc.js
+[linter]: #linting
+[public domain]: https://creativecommons.org/publicdomain/zero/1.0/
+[MPL]: https://www.mozilla.org/en-US/MPL/2.0/
+[CONTRIBUTING.md]: ../CONTRIBUTING.md
+
+
+Python
+------
+
+TODO
+
+
+Documentation
+-------------
+
+We keep our documentation in-tree under [testing/marionette/doc]
+and [testing/geckodriver/doc].  Updates and minor changes to
+documentation should ideally not be scrutinised to the same degree
+as code changes to encourage frequent updates so that the documentation
+does not go stale.  To that end, documentation changes with `r=me`
+from module peers are permitted.
+
+Use fmt(1) or an equivalent editor specific mechanism (such as Meta-Q
+in Emacs) to format paragraphs at a maximum width of 75 columns
+with a goal of roughly 65.  This is equivalent to `fmt -w 75 -g 65`,
+which happens to be the default on BSD and macOS.
+
+We endeavour to document all _public APIs_ of the Marionette component.
+These include public functions—or command implementations—on
+the `GeckoDriver` class, as well as all exported symbols from
+other modules.  Documentation for non-exported symbols is not required.
+
+The API documentation can be regenerated to [testing/marionette/doc/api]
+so:
+
+	% ./mach marionette doc
+
+You can also serve the documentation in an HTTP server if you work
+remotely on another terminal:
+
+	% ./mach marionette doc --http 0.0.0.0:6060
+	serving at 0.0.0.0:6060
+
+Normally there is no need to regenerate the API documentation after you
+make a change.  ato’s [mnapigen] script runs regularly in a cron job.
+
+[Mozilla eslint rules]: ../../../.eslintrc.js
+[mnapigen]: https://git.sny.no/rc/tree/bin/mnapigen
+[testing/geckodriver/doc]: ../../geckodriver/doc
+[testing/marionette/doc/api]: api/
+[testing/marionette/doc]: .
+
+
+Linting
+-------
+
+Marionette consists mostly of JavaScript (server) and Python (client,
+harness, test runner) code.  We lint our code with [mozlint],
+which harmonises the output from [eslint] and [flake8].
+
+To run the linter with a sensible output:
+
+	% ./mach lint -funix testing/marionette
+
+For certain classes of style violations the eslint linter has
+an automatic mode for fixing and formatting your code.  This is
+particularly useful to keep to whitespace and indentation rules:
+
+	% ./mach eslint --fix testing/marionette
+
+The linter is also run as a try job (shorthand `ES`) which means
+any style violations will automatically block a patch from landing
+(if using Autoland) or cause your changeset to be backed out (if
+landing directly on mozilla-inbound).
+
+If you use git(1) you can [enable automatic linting] before you push
+to a remote through a pre-push (or pre-commit) hook.  This will
+run the linters on the changed files before a push and abort if
+there are any problems.  This is convenient for avoiding a try run
+failing due to a stupid linting issue.
+
+[mozlint]: https://firefox-source-docs.mozilla.org/tools/lint/usage.html
+[eslint]: https://eslint.org/
+[flake8]: http://flake8.pycqa.org/en/latest/
+[enable automatic linting]: https://firefox-source-docs.mozilla.org/tools/lint/usage.html#using-a-vcs-hook