Merge lp:~gary-lasker/software-center/launcher-integration-unit-tests into lp:software-center/5.2

[Gary Lasker]

* lp:~gary-lasker/software-center/launcher-integration-unit-tests:
   - additional unit tests to round out the suite for the Unity
     launcher integration feature

This branch adds new unit tests for improved coverage of the Software Center side of the Unity launcher integration feature. First, it adds support for testing that the launcher add occurs on the "transaction-finished" event from aptdaemon per the latest functionality. Second, it adds coverage for the available pane's add_application_to_unity_launcher() method, and checks for proper operation (that is, whether or not the call to unity_launcher.send_application_to_launcher is actually made) for three different desktop file configurations:

  1. A fully displayable application (the launcher call is made)
  2. An application whose desktop file specifies "NoDisplay=true (the launcher call is *not* made)
  3. An application whose desktop file does not specify an "Exec" entry (the launcher call is *not* made)

As noted, the prerequisite for the tests are the branch:

  lp:~gary-lasker/software-center/tech-items-to-launcher-fix-lp1006483

Thanks for your review!

[Michael Vogt (mvo)]

Thanks, this looks good and its good that the tests are working with the new functionality and extended to cover the additional testcases.

Some suggestions for refactor:

* _navigate_to_appdetails_and_install and _install_from_list_view look pretty similar at the end, the common code should probably go into a single function to avoid duplication

* pyflakes has some comments that needs adressing before this can land

* I don't think that _fake_send_application_to_launcher_and_check is actually called in test_unity_launcher_integration_disabled, test_unity_launcher_integration_details_view or test_unity_launcher_integration_list_view. I added a invalid line in it ("xxx") and a print
and none of this appears to be executed. Please have a look at this, I suspect you just need
to use patch.object() here as the available_pane is already created when the function runs.

[Michael Vogt (mvo)]

I think I'm wrong about using patch.object(), it seems the patch is at the right spot, but AFAICT _fake_send_application_to_launcher_and_check is not called. It could be that the conditions in _add_application_to_unity_launcher are not meet.

[lp:~gary-lasker/software-center/launcher-integration-unit-tests]

3057. By Gary Lasker on 2012-07-17

fix up older tests and make other improvements

3058. By Gary Lasker on 2012-07-17

refactor some common code

[Gary Lasker (gary-lasker)]

Hi Michael, ok, I updated the approach for the older tests and they are working correctly and everything should be quite thoroughly tested at this point. I also did the refactor that you suggested.

Many thanks!
Gary

[Michael Vogt (mvo)]

Thanks, this looks good now, thanks for the update on this. I will approve it and land it so that it can get merged for the unity SRU that is expected shortly.

One more question though, in "test_unity_launcher_integration_list_view" the code reads:
...
        # test the automatic add to launcher enabled functionality when
        # installing an app form the list view
        available_pane.add_to_launcher_enabled = True
        test_pkgname = "lincity-ng"
        # for testing, we substitute a fake version of UnityLauncher's
        # send_application_to_launcher method that lets us check for the
        # correct values and also avoids firing the actual dbus signal
        # to the unity launcher service
        available_pane.add_to_launcher_enabled = True
        test_pkgname = "software-center"
...
I assume this is a copy/paste oversight and the first 4 lines can be deleted?

I pushed a lp:~mvo/software-center/launcher-integration-unit-tests with this (and some other tweaks)
and will do a seperate MP for that.

review: Approve

[Michael Vogt (mvo)]

I'm sorry but I just noticed one more thing and therefore have one more question:

The functions test_unity_launcher_integration_{disabled,displayed_app,no_exec_in_desktop_file} all set self.expected_launcher_info. For the case where we expect that nothing is send to the launcher this can be skipped I think (or is there anything I'm missing why we would want to set it in this case?). For the case where its added to the launcher ("displayed_app") AFAICT there is no call to self._check_send_application_to_launcher_args() so we don't need to set it (or we should call self._check_send_application_to_launcher_args()). Given that the expected_launcher_info got tested earlier it looks to me like simply skipping the "self.expected_launcher_info" for those three testcases. Does that make sense?

[lp:~gary-lasker/software-center/launcher-integration-unit-tests]

3059. By Gary Lasker on 2012-07-18

we don't need to set the expected pkgname and launcher_info results if we are not calling the method to check them (this was left over from how the tests worked previously)

3060. By Gary Lasker on 2012-07-18

remove bit of code that I missed during cleanup

[Gary Lasker (gary-lasker)]

Hi Michael. Ok, most recent comment first. Indeed, the settings for expected results were leftover from the way the tests worked previously and so indeed are superfluous. I have removed them in my branch. Thanks for noticing that!

Regarding your comment just previous to the most recent, yes, that was just a piece of cleanup that I missed. I fixed that now as well. Oh, please note that it wasn't the first four lines of code that needed to be removed, but rather some lines after that.

Thanks again!

[Gary Lasker (gary-lasker)]

Oh, and your changes in lp:~mvo/software-center/launcher-integration-unit-tests seem fine. I guess we'll merge that after?

[Michael Vogt (mvo)]

On Wed, Jul 18, 2012 at 03:07:19PM -0000, Gary Lasker wrote:
> Oh, and your changes in lp:~mvo/software-center/launcher-integration-unit-tests seem fine. I guess we'll merge that after?

Thanks, yes, if it looks good to you (you know this code better than
me :) that will be merged too after your branch has landed.

Cheers,
 Michael

[lp:~gary-lasker/software-center/launcher-integration-unit-tests]

3061. By Gary Lasker on 2012-07-18

fix typo in a comment