Code review comment for lp:~elachuni/software-center/reviews-tests

Revision history for this message
Michael Nelson (michael.nelson) wrote :

14:58 < noodles775> achuni: review swap? https://code.launchpad.net/~michael.nelson/software-center/833982-purchased-app-not-available-3/+merge/89695
14:58 < achuni> noodles775: for the reviews-tests mp?
14:58 * noodles775 starts achuni's MP, no probs if you've not time for mine.
14:58 < noodles775> Yeah.
14:58 < achuni> sounds fair :)
15:01 < noodles775> achuni: completely ignorable, but if we use multi-line imports for easier merging, l 102 is an opportunity.
15:01 < achuni> noodles775: ack
15:02 * noodles775 notices achuni's test class attributes, and wonders if there's any benefit to setUpClass from his own branch?
15:03 < achuni> noodles775: test class attributes?
15:03 < noodles775> line 122
15:04 < noodles775> I guess the only difference is *when* they are run... yours will be run when the code is parsed, where as using setUpClass it's when the test case (class) is executed. Either way they're run once only?
15:07 < achuni> noodles775: indeed, setUpClass looks interesting
15:07 < achuni> noodles775: let me switch for that
15:11 < noodles775> achuni: with the test on line 155
15:12 < noodles775> why do you do line 159: self.assertTrue(review_app._modify_review_is_the_same())?
15:13 < noodles775> I guess I expected to see a call to review_app.review_summary_entry.set_text(original) or similar?
15:14 < noodles775> Also, is there any significance to the cases defined on l161?
15:14 < noodles775> Ah, regarding my previous q re line 159, I guess i expected to see l167 right before line 159.
15:15 * noodles775 starts realising why doing email reviews with in line diffs is handy... when will LP get inline comments? :P
15:19 < noodles775> Again, totally ignorable, but you could replace the tearDown on line 223 with a self.addCleanup if you wanted to.
15:24 < noodles775> All good, adding a +1 vote now.

review: Approve

« Back to merge proposal