Normalize window.flags in tests

Normalize window.flags in tests

window.flags now works just as it does in the app: it is set in test/index.html to an empty object (since the tests don't have the :flags: ns), and each test or each feature (usually divided up into suites, as they should be) deals solely with that feature flag only.  If flags are set in a 'before', only those flags are unset in 'after', etc.  This is as much a policy proposal as a branch, and I've created a weekly card to discuss.

https://code.launchpad.net/~makyo/juju-gui/flags-cleanup/+merge/177470

(do not edit description out of merge proposal)

[matthew.scott, 2013-07-29 20:44:46]

Please take a look.

[gary.poster, 2013-07-29 21:03:01]

LGTM if you make the many changes I requested.  I tried to clarify the ones I
feel strongly about ("please"..."hygiene"...) and would like you to consider
("consider").

Happy to discuss if desired.

Thank you!

Gary

https://codereview.appspot.com/12060044/diff/1/test/index.html
File test/index.html (right):

https://codereview.appspot.com/12060044/diff/1/test/index.html#newcode29
test/index.html:29: window.flags = {};
+0.5. :-)  I'm fine with it and can see the argument.

https://codereview.appspot.com/12060044/diff/1/test/test_browser_search_widge...
File test/test_browser_search_widget.js (right):

https://codereview.appspot.com/12060044/diff/1/test/test_browser_search_widge...
test/test_browser_search_widget.js:40: window.flags.ac = true;
would love to see this removed in an after.  Understood it can be in another
branch.

alternatively (see following suggestions) put this in your beforeEach and clean
up in afterEach.

https://codereview.appspot.com/12060044/diff/1/test/test_fakebackend.js
File test/test_fakebackend.js (left):

https://codereview.appspot.com/12060044/diff/1/test/test_fakebackend.js#oldco...
test/test_fakebackend.js:549: delete window.flags;
Please move this "after" function to "afterEach", and change this line to
"window.flags = {};".  Otherwise, you are removing test hygiene.

https://codereview.appspot.com/12060044/diff/1/test/test_inspector_charm.js
File test/test_inspector_charm.js (right):

https://codereview.appspot.com/12060044/diff/1/test/test_inspector_charm.js#n...
test/test_inspector_charm.js:58: delete window.flags.serviceInspector;
Please move to afterEach.  I'd also prefer that you set "window.flags = {};"

https://codereview.appspot.com/12060044/diff/1/test/test_inspector_constraint...
File test/test_inspector_constraints.js (right):

https://codereview.appspot.com/12060044/diff/1/test/test_inspector_constraint...
test/test_inspector_constraints.js:32: window.flags.serviceInspector = true;
Consider moving this to beforeEach.

https://codereview.appspot.com/12060044/diff/1/test/test_inspector_constraint...
test/test_inspector_constraints.js:38: delete window.flags.serviceInspector;
Consider moving to afterEach.  I'd also prefer that you set "window.flags = {};"

https://codereview.appspot.com/12060044/diff/1/test/test_inspector_overview.js
File test/test_inspector_overview.js (right):

https://codereview.appspot.com/12060044/diff/1/test/test_inspector_overview.j...
test/test_inspector_overview.js:34: window.flags.serviceInspector = true;
consider moving to beforeEach.

https://codereview.appspot.com/12060044/diff/1/test/test_inspector_overview.j...
test/test_inspector_overview.js:64: delete window.flags.serviceInspector;
Same as before (consider move to afterEach? consider setting flags = {}?).

https://codereview.appspot.com/12060044/diff/1/test/test_inspector_settings.js
File test/test_inspector_settings.js (right):

https://codereview.appspot.com/12060044/diff/1/test/test_inspector_settings.j...
test/test_inspector_settings.js:33: window.flags.serviceInspector = true;
...same...

https://codereview.appspot.com/12060044/diff/1/test/test_inspector_settings.j...
test/test_inspector_settings.js:69: delete window.flags.serviceInspector;
...same...

https://codereview.appspot.com/12060044/diff/1/test/test_routing.js
File test/test_routing.js (left):

https://codereview.appspot.com/12060044/diff/1/test/test_routing.js#oldcode43
test/test_routing.js:43: delete window.flags;
this removes test hygiene.  Please set window.flags = {} in afterEach.

https://codereview.appspot.com/12060044/diff/1/test/test_sandbox_go.js
File test/test_sandbox_go.js (left):

https://codereview.appspot.com/12060044/diff/1/test/test_sandbox_go.js#oldcode56
test/test_sandbox_go.js:56: delete window.flags;
this removes hygiene. please move to afterEach and set flags.

https://codereview.appspot.com/12060044/diff/1/test/test_sandbox_python.js
File test/test_sandbox_python.js (left):

https://codereview.appspot.com/12060044/diff/1/test/test_sandbox_python.js#ol...
test/test_sandbox_python.js:58: delete window.flags;
...same hygiene comment...

https://codereview.appspot.com/12060044/diff/1/test/test_service_module.js
File test/test_service_module.js (left):

https://codereview.appspot.com/12060044/diff/1/test/test_service_module.js#ol...
test/test_service_module.js:153: delete window.flags;
same hygiene comment

[matthew.scott, 2013-07-29 21:26:11]

Please take a look.

https://codereview.appspot.com/12060044/diff/1/test/test_browser_search_widge...
File test/test_browser_search_widget.js (right):

https://codereview.appspot.com/12060044/diff/1/test/test_browser_search_widge...
test/test_browser_search_widget.js:40: window.flags.ac = true;
On 2013/07/29 21:03:01, gary.poster wrote:
> would love to see this removed in an after.  Understood it can be in another
> branch.
> 
> alternatively (see following suggestions) put this in your beforeEach and
clean
> up in afterEach.

It's already there.  Will move to afterEach per other suggestions, though.

[matthew.scott, 2013-07-29 21:29:33]

flag handling now done in *Each methods.

https://codereview.appspot.com/12060044/diff/1/test/test_browser_search_widge...
File test/test_browser_search_widget.js (right):

https://codereview.appspot.com/12060044/diff/1/test/test_browser_search_widge...
test/test_browser_search_widget.js:40: window.flags.ac = true;
On 2013/07/29 21:03:01, gary.poster wrote:
> would love to see this removed in an after.  Understood it can be in another
> branch.
> 
> alternatively (see following suggestions) put this in your beforeEach and
clean
> up in afterEach.

Already in place, but moved to afterEach per suggestion.

https://codereview.appspot.com/12060044/diff/1/test/test_fakebackend.js
File test/test_fakebackend.js (left):

https://codereview.appspot.com/12060044/diff/1/test/test_fakebackend.js#oldco...
test/test_fakebackend.js:549: delete window.flags;
On 2013/07/29 21:03:01, gary.poster wrote:
> Please move this "after" function to "afterEach", and change this line to
> "window.flags = {};".  Otherwise, you are removing test hygiene.

Per IRC, this is legacy and being removed.

[rharding, 2013-07-30 15:40:32]

LGTM thanks for the cleanup on most of that though I don't follow why the flag
setting can't be in before/after and managed once per test block vs before/after
every test.

[matthew.scott, 2013-07-30 16:20:43]

*** Submitted:

Normalize window.flags in tests

window.flags now works just as it does in the app: it is set in test/index.html
to an empty object (since the tests don't have the :flags: ns), and each test or
each feature (usually divided up into suites, as they should be) deals solely
with that feature flag only.  If flags are set in a 'before', only those flags
are unset in 'after', etc.  This is as much a policy proposal as a branch, and
I've created a weekly card to discuss.

R=gary.poster, rharding
CC=
https://codereview.appspot.com/12060044