launchpad-reviewers team mailing list archive

[Richard Harding <rick.harding@xxxxxxxxxxxxx>]

> 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.

Yes, it's a debate on how far to touch things in the process of just trying to get tests passing under YUI3.5. I'll probably hold off on this at the moment.

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

Sorry, forgot to repush after I ran lint. The branch should be updated. It was mainly comment blocks that were too long.

 
> [2]
> 
> 285     +     * gets pulled into the next text automatically.
> 
> s/text/test/ ?

ty
 
> [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?

I had the thought, but here's the interesting thing. If I were to reload the page, the previous state was causing the pollution from the browser itself. Now, if a user were to load the page, and do something, and reload the page, it would do the same. So I don't know that I'd want to blindly clear this in all tests. I'd want to make sure people were aware of how this works.

I'm blindly adding here to these tests because I'm documenting the issues as they come up and I've got a list of issues to look into going forward. If there is still an issue in production code, it'll be part of the tests coming out of this. If every test blanked it then tests would always pass, but we'd have a strange history state bug in production.

I guess I lean a bit toward explicit in the case of test setup/teardowns so that I can see expected changes and cleanup that my code does. I hate when people just do things like Y.one('body').setHTML('') because it doesn't say much about what they were expecting in there. There could have been extra garbage no one noticed because it wasn't left dangling for all to see. 
-- 
https://code.launchpad.net/~rharding/launchpad/listingnav_yui35/+merge/112334
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.