Code review comment for lp:~tveronezi/juju-gui/change-requires-param

Revision history for this message
Gary Poster (gary) wrote :

Hi Thiago. I like what you did with the tests, even though I wish it
didn't have to be part of this branch. I'm OK with it landing though.

As I mentioned on IRC, I do see a test failure, but it seems shallow.
Please fix that, and maybe the small comment change I suggest.

Thank you,

Gary

https://codereview.appspot.com/6856070/diff/9001/app/modules-debug.js
File app/modules-debug.js (right):

https://codereview.appspot.com/6856070/diff/9001/app/modules-debug.js#newcode10
app/modules-debug.js:10: // minimizer will not parse it.
Maybe add "Please put in in the module itself, instead."

https://codereview.appspot.com/6856070/diff/9001/test/test_app.js
File test/test_app.js (right):

https://codereview.appspot.com/6856070/diff/9001/test/test_app.js#newcode34
test/test_app.js:34: YUI(GlobalConfig).use(['juju-gui',
'juju-tests-utils'], function(Y) {
This looks good to me. However, it is a big change to how we write our
tests, and not consistently applied to our tests. As I said on IRC, I
think there is a good argument to change our tests to be written this
way, but I don't understand why we need to change them here, in this
branch.

In the interest of progress, I'm ok with landing this if you then make a
retrospective discussion card for us to collectively determine the way
we want to move forward.

https://codereview.appspot.com/6856070/

« Back to merge proposal