yellow team mailing list archive

[Francesco Banconi <francesco.banconi@xxxxxxxxxxxxx>]

Thanks for the review Nicola.

On 2012/11/22 15:06:01, teknico wrote:
> Nice cleanup, just a couple minor comments inline.


https://codereview.appspot.com/6856075/diff/1/test/test_app.js#oldcode50
> test/test_app.js:50: }
> I'm a bit concerned about this: I wonder what exactly "losing the
race" means.
> It might warrant further investigation.

Maybe it means a subsequent test tries to create a container before it
is destroyed by the first one, but I'm just guessing. Anyway, as
mentioned, I was not able to reproduce that behavior, and we are doing
the same elsewhere. Also notice that removing the node in beforeEach
means it is not removed at all after the last test of that test case.

> https://codereview.appspot.com/6856075/diff/1/test/test_app_hotkeys.js
> File test/test_app_hotkeys.js (right):


https://codereview.appspot.com/6856075/diff/1/test/test_app_hotkeys.js#newcode41
> test/test_app_hotkeys.js:41: keyCode: 83, //  "S" key.
> Is the additional space before the "S" useful?

Absolutely not. And it's also wrong. Removing them.



https://codereview.appspot.com/6856075/

-- 
https://code.launchpad.net/~frankban/juju-gui/bug-1081803-tests-mutate-url/+merge/135670
Your team Juju GUI Hackers is requested to review the proposed merge of lp:~frankban/juju-gui/bug-1081803-tests-mutate-url into lp:juju-gui.