Code review comment for lp:~gary-lasker/software-center/tech-items-to-launcher-fix-lp1006483

Revision history for this message
Michael Vogt (mvo) wrote :

Thanks, this change looks fine.

It would be nice to have a test for this too, _add_application_to_unity_launcher() appears to be not covered
currently and it would be nice to test it with additional test files in data/app-install/desktop/ using a
mock launcher_info to test that send_application_to_launcher() is called for the right desktop files
(should be really straightforward to add I hope).

While looking at this a bit more (trying to figure out if it was covered for not) I noticed that it seems
like the "test_unity_launcher_integration.py" file needs updating to cover the new behavior that the launcher
item is added on "transaction-finished" instead of "transaction-started".

It currently sends a fake "transaction-started" signal in the test, but in order to test
if the signal is actually delivered to unity it also needs to send a "transaction-finished" signal too.
So I would suggest (probably in the original branch or as a seperate branch not here in this branch as this
branch is nice and short) to update the test to send the "transaction-finished" signal and also to add a mock
for available_pane._add_application_to_unity_launcher so that after
self._navigate_to_appdetails_and_install(test_pkgname)" in the test the test can check that
the call was made as expected.

This got a bit long, so the summary is: this branch is fine and approve once the test situation is
sorted out.

« Back to merge proposal