Code review comment for lp:~frankban/juju-gui/bug-1081803-tests-mutate-url

Revision history for this message
Francesco Banconi (frankban) wrote :

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/

« Back to merge proposal