launchpad-reviewers team mailing list archive

[Raphaël Badin <raphael.badin@xxxxxxxxxxxxx>]

Review: Approve

Looks good.

[0]

1436	+        setUp: function() {
1437	+            this.target = Y.Node.create('<div></div>').set(
1438	+                'id', 'client-listing');
1439	+            Y.one('body').appendChild(this.target);
1440	+        },
1441	+
1442	+        tearDown: function() {
1443	+            this.target.remove();
1444	+            delete this.target;
1445	+            reset_history();
1446	+        },

I see a lot of identical setUp/tearDown methods.  Maybe you could put them in a test case class and use that class instead of having that code duplicated.

[1]

pocketlint thinks some of the lines are too long: http://paste.ubuntu.com/1062555/.

[2]

285	+     * gets pulled into the next text automatically.

s/text/test/ ?

[3]

>This also updates for YUI 3.5 by fixing some cleanup of the browser history state in the teardown methods of the 
> various test suites

> 1191	+    tests.suite.add(new Y.Test.Case({

Too bad we're not deriving our tests from a custom test case as this seems like the kind of cleanup that should always be done… don't you think?
-- 
https://code.launchpad.net/~rharding/launchpad/listingnav_yui35/+merge/112334
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.