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

From MozillaWiki
Jump to navigation Jump to search
No edit summary
 
(21 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 ==
=== Draft ===
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.
1. Local variables should be named using camel-case
<pre> var exampleVariableName = value;</pre>


2. Constants should be named using all-caps
== Refactoring  ==
<pre>const EXAMPLE_CONSTANT = value;</pre>
=== Phase I ===
 
{| class="fullwidth-table"
3. Blocks of code should be commented in-line:
| Goal
<pre>
| Implement lowest hanging fruit, make all tests reflect the styleguide
  // What the code does
|-
  some code;
| {{bug|}}
</pre>
| Replace all constants with CONSTANTS
 
* gDelay, 100 => DELAY
4. Functions should use JSDoc block-style comments:
* gTimeout, 5000 => TIMEOUT
<pre>
* sleep() parameter => SLEEP_TIMEOUT
  /**
* unique timeouts => CONSTANT (ie. SEARCH_TIMEOUT)
  * Purpose of the function
|-
  *
| {{bug|}}
  * @param {type} paramName
| Replace discrete, unchanging values with CONSTANTS
  *        Purpose of the parameter
|-
  */
| {{bug|}}
  function someFunction(someParam) {
| Migrate tests away from testGeneral
</pre>
|-
 
| {{bug|}}
5. Arrays block-style
| All test pages in LOCAL_TEST_PAGES array
<pre>
|-
  var obj = [
| {{bug|}}
    { child: ‘value’,
| Replace waitForEval() with waitFor() where possible
      child: ‘value }
|-
    { child: ‘value’,
| {{bug|}}
      child: ‘value}
| Replace DELAY/TIMEOUT with waitFor() where possible
  ];
|-
</pre>
| {{bug|}}
 
| License block formatting
6. Named Functions
* "the Mozilla Foundation"
<pre>
* contributor list alignment
  function someFunction() {
|-
    some code;
| {{bug|}}
  }
| All error messages use format:
</pre>
* message + " - got " + condition1 + ", expected " + condition2
* Geo: Only top-level and object funtions should be named
* ''message'' uses positive wording
* 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.
|-
 
| {{bug|}}
7. Anonymous Functions
| Whitespace formatting
<pre>
* No trailing whitespace
  var someFunction() = function() {
* 1 newline at end of file
    some code;
* 2-space indentation where appropriate
  }
* Wrap on 80-characters where appropriate
</pre>
|-
 
| {{bug|}}
8. Function Usage
  | Replace sleep() with waitFor() where possible
<pre>
|-
  var someVariable = someFunction(
| {{bug|}}
                      param1,
| Extract any ''new elementslib...'' from function parameter list
                      param2,
|-
                      paramN,
| {{bug|}}
                    );
  | Replace all array iteration with Array.forEach()
</pre>
|-
 
| {{bug|}}
9. Function Parameter Concatenation
| Replace anonymous functions with named functions
<pre>
|-
  var someVariabe = someFunction(param1,
| {{bug|}}
                      'some' +
| Adopt consistent commenting style
                      'concat' +
|-
                      'string'
| {{bug|}}
                    );
| All iterators as ''i'', not ''ii''
</pre>
|-
 
| {{bug|}}
10. waitFor() block-style
| Adopt consistent conditional cuddling
<pre>
|-
  controller.waitFor(function() {
| {{bug|}}
    return something == somethingElse;
| All included modules should use variable names starting with capital
  }, "Some error message", TIMEOUT);
|-
</pre>
| {{bug|}}
 
| All conditionals use !== or ===
11. XPath: split on '/'
|}
<pre>
var path = "/something" +
            "/something" +  
            "/something";
</pre>
 
12. 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>
* 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.
 
13. Component declarations should be indented in line with the parent
<pre>
var svc = Cc["string/for/service/component"].
          getService(Ci.nsINameOfInterface);
</pre>
 
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
<pre>
const TIMEOUT = 5000;
  ...
controller.waitFor(function() {
  return something == something;
} "Some message", TIMEOUT);
</pre>
* Henrik: Timeouts should be encapsulated within a global shared module
* 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
<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.
 
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
''Note: The above comes from the agreed to refactoring [http://etherpad.mozilla.com:9000/mozmill-test-refactor guidelines]''
<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.


24. Local test pages encapsulated in Array
=== Phase II ===
<pre>
Goal: Bring tests in-line with Shared Module Refactor
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>
* 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()
== Outstanding Items ==
<pre>
Discussion [http://mozqa.ietherpad.com/refactor-discussion here]
// Include required modules
var addons = require("../../../shared-modules/addons");
var modalDialog = require("../../../shared-modules/modal-dialog");


const TIMEOUT = 5000;
; Generally Agreed
const SEARCH_TIMEOUT = 30000;
* We should always use the triple operator to not experience strange behavior
const INSTALL_TIMEOUT = 30000;
** ===/!== 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


var setupModule = function() {
; Needs Discussion
</pre>
* TestFilesAPI for loading of test files
* Geo: All unchanging variables as constants
** We should have a module specific to loading of test files
* Henrik: This space should not be strictly reserved for constants
** 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
26. Do not pass a ''module'' parameter in setupModule() and teardownModule()
** shows that a variable inside a long function is a parameter and hasn't been declared locally
* Geo: Strange to have a function not declare a parameter that’s actually being supplied, but maybe that’s more normal in JS.
* Constant scope - should they be global and local?
* Henrik: Removing module was probably a mistake; used for injecting global functions into the module scope.
 
27. Use waitFor() when asserting for timeouts
<pre>
const TIMEOUT = 5000;
...
controller.waitFor(function() {
  return something == something;
} "Some message", TIMEOUT);
</pre>
* 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
 
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)
 
=== Vetted ===
 
== Refactoring  ==

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"