MozillaWiki

QA/Mozmill Test Automation/Test Modules Refactor: Difference between revisions

Page Discussion

QA/Mozmill Test Automation/Test Modules Refactor (view source)

Revision as of 21:14, 16 February 2011

5,149 bytes removed ,  16 February 2011
→‎Outstanding Items
 
(31 intermediate revisions by 3 users not shown)
Line 14: Line 14:
  |- valign="top"
  |- valign="top"
  | '''Status:'''
  | '''Status:'''
  | In process of determining style guidelines
  | '''on hold'''
  |- valign="top"
  |- valign="top"
  | '''Repository Location:'''
  | '''Repository Location:'''
Line 23: Line 23:
  |- valign="top"
  |- valign="top"
  | '''Documentation:'''
  | '''Documentation:'''
  | [https://docs.google.com/document/d/13OhM4T2nxYqrvCAtVicjB-PCrDeLiSOnc6N-pXw939A/edit?hl=en&authkey=CPmR7d4J Draft Proposal]
  |
* [https://docs.google.com/document/d/13OhM4T2nxYqrvCAtVicjB-PCrDeLiSOnc6N-pXw939A/edit?hl=en&authkey=CPmR7d4J Draft Proposal]
* [http://neil.rashbrook.org/JS.htm Neil Rashbrook's style guide]
* [http://javascript.crockford.com/code.html Douglas Crockford's style guide]
  |}
  |}


Line 31: Line 34:


== Style Guidelines ==
== Style Guidelines ==
; Timeouts & Delays
The [https://developer.mozilla.org/en/Mozmill_Tests/Mozmill_Style_Guide Mozmill Style Guide] v1.0 has been created. The following refactoring work is an effort to implement those guidelines throughout the mozmill-tests repository. This refactoring will be implemented in multiple phases, outlined below.
* Proposed Guidelines:
** Delay: Use discrete value
** Timeout: Use global TIMEOUT
* Henrik: Timeouts should be encapsulated within a global shared module
* Geo: Use default params where appropriate, wrapper functions where not
 
; Constants
* Proposed Guidelines:
** Exist between ''requires'' and ''setupModule()''
** const SOME_CONSTANT = value;
* Geo: All ''unchanging'' variables as constants
 
; Function Signatures
* Proposed Guidelines:
** Top-level functions should be named ie. function testDisableEnablePlugin() {
** All other functions should be anonymous ie. var nameOfFunction = function() {
** Opening brace should exist on the signature line
* 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.
 
; ''module'' as a param for setupModule & teardownModule
* Proposed Guidelines:
** ???
* 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.
 
; Mozilla Components
* Proposed Guidelines:
** var obj = Cc[“@mozilla.org/component;1”].getService(Ci.interface);
** Declare in setupModule()
** For word-wrapping, split after the ., getService() aligned with Cc[]
 
; LOCAL_TEST_PAGES
* Proposed Guidelines:
** Encapsulate all local test pages within a const LOCAL_TEST_PAGES array
** Each element (test page) consists of a URL and an ID
* Henrik: We should use Name, Link, and XPath as well
* Aaron: Establish a module/API to handle loading of any needed test files
 
; Array Formatting
* Proposed Guidelines:
** Each array element on its own line
** Each element member on its own line
<pre>
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’ }
];
</pre>
 
; Assertions
* Use waitFor() for all timeout assertions
<pre>
controller.waitFor(function() {
  return someObject.value == expectedValue;
}, TIMEOUT, 100, “My failure message”);
</pre>
* Use assertDOMProperty() for all DOM property assertions
<pre>controller.assertDOMProperty(someObject.someProperty, expectedValue);</pre>
* Use assertJSProperty() for all JS property assertions
<pre>controller.assertJSProperty(someObject.someProperty, expectedValue);</pre>
* Geo: Not necessarily needs definition in style guide; “Use the most appropriate function” can be a common sense standard.
 
; Element Before Action
* Proposed Guideline:
** Always declare an element before performing an action on it
** Example:
<pre>
var obj = new elementGetter(params);
controller.action(obj, params);
</pre>
* Geo: Element getting will be considerably simpler post-refactor and won’t require a long line.
 
; Iteration
* Proposed Guideline:
** Use array.forEach() for iterating arrays
** Use traditional for() for iterating strings
<pre>
array.forEach(function(elem, [index, [array]]) {
  statements; 
}, [thisObject]);
</pre>
* 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.
 
; Word-wrapping and Indentation
* Proposed Guideline:
** Adhere to an 80 character per line limitation
** In most cases, indentation should be 2-spaces from the parent
* 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.
Examples:
* Function call:
<pre>
controller.function(
  param1,
  param2,
  paramN
);
</pre>
* Variable from Function call:
<pre>
var obj = controller.method(
            param1,
            param2,
            paramN
          );
</pre>
* XPath:
<pre>
var obj = new elementslib.Lookup(
            controller.window.document,
            ‘/path’ +
            ‘/to’ +
            ‘/object’
          );
</pre>
* Arrays:
<pre>
var obj = [
  { child: ‘value’,
    child: ‘value }
  { child: ‘value’,
    child: ‘value}
];
</pre>
 
; sleep
* Proposed Guideline:
** Avoid use of sleep() where possible and use waitFor() instead
* Geo: If you’re only saving a few milliseconds and not causing a robustness issue, you should use sleep for simplicity’s sake.
 
; Misc Tests
* Proposed Guideline:
** No "general" or "misc" tests should exist ie. no firefox/testGeneral/
* Geo: Sometimes a “misc” folder makes sense.  We just might be leaning on it too hard.  Agree we should review the organization.
 
; One Test per File
* Proposed Guideline:
** 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.
 
; Commenting
* Proposed Guideline:
** Block-style for function headers
** JSDoc-style for additional helper functions
 
; Camel-case
* Proposed Guideline:
** Use camel-case for all non-constant variables and functions
<pre>
var thisIsAVariable = value;
var thisIsATestMethod = function() {
</pre>
 
== Notes on the Goals ==
* Geo: Style guide should offer guidelines for...
** naming/caps
** commenting style
** block style
** indentation style
** line length
** local idioms (waitFor, assert)
** things we lock down because alternatives are risky (array.forEach)
* Aaron: We should offer style guidelines on the license block
 
== Style Guidelines ==
=== Naming ===
1. Local variables should be named using camel-case
<pre> var exampleVariableName = value;</pre>
2. Constants should be named using all-caps
<pre>const EXAMPLE_CONSTANT = value;</pre>
 
=== Commenting ===
1. Blocks of code should be commented in-line:
<pre>
  // What the code does
  some code;
</pre>
 
2. Functions should use JSDoc block-style comments:
<pre>
  /**
  * Purpose of the function
  *
  * @param {type} paramName
  *        Purpose of the parameter
  */
  function someFunction(someParam) {
</pre>
 
=== Block-style ===
1. Arrays
<pre>
  var obj = [
    { child: ‘value’,
      child: ‘value }
    { child: ‘value’,
      child: ‘value}
  ];
</pre>
 
2. Named Functions
<pre>
  function someFunction() {
    some code;
  }
</pre>
 
3. Anonymous Functions
<pre>
  var someFunction() = function() {
    some code;
  }
</pre>
 
4. Function Usage
<pre>
  var someVariable = someFunction(
                      param1,
                      param2,
                      paramN,
                    );
</pre>
 
5. Function Parameter Concatenation
<pre>
  var someVariabe = someFunction(param1,
                      'some' +
                      'concat' +
                      'string'
                    );
</pre>
 
6. waitFor()
<pre>
  controller.waitFor(function() {
    return something == somethingElse;
  }, "Some error message", TIMEOUT);
</pre>
 
7. XPath: split on '/'
<pre>
var path = "/something" +
            "/something" +
            "/something";
</pre>
 
=== Indentation ===
1. Lines of code should be indented 2-spaces to the right of their parent
<pre>
var cancelButton = new elementslib.Lookup(controller.window.document,
                    'path/to/element'
                  );
</pre>
 
2. Component declarations should be indented in line with the parent
<pre>
var obj = Cc['someComponentInferface'].
          getService('someService');
</pre>
 
=== Line Length ===
1. Lines of code should not exceed 80 characters
 
=== Idioms ===
1. Use waitFor() as much as possible.  Only use sleep() when a waitFor() fails.
 
2. Always use a discrete int value for any DELAYs
 
3. Always use a global constant for any TIMEOUTs
<pre>
const TIMEOUT = 5000;
...
controller.waitFor(function() {
  return something == something;
} "Some message", TIMEOUT);
</pre>
 
4. Use assertDOMProperty(object, property) when evaluating DOM Object Properties
 
5. Use assertJSProperty(object, property) when evaluating JS Object Properties


== Refactoring  ==
== Refactoring  ==
; Timeouts & Delays
=== Phase I ===
* gDelay: replace with a discrete int value
{| class="fullwidth-table"
* gTimeout: replace with TIMEOUT
| Goal
 
| Implement lowest hanging fruit, make all tests reflect the styleguide
; Constants
|-
* const SOME_CONST = value;
| {{bug|}}
 
| Replace all constants with CONSTANTS
; setupModule & teardownModule
* gDelay, 100 => DELAY
* Remove ''module'' parameter
* gTimeout, 5000 => TIMEOUT
* sleep() parameter => SLEEP_TIMEOUT
* unique timeouts => CONSTANT (ie. SEARCH_TIMEOUT)
|-
| {{bug|}}
| Replace discrete, unchanging values with CONSTANTS
|-
| {{bug|}}
| Migrate tests away from testGeneral
|-
| {{bug|}}
| All test pages in LOCAL_TEST_PAGES array
|-
| {{bug|}}
| Replace waitForEval() with waitFor() where possible
|-
| {{bug|}}
| Replace DELAY/TIMEOUT with waitFor() where possible
|-
| {{bug|}}
| License block formatting
* "the Mozilla Foundation"
* contributor list alignment
|-
| {{bug|}}
| All error messages use format:
* message + " - got " + condition1 + ", expected " + condition2
* ''message'' uses positive wording
|-
| {{bug|}}
| Whitespace formatting
* No trailing whitespace
* 1 newline at end of file
* 2-space indentation where appropriate
* Wrap on 80-characters where appropriate
|-
| {{bug|}}
| Replace sleep() with waitFor() where possible
|-
| {{bug|}}
| Extract any ''new elementslib...'' from function parameter list
|-
| {{bug|}}
| Replace all array iteration with Array.forEach()
|-
| {{bug|}}
| Replace anonymous functions with named functions
|-
| {{bug|}}
| Adopt consistent commenting style
|-
| {{bug|}}
| All iterators as ''i'', not ''ii''
|-
| {{bug|}}
| Adopt consistent conditional cuddling
|-
| {{bug|}}
| All included modules should use variable names starting with capital
|-
| {{bug|}}
| All conditionals use !== or ===
|}


; Mozilla Components
''Note: The above comes from the agreed to refactoring [http://etherpad.mozilla.com:9000/mozmill-test-refactor guidelines]''
* Ensure proper alignment of getService() on Cc[]


; Local Test Pages
=== Phase II ===
* Replace any LOCAL_PAGE or LOCAL_TEST_PAGE with a LOCAL_TEST_PAGES array
Goal: Bring tests in-line with Shared Module Refactor
* Each array element should have at least a URL and ID member


; Arrays
== Outstanding Items ==
* Ensure proper alignment from style guidelines
Discussion [http://mozqa.ietherpad.com/refactor-discussion here]


; Assertions
; Generally Agreed
* waitForEval: replace with waitFor()
* We should always use the triple operator to not experience strange behavior
** ===/!== instead of ==/!=
* Variables for imported modules have to start with a capital letter
** i.e. var Tabs = require(".../tabs")
* Class names have to start with a capital letter
* setupModule(module) and teardownModule(module) have to specify the module parameter
* If variable name contains an acronym, capitalize the acronym
* Error messages should use the following format:
** Boolean: "<element> is <state>."
** Value: "<element> is <state>: got 'value', expected 'value'"
** Example: "Password notification is visible"
** Example: "mozilla.org is loaded: got '<loaded_domain>', expected 'mozilla.org'"
* sleep() should be avoided and only used when...
** element is not immediately available
** state is not immediately available
** waitFor() fails to capture event


== Examples ==
; Needs Discussion
* TestFilesAPI for loading of test files
** We should have a module specific to loading of test files
** We should also check how we can make sure those locations are easily replacable with other servers (apache, ...)
* Parameters should be named using aParam format
** shows that a variable inside a long function is a parameter and hasn't been declared locally
* Constant scope - should they be global and local?
Ashughes
Confirmed users
14,525

edits

Retrieved from "https://wiki.allizom.org/Special:MobileDiff/266323...285247"