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

From MozillaWiki
Jump to navigation Jump to search
 
(40 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 30: Line 33:
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.
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.


== Phase I ==
== Style Guidelines ==
Goal: Refactor "lowest-hanging-fruit" changes<br>
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.
Dependencies: Agreed upon style guidelines


== 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
; Mozilla Components
* unique timeouts => CONSTANT (ie. SEARCH_TIMEOUT)
* Ensure proper alignment of getService() on Cc[]
|-
 
| {{bug|}}
; Local Test Pages
| Replace discrete, unchanging values with CONSTANTS
* Replace any LOCAL_PAGE or LOCAL_TEST_PAGE with a LOCAL_TEST_PAGES array
|-
* Each array element should have at least a URL and ID member
| {{bug|}}
 
| Migrate tests away from testGeneral
; Arrays
|-
* Ensure proper alignment from style guidelines
| {{bug|}}
 
| All test pages in LOCAL_TEST_PAGES array
; Assertions
|-
* waitForEval: replace with waitFor()
| {{bug|}}
 
| Replace waitForEval() with waitFor() where possible
== Style Guidelines ==
|-
; Timeouts & Delays
| {{bug|}}
* Proposed Guidelines:
| Replace DELAY/TIMEOUT with waitFor() where possible
** Delay: Use discrete value
|-
** Timeout: Use global TIMEOUT
| {{bug|}}
* Henrik: Timeouts should be encapsulated within a global shared module
| License block formatting
* Geo: Use default params where appropriate, wrapper functions where not
* "the Mozilla Foundation"
 
* contributor list alignment
; Constants
|-
* Proposed Guidelines:
| {{bug|}}
** Exist between ''requires'' and ''setupModule()''
| All error messages use format:
** const SOME_CONSTANT = value;
* message + " - got " + condition1 + ", expected " + condition2
* Geo: All ''unchanging'' variables as constants
* ''message'' uses positive wording
 
|-
; Function Signatures
| {{bug|}}
* Proposed Guidelines:
| Whitespace formatting
** var nameOfFunction = function() {
* No trailing whitespace
* 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.
* 1 newline at end of file
 
* 2-space indentation where appropriate
; ''module'' as a param for setupModule & teardownModule
* Wrap on 80-characters where appropriate
* Proposed Guidelines:
|-
** ???
| {{bug|}}
* Geo: Strange to have a function not declare a parameter that’s actually being supplied, but maybe that’s more normal in JS.
| Replace sleep() with waitFor() where possible
* Henrik: Removing module was probably a mistake; used for injecting global functions into the module scope.
|-
 
| {{bug|}}
; Mozilla Components
| Extract any ''new elementslib...'' from function parameter list
* Proposed Guidelines:
|-
** var obj = Cc[“@mozilla.org/component;1”].getService(Ci.interface);
| {{bug|}}
** Declare in setupModule()
| Replace all array iteration with Array.forEach()
** For word-wrapping, split after the ., getService() aligned with Cc[]
|-
 
| {{bug|}}
; LOCAL_TEST_PAGES
| Replace anonymous functions with named functions
* Proposed Guidelines:
|-
** Encapsulate all local test pages within a const LOCAL_TEST_PAGES array
| {{bug|}}
** Each element (test page) consists of a URL and an ID
| Adopt consistent commenting style
* Henrik: We should use Name, Link, and XPath as well
|-
* Aaron: Establish a module/API to handle loading of any needed test files
| {{bug|}}
 
| All iterators as ''i'', not ''ii''
; Array Formatting
|-
* Proposed Guidelines:
| {{bug|}}
** Each array element on its own line
| Adopt consistent conditional cuddling
** Each element member on its own line
|-
<pre>
  | {{bug|}}
const LOCAL_TEST_PAGES = [
  | All included modules should use variable names starting with capital
  { id: “Some_ID”,
|-
  url: LOCAL_TEST_FOLDER + ‘someFolder/someFile.html’ },
  | {{bug|}}
  { id: “Some_ID”,
| All conditionals use !== or ===
  url: LOCAL_TEST_FOLDER + ‘someFolder/someFile.html’ },
|}
  { id: “Some_ID”,
  url: LOCAL_TEST_FOLDER + ‘someFolder/someFile.html’ }
];
</pre>


; Assertions
''Note: The above comes from the agreed to refactoring [http://etherpad.mozilla.com:9000/mozmill-test-refactor guidelines]''
* 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
=== Phase II ===
* Proposed Guideline:
Goal: Bring tests in-line with Shared Module Refactor
** 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.


== Outstanding Items ==
Discussion [http://mozqa.ietherpad.com/refactor-discussion here]


; Iteration
; Generally Agreed
* Proposed Guideline:
* We should always use the triple operator to not experience strange behavior
** Use array.forEach() for iterating arrays
** ===/!== instead of ==/!=
** Use traditional for() for iterating strings
* Variables for imported modules have to start with a capital letter
<pre>
** i.e. var Tabs = require(".../tabs")
array.forEach(function(elem, [index, [array]]) {
* Class names have to start with a capital letter
  statements; 
* setupModule(module) and teardownModule(module) have to specify the module parameter
}, [thisObject]);
* If variable name contains an acronym, capitalize the acronym
</pre>
* Error messages should use the following format:
* Geo: Documentation should call out that thisObject is the fourth parameter
** Boolean: "<element> is <state>."
* 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.
** 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


; General Formatting
; Needs Discussion
* Proposed Guideline:
* TestFilesAPI for loading of test files
** Adhere to an 80 character per line limitation
** We should have a module specific to loading of test files
** In most cases, indentation should be 2-spaces from the parent
** We should also check how we can make sure those locations are easily replacable with other servers (apache, ...)
* 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.
* Parameters should be named using aParam format
Examples:
** shows that a variable inside a long function is a parameter and hasn't been declared locally
* Function call:
* Constant scope - should they be global and local?
<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>

Latest revision as of 21:14, 16 February 2011

Overview

Lead: Anthony Hughes
Co-workers: TBD
Dates: TBD
Status: on hold
Repository Location: TBD
Tracking Bug(s) bug 604700
Documentation:

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

The 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.

Refactoring

Phase I

Goal Implement lowest hanging fruit, make all tests reflect the styleguide
bug Replace all constants with CONSTANTS
  • gDelay, 100 => DELAY
  • 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 ===

Note: The above comes from the agreed to refactoring guidelines

Phase II

Goal: Bring tests in-line with Shared Module Refactor

Outstanding Items

Discussion here

Generally Agreed
  • 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
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?
Retrieved from "https://wiki.allizom.org/index.php?title=QA/Mozmill_Test_Automation/Test_Modules_Refactor&oldid=285247"