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
