Further improve test independence.

Further improve test independence.

Remove many test interdependencies by giving local window.flags variables
to modules that need it.

Use loadFixture everywhere instead of duplicating code, including in the
jujuTests.utils object itself.

Mark the remaining test interdependencies and defects, to be fixed later.

Remove a meaningless test at the bottom of test_startup.js.bottom .

Add removal of generated test_startup.js file to Makefile "clean" entry.

https://code.launchpad.net/~teknico/juju-gui/test-interdep-cleanup/+merge/170408

(do not edit description out of merge proposal)

[teknico, 2013-06-19 17:36:37]

Please take a look.

[gary.poster, 2013-06-19 18:07:26]

LGTM with addressing/considering comments.

Thank you

Gary

https://codereview.appspot.com/10383047/diff/1/app/templates/overview.handlebars
File app/templates/overview.handlebars (right):

https://codereview.appspot.com/10383047/diff/1/app/templates/overview.handleb...
app/templates/overview.handlebars:20: <li class="landscape-reboot"><a
href="">Machine Restart Needed</a></li>
Please make sure that this change still renders the same across all of our
target browsers.  And if it does, can we just remove the href attribute
entirely?

https://codereview.appspot.com/10383047/diff/1/app/templates/service-header-d...
File app/templates/service-header-destroy-service.partial (right):

https://codereview.appspot.com/10383047/diff/1/app/templates/service-header-d...
app/templates/service-header-destroy-service.partial:3: <a id="destroy-service"
data-toggle="modal" href="">
Same comments as with other link tags.

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

https://codereview.appspot.com/10383047/diff/1/test/index.html#newcode94
test/index.html:94: position matches on setBusy: expected 100 to equal
109.09090423583984 -->
:-/ I wonder if we should disable this one.

https://codereview.appspot.com/10383047/diff/1/test/utils.js
File test/utils.js (right):

https://codereview.appspot.com/10383047/diff/1/test/utils.js#newcode122
test/utils.js:122: this._cached_charms = this._generate_cached_charms();
The point of the function was to cache them across all test runs because loading
these seemed to be slow.  I believe I had evidence that it saved a noticeable
amount of time in test runs.  If you want to rip this out for isolation, I
suggest that you place the generation in TestCharmStore initialize or something,
rather than stashing them on an attribute on the module for no reason AFAICT. 
Alternatively, keep the cache around but only return a copy (Y.mix) of the
charms in the TestCharmStore.loadByPath method.

[bac, 2013-06-19 18:20:28]

LGTM 2

https://codereview.appspot.com/10383047/diff/1/app/templates/overview.handlebars
File app/templates/overview.handlebars (right):

https://codereview.appspot.com/10383047/diff/1/app/templates/overview.handleb...
app/templates/overview.handlebars:20: <li class="landscape-reboot"><a
href="">Machine Restart Needed</a></li>
what is the effect of this change?  was something bad happening?

[teknico, 2013-06-20 11:50:17]

Thanks for both reviews, Gary and Brad.

https://codereview.appspot.com/10383047/diff/1/app/templates/overview.handlebars
File app/templates/overview.handlebars (right):

https://codereview.appspot.com/10383047/diff/1/app/templates/overview.handleb...
app/templates/overview.handlebars:20: <li class="landscape-reboot"><a
href="">Machine Restart Needed</a></li>
bac wrote:
> what is the effect of this change?  was something bad happening?

It's a leftover from the first supposition that tests changing URLs were causing
problems, I'm reverting it.

https://codereview.appspot.com/10383047/diff/1/app/templates/service-header-d...
File app/templates/service-header-destroy-service.partial (right):

https://codereview.appspot.com/10383047/diff/1/app/templates/service-header-d...
app/templates/service-header-destroy-service.partial:3: <a id="destroy-service"
data-toggle="modal" href="">
gary.poster wrote:
> Same comments as with other link tags.

Reverting this one too.

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

https://codereview.appspot.com/10383047/diff/1/test/index.html#newcode94
test/index.html:94: position matches on setBusy: expected 100 to equal
109.09090423583984 -->
gary.poster wrote:
> :-/ I wonder if we should disable this one.

Do we understand why it's failing?

https://codereview.appspot.com/10383047/diff/1/test/utils.js
File test/utils.js (right):

https://codereview.appspot.com/10383047/diff/1/test/utils.js#newcode122
test/utils.js:122: this._cached_charms = this._generate_cached_charms();
gary.poster wrote:
> The point of the function was to cache them across all test runs
> because loading these seemed to be slow.  I believe I had evidence
> that it saved a noticeable amount of time in test runs.

Right, sorry for disrupting this, it was not meant. Looking at test run times in
trunk and this branch, I notice an increase of about five seconds in the exec
time due to the ineffective caching.


> If you want to rip this out for isolation, I suggest that you place
> the generation in TestCharmStore initialize or something, rather
> than stashing them on an attribute on the module for no reason AFAICT.
> Alternatively, keep the cache around but only return a copy (Y.mix)
> of the charms in the TestCharmStore.loadByPath method.

I split the definition of jujuTests.utils using Y.mix, to be able to access the
loadFixture method from the caching code.

Test run times are back to what they were. Thank you.

[teknico, 2013-06-20 12:04:08]

*** Submitted:

Further improve test independence.

Remove many test interdependencies by giving local window.flags variables
to modules that need it.

Use loadFixture everywhere instead of duplicating code, including in the
jujuTests.utils object itself.

Mark the remaining test interdependencies and defects, to be fixed later.

Remove a meaningless test at the bottom of test_startup.js.bottom .

Add removal of generated test_startup.js file to Makefile "clean" entry.

R=gary.poster, bac
CC=
https://codereview.appspot.com/10383047

[teknico, 2013-06-20 13:48:34]

*** Submitted:

Further improve test independence.

Remove many test interdependencies by giving local window.flags variables
to modules that need it.

Use loadFixture everywhere instead of duplicating code, including in the
jujuTests.utils object itself.

Mark the remaining test interdependencies and defects, to be fixed later.

Remove a meaningless test at the bottom of test_startup.js.bottom .

Add removal of generated test_startup.js file to Makefile "clean" entry.

R=
CC=
https://codereview.appspot.com/10383047