launchpad-reviewers team mailing list archive

[Richard Harding <rick.harding@xxxxxxxxxxxxx>]

Review: Approve

Thanks for more tests!

#10
This would probably work out better as an attribute. Then you could use a valueFn() to set the lp_client. In this way passing a custom lp.client into initializer would automatically set it vs doing the same logic in the initializer itself.

#73
I think it'd be nice to have the html either part of a <script type="text/x-template"> and pulled in via setUp methods. This was we can keep all test dom in one container that's easy to .empty() and makes for more stable tests. You can be sure the tests aren't polluting the dom. I've been running into some of these in my quest to update to YUI 3.5.

#104
I'd say to go ahead and remove the empty setup/teardown, but if you implement ^^ it'll make it necessary. What might actually be best is to have a second test suite that's working on the DOM state so that setup/teardown aren't doing work not required for the test_library_exists.

I've mocked up the changes here:
https://pastebin.canonical.com/69284/

I've made changed the call to the constructor to just pass the config object inline vs a new variable config. No sense creating variables not required, extra memory, etc.
-- 
https://code.launchpad.net/~jcsackett/launchpad/add-yui-tests-for-comments/+merge/113118
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.