Review: Approve Most of these comments apply in multiple places, but I've only commented on the first occurrence of each.
Diff comments: > === modified file 'lib/lp/services/sitesearch/tests/test_bing.py' > --- lib/lp/services/sitesearch/tests/test_bing.py 2018-04-12 19:42:23 > +0000 > +++ lib/lp/services/sitesearch/tests/test_bing.py 2018-04-13 19:56:29 > +0000 > @@ -7,31 +7,177 @@ > > __metaclass__ = type > > +import json > +from os import path I know this is ported from the doctests, but could you please make it just 'import os.path'? We don't do 'from os import path' anywhere in LP other than sitesearch. > + > from fixtures import MockPatch > from requests.exceptions import ( > ConnectionError, > HTTPError, > ) > -from testtools.matchers import MatchesStructure > +from testtools.matchers import ( > + HasLength, > + MatchesStructure, > +) LP style is to indent the closing paren. (Use utilities/format-new-and-modified-imports.) > > +from lp.services.config import config > from lp.services.sitesearch import BingSearchService > from lp.services.sitesearch.interfaces import SiteSearchResponseError > from lp.services.timeout import TimeoutError > from lp.testing import TestCase > -from lp.testing.layers import ( > - BingLaunchpadFunctionalLayer, > - LaunchpadFunctionalLayer, > - ) > +from lp.testing.layers import BingLaunchpadFunctionalLayer > > > class TestBingSearchService(TestCase): > """Test BingSearchService.""" > > - layer = LaunchpadFunctionalLayer > - > def setUp(self): > super(TestBingSearchService, self).setUp() > self.search_service = BingSearchService() > + self.base_path = path.normpath( > + path.join(path.dirname(__file__), 'data')) > + > + def test_configuration(self): > + self.assertEqual(config.bing.site, self.search_service.site) > + self.assertEqual( > + config.bing.subscription_key, > self.search_service.subscription_key) > + self.assertEqual( > + config.bing.custom_config_id, > self.search_service.custom_config_id) > + > + def test_create_search_url(self): > + self.assertEndsWith( > + self.search_service.create_search_url(terms='svg +bugs'), > + '&offset=0&q=svg+%2Bbugs') > + > + def test_create_search_url_escapes_unicode_chars(self): > + self.assertEndsWith( > + self.search_service.create_search_url('Carlo Perell\xf3 > Mar\xedn'), Typo for "Carlos" (OK, it's test data, but I prefer not to misspell people's names, and this one was spelled correctly in the doctests). > + '&offset=0&q=Carlo+Perell%C3%B3+Mar%C3%ADn') > + > + def test_create_search_url_with_offset(self): > + self.assertEndsWith( > + self.search_service.create_search_url(terms='svg +bugs', > start=20), > + '&offset=20&q=svg+%2Bbugs') > + > + def test_create_search_url_empty_terms(self): > + e = self.assertRaises( > + ValueError, self.search_service.create_search_url, '') > + self.assertEqual("Missing value for parameter 'q'.", str(e)) You could use self.assertRaisesWithContent for this kind of test. > + > + def test_create_search_url_null_terms(self): > + e = self.assertRaises( > + ValueError, self.search_service.create_search_url, None) > + self.assertEqual("Missing value for parameter 'q'.", str(e)) > + > + def test_create_search_url_requires_start(self): > + e = self.assertRaises( > + ValueError, self.search_service.create_search_url, 'bugs', > 'true') > + self.assertEqual("Value for parameter 'offset' is not an int.", > str(e)) > + > + def test_parse_search_response_invalid_total(self): > + """The PageMatches's total attribute comes from the > + `webPages.totalEstimatedMatches` JSON element. > + When it cannot be found and the value cast to an int, > + an error is raised. If Bing were to redefine the meaning of the > + element to use a '~' to indicate an approximate total, an error would > + be raised. > + """ > + file_name = path.join( > + self.base_path, 'bingsearchservice-incompatible-matches.json') > + with open(file_name, 'r') as response_file: > + response = response_file.read() > + assert ( > + json.loads(response)['webPages']['totalEstimatedMatches'] == > '~25') self.assertEqual, not assert. > + > + e = self.assertRaises( > + SiteSearchResponseError, > + self.search_service._parse_search_response, response) > + self.assertEqual( > + "Could not get the total from the Bing JSON response.", str(e)) > + > + def test_parse_search_response_negative_total(self): > + """If the total is ever less than zero (see bug 683115), > + this is expected: we simply return a total of 0. > + """ > + file_name = path.join( > + self.base_path, 'bingsearchservice-negative-total.json') > + with open(file_name, 'r') as response_file: > + response = response_file.read() > + assert json.loads(response)['webPages']['totalEstimatedMatches'] == > -25 > + > + matches = self.search_service._parse_search_response(response) > + self.assertEqual(0, matches.total) > + > + def test_parse_search_response_missing_title(self): > + """A PageMatch requires a title, url, and a summary. If those > elements > + cannot be found, a PageMatch cannot be made. A missing title ('name') > + indicates a bad page on Launchpad, so it is ignored. In this example, > + the first match is missing a title, so only the second page is > present > + in the PageMatches. > + """ > + file_name = path.join( > + self.base_path, 'bingsearchservice-missing-title.json') > + with open(file_name, 'r') as response_file: > + response = response_file.read() > + assert len(json.loads(response)['webPages']['value']) == 2 > + > + matches = self.search_service._parse_search_response(response) > + self.assertThat(matches, HasLength(1)) > + self.assertEqual('GCleaner in Launchpad', matches[0].title) > + self.assertEqual('http://launchpad.dev/gcleaner', matches[0].url) I'd be inclined to go testtools matchers all the way rather than just for the first bit, since it produces more informative failures: self.assertThat(matches, MatchesListwise([ MatchesStructure.byEquality( title='GCleaner in Launchpad', url='http://launchpad.dev/gcleaner'), ])) > + > + def test_parse_search_response_missing_summary(self): > + """When a match is missing a summary ('snippet'), the match is > skipped > + because there is no information about why it matched. This appears to > + relate to pages that are in the index, but should be removed. In this > + example taken from real data, the links are to the same page on > + different vhosts. The edge vhost has no summary, so it is skipped. > + """ > + file_name = path.join( > + self.base_path, 'bingsearchservice-missing-summary.json') > + with open(file_name, 'r') as response_file: > + response = response_file.read() > + assert len(json.loads(response)['webPages']['value']) == 2 > + > + matches = self.search_service._parse_search_response(response) > + self.assertThat(matches, HasLength(1)) > + self.assertEqual('BugExpiry - Launchpad Help', matches[0].title) > + self.assertEqual( > + 'https://help.launchpad.net/BugExpiry', matches[0].url) > + > + def test_parse_search_response_missing_url(self): > + """When the URL ('url') cannot be found the match is skipped. There > are > + no examples of this. We do not want this hypothetical situation to > give > + users a bad experience. > + """ > + file_name = path.join( > + self.base_path, 'bingsearchservice-missing-url.json') > + with open(file_name, 'r') as response_file: > + response = response_file.read() > + assert len(json.loads(response)['webPages']['value']) == 2 > + > + matches = self.search_service._parse_search_response(response) > + self.assertThat(matches, HasLength(1)) > + self.assertEqual('LongoMatch in Launchpad', matches[0].title) > + self.assertEqual('http://launchpad.dev/longomatch', matches[0].url) > + > + def test_parse_search_response_with_no_meaningful_results(self): > + """If no matches are found in the response, and there are 20 or fewer > + results, an Empty PageMatches is returned. This happens when the > + results are missing titles and summaries. This is not considered to > be > + a problem because the small number implies that Bing did a poor job > + of indexing pages or indexed the wrong Launchpad server. In this > + example, there is only one match, but the results is missing a title > so > + there is not enough information to make a PageMatch. > + """ > + file_name = path.join( > + self.base_path, 'bingsearchservice-no-meaningful-results.json') > + with open(file_name, 'r') as response_file: > + response = response_file.read() > + assert len(json.loads(response)['webPages']['value']) == 1 > + > + matches = self.search_service._parse_search_response(response) > + self.assertThat(matches, HasLength(0)) > > def test_search_converts_HTTPError(self): > # The method converts HTTPError to SiteSearchResponseError. -- https://code.launchpad.net/~maxiberta/launchpad/sitesearch-cleanup-2/+merge/343236 Your team Launchpad code reviewers is subscribed to branch lp:launchpad. _______________________________________________ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : launchpad-reviewers@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp