Confirmed users
14,525
edits
QA/Mozmill Test Automation/Test Modules Refactor: Difference between revisions
QA/Mozmill Test Automation/Test Modules Refactor (view source)
Revision as of 22:06, 8 November 2010
, 8 November 2010→Style Guidelines
| Line 241: | Line 241: | ||
} | } | ||
</pre> | </pre> | ||
* 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 | 7. Anonymous Functions | ||
| Line 287: | Line 289: | ||
); | ); | ||
</pre> | </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 | 13. Component declarations should be indented in line with the parent | ||
| Line 297: | Line 300: | ||
15. Use waitFor() as much as possible. Only use sleep() when a waitFor() fails. | 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 | 16. Always use a discrete int value for any DELAYs | ||
| Line 308: | Line 312: | ||
} "Some message", TIMEOUT); | } "Some message", TIMEOUT); | ||
</pre> | </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 | 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 | 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 | 20. Use array.forEach() for iterating array elements | ||
| Line 319: | Line 327: | ||
}, [thisObject]); | }, [thisObject]); | ||
</pre> | </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 | 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 | 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 | 23. Element before action | ||
| Line 329: | Line 341: | ||
controller.action(obj, params); | controller.action(obj, params); | ||
</pre> | </pre> | ||
* Geo: Element getting will be considerably simpler post-refactor and won’t require a long line. | |||
24. Local test pages encapsulated in Array | 24. Local test pages encapsulated in Array | ||
| Line 341: | Line 354: | ||
]; | ]; | ||
</pre> | </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() | 25. Constants between requires and setupModule() | ||
| Line 354: | Line 369: | ||
var setupModule = function() { | var setupModule = function() { | ||
</pre> | </pre> | ||
* 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() | 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 | |||
<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) | |||
== Refactoring == | == Refactoring == | ||