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.
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 debug.js (right):
File app/modules-
https:/ /codereview. appspot. com/6856070/ diff/9001/ app/modules- debug.js# newcode10 debug.js: 10: // minimizer will not parse it.
app/modules-
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 app.js: 34: YUI(GlobalConfi g).use( ['juju- gui', utils'] , function(Y) {
test/test_
'juju-tests-
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/