Code review comment for lp:~gary-lasker/software-center/handle-trans-cancel-lp1027209

> On Fri, Sep 14, 2012 at 10:53:23PM -0000, Gary Lasker wrote:
> > Gary Lasker has proposed merging lp:~gary-lasker/software-center/handle-
> trans-cancel-lp1027209 into lp:software-center.
> [..]
> > Follow the below steps to see the issue:
> > #. Open software-center
> > #. Click on install on a big application like wesnoth
> > #. Wait until the "In Progress" appears in the toplevel toolbar of s-c
> > #. Click on the "In Progress" button in the toolbar
> > #. Click on the "cancel" button of the install of the application
> >
> > I've also included a new unit test for this bug to insure we don't regress
> on it.
>
> Thanks for the branch and for adding a unittest (and the adding of
> docstrings, always nice).
>
> It looks fine, the only question I have is what happens if the
> transaction errors, e.g. if the network gets lost during a download of
> a application. Will that work as expected, i.e. not add anything to
> the launcher or do we need to connect the transaction-stopped signal
> for that?

Thanks! I've added a unit test for this case and and shows that the launcher call does not occur. I took the opportunity to consolodate some (now duplicated) code.

>
> [..]
> > === modified file 'tests/gtk3/test_unity_launcher_integration.py'
> > --- tests/gtk3/test_unity_launcher_integration.py 2012-08-30 07:45:31
> +0000
> > +++ tests/gtk3/test_unity_launcher_integration.py 2012-09-14 22:52:21
> +0000
> > @@ -60,6 +60,20 @@
> > self.available_pane.backend.emit("transaction-finished",
> > mock_result)
> > do_events_with_sleep()
> > +
> > + def _simulate_install_cancellation(self, app):
> > + # pretend we started an install
> > + self.available_pane.backend.emit("transaction-started",
> > + app.pkgname, app.appname,
> > + "testid101",
> > + TransactionTypes.INSTALL)
> > + do_events_with_sleep()
>
> self.assertEqual(
> self.available_pane.unity_launcher_transaction_queue.keys(),
> ["software-center"])
>
> > + # send the signal to cancel the install
> > + mock_result = Mock()
> > + mock_result.pkgname = app.pkgname
> > + self.available_pane.backend.emit("transaction-cancelled",
> > + mock_result)
> > + do_events_with_sleep()
>
> self.assertEqual(
> self.available_pane.unity_launcher_transaction_queue, {})
>
>
> [..]
>
> I like the test in general, the above lines may make it even
> better. To also check that the queue behaves in the right way
> (i.e. stuff is added and taken out again).

This diff is a little confusing to me as it appears to add code that is already in the test (in the method _simulate_install_cancellation). But I get the gist which is to check that the queue is maintained correcty. This may not be strictly necessary, however, but I'll think about it a bit more and will add this test enhancement in a separate follow-on branch in the interest of getting the current branch merged (we are close to the wire for b2 freeze).
>
> Cheers,
> Michael