juju-gui

Merge lp://staging/~benji/juju-gui/bug-1088507 into lp://staging/juju-gui/experimental

bug-1088507
Merge into trunk

Proposed by Benji York on 2012-12-14

Status:

Merged

Merged at revision:

285

Proposed branch:

lp://staging/~benji/juju-gui/bug-1088507

Merge into:

lp://staging/juju-gui/experimental

Diff against target:

925 lines (+433/-395)

7 files modified

Makefile (+19/-10)
app/app.js (+1/-0)
app/views/charm-panel.js (+32/-4)
bin/merge-files (+2/-0)
test/index.html (+5/-1)
test/test_charm_configuration.js (+20/-29)
test/test_model.js (+354/-351)

To merge this branch:

bzr merge lp://staging/~benji/juju-gui/bug-1088507

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Juju GUI Hackers		2012-12-14	Pending
Review via email: mp+140020@code.staging.launchpad.net

Description of the change

Make the prod tests pass.

Changes a test that was failing because of some slight style difference to be
more direct. Some slight style differences still exist between prod and dev.

The charm model module (app/models/charm.js) is now included in the combined
and minified JS file.

The event-simulate and node-event-simulate functions are needed for the tests,
so they are now directly loaded by test/index.html.

The test_charm_configuration.js file needs juju-charm-models but that module
was not specified.

test/test_model.js had to be rearranged so the YUI().use method was called
inside the "before" method instead of around the entire test module. This
resulted in a mass reindenting.

Incidentally: Fixes the Makefile so YUI module assets are in the right place
so requests for them do not 404.

https://codereview.appspot.com/6947057/

Revision history for this message

Gary Poster (gary) wrote on 2012-12-14:

Land with changes.

Thank you for accomplishing this difficult task.

In addition to the comments below, please add a request in process.rst
that reviewers run both prod tests and debug tests. Then, in the
Makefile, either make the default test the prod test, or make it thumb
its nose at us and tell us to run one of the other targets.

Gary

https://codereview.appspot.com/6947057/diff/1/Makefile
File Makefile (right):

https://codereview.appspot.com/6947057/diff/1/Makefile#newcode293
Makefile:293: cp -r --parents */assets
"$(PWD)/build-$(1)/juju-ui/assets/")
Looks nicer to me, thanks.

https://codereview.appspot.com/6947057/diff/1/bin/merge-files
File bin/merge-files (right):

https://codereview.appspot.com/6947057/diff/1/bin/merge-files#newcode50
bin/merge-files:50: paths.push(syspath.join(process.cwd(),
'app/models/charm.js'));
As we discussed, please either figure out why readdir is not finding
charm.js, or put an XXX and a bug, and we will come back to this.

https://codereview.appspot.com/6947057/diff/1/test/index.html
File test/index.html (right):

https://codereview.appspot.com/6947057/diff/1/test/index.html#newcode9
test/index.html:9: <script
src="/juju-ui/assets/node-event-simulate.js"></script>
You explained the horror behind these two inclusions. Please add a
comment so others can enjoy.

https://codereview.appspot.com/6947057/

Revision history for this message

Nicola Larosa (teknico) wrote on 2012-12-17:

Needs fixing.

Running "make test-prod" produces 60 failures here, which is admittedly
a nice improvement down from 79, but still not enough. :-)

You most likely don't see these failures: I'd like to understand why I
do.

There's one more thing I'd like to clarify, see comment inline.

https://codereview.appspot.com/6947057/diff/1/Makefile
File Makefile (right):

https://codereview.appspot.com/6947057/diff/1/Makefile#newcode293
Makefile:293: cp -r --parents */assets
"$(PWD)/build-$(1)/juju-ui/assets/")
There's been some back and forth on this. You originally put in the "cp"
instruction; then I wanted to have symlinks in the build directories
rather than copies, so I spent some effort to come up with the "find"
command that created the right links in the right places, and did not
see requests for them return 404. Your experience has apparently been
different. I'd like to understand: 1) why; 2) if there's value enough,
in having symlinks instead of copies, to worry about this.

https://codereview.appspot.com/6947057/

lp://staging/~benji/juju-gui/bug-1088507 updated on 2012-12-17

278. By Benji York on 2012-12-14: Review change: remove the "test" target and have it tell the user to be explicit about what they want
279. By Benji York on 2012-12-14: add more commentary from reveis
280. By Benji York on 2012-12-17: add comment suggested in review
281. By Benji York on 2012-12-17: add some test tiles to the build directories