Merge lp:~gary-lasker/software-center/recommender-unit-test-updates into lp:software-center

[Gary Lasker]

 * lp:~gary-lasker/software-center/recommender-unit-test-updates:
   - update unit test to accomodate the new subcategory that has been
     added to the themes and tweaks category

When choosing a subcategory for get_test_window_catview specify the Internet category specifically rather than just taking the first one that contains subcategories; this lets us predict the actual subcategory results and fixes a failure in e.g. test_catview.py.

Many thanks for your review!

[Michael Vogt (mvo)]

Thanks for your branch.

I looked at the code and I it appears that the place that relies on a specific subcategory is
"test_recommended_for_you_display_recommendations_opted_in". So I think instead of hardcoding it in window.py
we should either set it in the test that needs it explicitely via self.subcat_view.set_subcategory() or by
passing a parameter to get_test_window_catview() so that in RecommendationsTestCase we can explicitely specify
that the subcategory is required. This makes the connection of what needs it more explicit and window.py more
generic. Could you please update that?

[Gary Lasker (gary-lasker)]

Hi Michael, thanks! So, the way I see it, get_test_window_catview is test code itself and so it should be determinate, meaning that when somebody calls it in a test they should know exactly what they are getting. Previously, the code there just iterated through categories until it found one that had subcategories and returned that. This is what caused the "client" test case to fail; we added a subcategory to a category that previously had none, and so get_test_window_catview suddenly returned a different category. Since the "client" test does extensive (and very useful) checks on the results, it failed when the "new" category suddenly got returned.

So, for that reason, I think that it's best that this is set the test category unambiguously right in the get_test_window_catview call. This way, any new tests that are written can rely on knowing exactly what they are getting by default (and if they want to test a different category to the default they can simply override the default provided by the test). In other words, I think it is preferable to set a reliable default category in a central location (the test setup code after all), and I feel it's it's a big improvement over the fragile approach we had previously (which I think it would be a mistake to return to).

I am happy to add a comment in the code to make it clearer that the "Internet" category will be returned by default, this may help for any new tests that rely on knowing the subcategory. But I prefer to leave the code as I've made it in my branch.

[Michael Vogt (mvo)]

On Tue, Aug 28, 2012 at 02:27:20PM -0000, Gary Lasker wrote:
> Hi Michael, thanks! So, the way I see it, get_test_window_catview is test code itself and so it should be determinate, meaning that when somebody calls it in a test they should know exactly what they are getting. Previously, the code there just iterated through categories until it found one that had subcategories and returned that. This is what caused the "client" test case to fail; we added a subcategory to a category that previously had none, and so get_test_window_catview suddenly returned a different category. Since the "client" test does extensive (and very useful) checks on the results, it failed when the "new" category suddenly got returned.

Thanks for your comments, I probably did not express myself very well,
I'm all for making it deterministic :) There is no disagreement here.

In the old days when the code was originally writen to get the test
window with the catview it did not mater what category was
selected, iirc it just tested basic properties like if there was a
subcategory window visilbe or not. At some later point it started to
reply on a specific category so for that test-case, it needs to be
deterministic and one way of archiving this is to make it explicit in
the test that needs it. We could use something like:
  get_test_window_catview(selected_category="Internet")
there and it would be deterministic and also clear that this tests
needs the category (as its requesting it explicitely).

> So, for that reason, I think that it's best that this is set the test category unambiguously right in the get_test_window_catview call. This way, any new tests that are written can rely on knowing exactly what they are getting by default (and if they want to test a different category to the default they can simply override the default provided by the test). In other words, I think it is preferable to set a reliable default category in a central location (the test setup code after all), and I feel it's it's a big improvement over the fragile approach we had previously (which I think it would be a mistake to return to).

Indeed, no disagreement here :) We do not want to go back to the old
system. Its unfortunately not entirely trivial (or not as trivial as
when its created) to set the subcategory after the window is created
(it would have to do a iteration over the categories in the test
itself) so having something like get_test_window_catview(db=None,
selected_category="Internet") sounds like a good compromise to me.

> I am happy to add a comment in the code to make it clearer that the "Internet" category will be returned by default, this may help for any new tests that rely on knowing the subcategory. But I prefer to leave the code as I've made it in my branch.

I think a comment would be needed in get_test_window_catview() to say
that tests will break if that category gets changed.

Thanks,
 Michael

[lp:~gary-lasker/software-center/recommender-unit-test-updates]

3128. By Gary Lasker on 2012-08-28

make the category selectable in get_test_window_catview and use use this facility for test_catview.py's RecommendationsTestCase

3129. By Gary Lasker on 2012-08-28

since get_test_window_catview *requires* a category that includes subcategories, we enforce this requirement with a ValueError

[Gary Lasker (gary-lasker)]

Ok, should be all fixed up per your suggestion. Note that since the test call get_test_window_catview requires a category that contains subcategories and we can now specify the selected_category, I added a ValueError check to enforce this.

Thanks again!

[Michael Vogt (mvo)]

On Tue, Aug 28, 2012 at 09:37:21PM -0000, Gary Lasker wrote:
> Ok, should be all fixed up per your suggestion. Note that since the test call get_test_window_catview requires a category that contains subcategories and we can now specify the selected_category, I added a ValueError check to enforce this.

Woah, this is very nice now, thanks for this!

=== modified file 'tests/gtk3/test_catview.py'
--- tests/gtk3/test_catview.py	2012-07-31 22:05:10 +0000
+++ tests/gtk3/test_catview.py	2012-08-28 21:36:26 +0000
@@ -124,7 +124,10 @@
         # patch the recommender to specify that we are not opted-in at
         # the start of each test
         mock_recommender_is_opted_in.return_value = False
-        self.win = get_test_window_catview(self.db)
+        # we specify the "Internet" category because we do specific checks
+        # in the following tests that depend on this being the category choice
+        self.win = get_test_window_catview(self.db,
+                                           selected_category="Internet")
         self.addCleanup(self.win.destroy)
         self.notebook = self.win.get_child()
         self.lobby = self.win.get_data("lobby")
 
=== modified file 'tests/gtk3/windows.py'
--- tests/gtk3/windows.py	2012-08-22 06:49:53 +0000
+++ tests/gtk3/windows.py	2012-08-28 21:36:26 +0000
@@ -389,8 +389,11 @@
     return win
 
 
-def get_test_window_catview(db=None):
-
+def get_test_window_catview(db=None, selected_category="Internet"):
+    ''' 
+        Note that selected_category must specify a category that includes
+        subcategories, else a ValueError will be raised.
+    '''
     def on_category_selected(view, cat):
         print("on_category_selected view: ", view)
         print("on_category_selected cat: ", cat)
@@ -422,10 +425,13 @@
     scroll.add(lobby_view)
     notebook.append_page(scroll, Gtk.Label(label="Lobby"))
 
-    # find a cat in the LobbyView that has subcategories
     subcat_cat = None
-    for cat in reversed(lobby_view.categories):
-        if cat.subcategories:
+    for cat in lobby_view.categories:
+        if cat.name == selected_category:
+            if not cat.subcategories:
+                raise ValueError('The value specified for selected_category '
+                                 '*must* specify a '
+                                 'category that contains subcategories!!')
             subcat_cat = cat
             break