launchpad-reviewers team mailing list archive

[Richard Harding <rick.harding@xxxxxxxxxxxxx>]

Review: Approve

Yay tests!!!

#33
Can you toss in some comments while you're adding methods to help aid others. While it's too much to comment out the whole thing, adding new items should probably have something to prevent digging out hole deeper.

While in there, can you toss a ; for lint on line 36 of the diff right after your code change?

#61
The usual layout would be to have the { on the end of the prev line and have a ({ with the rest of that block dedented.

#68
Tests should have an initial comment line describing the test in a bit more detail than the name.

#86/87
If you assert the same type (isTrue) multiple times, please toss the extra param of a comment on at least one of the assertions to aid in debugging which assertion failed. Otherwise the output is just "assert True failed "false" or something generic. 

#122
Same as above, but with the multiple areEqual assertions.


-- 
https://code.launchpad.net/~jcsackett/launchpad/comment-js-test-coverage/+merge/115165
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.