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. -- Mailing list: https://launchpad.net/~yellow Post to : [email protected] Unsubscribe : https://launchpad.net/~yellow More help : https://help.launchpad.net/ListHelp
