QA/Mozmill Test Automation/Test Modules Refactor

Overview

Lead:	Anthony Hughes
Co-workers:	TBD
Dates:	TBD
Status:	In process of determining style guidelines
Repository Location:	TBD
Tracking Bug(s)	bug 604700
Documentation:	Draft Proposal Neil Rashbrook's style guide Douglas Crockford's style guide

Project Details

Summary

The purpose of this project is to ensure all tests are implemented using an agreed style guideline. The workflow of this project will mostly be parallel with the Shared Modules Refactor project. This project will be rolled out in multiple phases.

Style Guidelines

Draft

1. Local variables should be named using camel-case

 var exampleVariableName = value;

2. Constants should be named using all-caps

const EXAMPLE_CONSTANT = value;

3. Blocks of code should be commented in-line:

  // What the code does
  some code;

4. Functions should use JSDoc block-style comments:

  /**
   * Purpose of the function
   *
   * @param {type} paramName
   *        Purpose of the parameter
   */
   function someFunction(someParam) {

5. Arrays block-style

  var obj = [
    { child: ‘value’,
      child: ‘value }
    { child: ‘value’,
      child: ‘value}
  ];

6. Named Functions

  function someFunction() {
    some code;
  }

• Geo: Only top-level and object funtions should be named
• Geo: Agree on open-brace on same line but common in JS to have an next-line exception for named functions; could go either way.

7. Anonymous Functions

  var someFunction() = function() {
    some code;
  }

• Henrik: We should completely avoid anonymous functions

8. Function Usage

  var someVariable = someFunction(
                       param1,
                       param2,
                       paramN,
                     );

• Henrik: Indent by 2 vs. aligned by opening bracket

9. Function Parameter Concatenation

  var someVariabe = someFunction(param1,
                      'some' +
                      'concat' +
                      'string'
                    );

• Henrik: Same here

10. waitFor() block-style

  controller.waitFor(function() {
    return something == somethingElse;
  }, "Some error message", TIMEOUT);

• Henrik: We should check how we want to handle long message strings

11. XPath: split on '/'

 var path = "/something" +
            "/something" + 
            "/something";

12. Lines of code should be indented 2-spaces to the right of their parent

var cancelButton = new elementslib.Lookup(controller.window.document,
                     'path/to/element'
                   );

• Geo: This keeps it mostly aligned with the actual structure. When we pull it back to the 0-column, makes things that are variables decls look like flow control structures instead, and I really don’t like that.
• Henrik: Inside function calls we should probably align with the opening bracket.

13. Component declarations should be indented in line with the parent

var svc = Cc["string/for/service/component"].
          getService(Ci.nsINameOfInterface);

14. Lines of code should not exceed 80 characters

15. Use waitFor() as much as possible. Only use sleep() when a waitFor() fails.

• Geo: If you’re only saving a few milliseconds and not causing a robustness issue, you should use sleep for simplicity’s sake.

16. Always use a discrete int value for any DELAYs

17. Always use a global constant for any TIMEOUTs

const TIMEOUT = 5000;
 ...
controller.waitFor(function() {
  return something == something;
} "Some message", TIMEOUT);

• Henrik: A timeout like 5s doens't have be specified anymore - it's the default now.
• Geo: Use default params where appropriate, wrapper functions where not

18. Use assertDOMProperty(object, property) when evaluating DOM Object Properties

• Geo: Not necessarily needs definition in style guide; “Use the most appropriate function” can be a common sense standard.

19. Use assertJSProperty(object, property) when evaluating JS Object Properties

• Geo: Not necessarily needs definition in style guide; “Use the most appropriate function” can be a common sense standard.

20. Use array.forEach() for iterating array elements

array.forEach(function(elem, [index, [array]]) {
  statements;   
}, [thisObject]);

• Geo: Documentation should call out that thisObject is the fourth parameter
• Henrik: index and array are optional but can be helpful in some cases when you need the current index or other elements from the array.

21. Don't put tests in testGeneral (ie. no "misc" tests)

• Geo: Sometimes a “misc” folder makes sense. We just might be leaning on it too hard. Agree we should review the organization.

22. Only one test per file

• Geo: Basically OK with that. This means we’ll be managing thousands of files eventually. However, unit test “common setup” paradigm doesn’t work that well with UI tests, I’m cool subverting it with 1:1.

23. Element before action

var obj = new elementGetter(params);
controller.action(obj, params);

• Geo: Element getting will be considerably simpler post-refactor and won’t require a long line.

24. Local test pages encapsulated in Array

const LOCAL_TEST_PAGES = [
 { id: “Some_ID”,
   url: LOCAL_TEST_FOLDER + ‘someFolder/someFile.html’ },
 { id: “Some_ID”,
   url: LOCAL_TEST_FOLDER + ‘someFolder/someFile.html’ },
 { id: “Some_ID”,
   url: LOCAL_TEST_FOLDER + ‘someFolder/someFile.html’ }
];

• Henrik: We should use Name, Link, and XPath as well
• Aaron: Establish a module/API to handle loading of any needed test files

25. Constants between requires and setupModule()

// Include required modules
var addons = require("../../../shared-modules/addons");
var modalDialog = require("../../../shared-modules/modal-dialog");

const TIMEOUT = 5000;
const SEARCH_TIMEOUT = 30000;
const INSTALL_TIMEOUT = 30000;

var setupModule = function() {

• Geo: All unchanging variables as constants
• Henrik: This space should not be strictly reserved for constants

26. Do not pass a module parameter in setupModule() and teardownModule()

• Geo: Strange to have a function not declare a parameter that’s actually being supplied, but maybe that’s more normal in JS.
• Henrik: Removing module was probably a mistake; used for injecting global functions into the module scope.

27. Use waitFor() when asserting for timeouts

const TIMEOUT = 5000;
 ...
controller.waitFor(function() {
  return something == something;
} "Some message", TIMEOUT);

• Geo: Not necessarily needs definition in style guide; “Use the most appropriate function” can be a common sense standard.

28. Use traditional for() when iterating characters of a string

29. License block

• Aaron: We should define a style guideline for the license block
• Henrik: Style guideline for what?

30. Things the style guide should cover

• Geo: naming/caps, commenting style, block style, indentation style, line length, local idioms (waitFor, assert), things we lock down because alternatives are risky (array.forEach)

31. waitFor() error messages

• Henrik: How do we describe the text for the message and how close do we want to stay with the way mochitests are doing it.

Vetted

The following is a breaking down of the above points into two buckets.

Agreed principles are those points which we all have agreed to unanimously.

Open principles are those points which we felt should be discussed further before including them in any official style guide.

Refactoring

Phase I

Goal: Implement lowest hanging fruit

Phase II

Goal: Bring tests in-line with Shared Module Refactor