HTML Search: Add test for querying #11950
Conversation
| @@ -367,8 +372,7 @@ const Search = { | |||
| //Search.lastresults = results.slice(); // a copy | |||
| // console.info("search results:", Search.lastresults); | |||
|
|
|||
| // print the results | |||
| _displayNextItem(results, results.length, searchTerms, highlightTerms); | |||
| return { results, searchTerms, highlightTerms }; | |||
There was a problem hiding this comment.
Refactored this to be seperate from displaying the results, which was necessary to add the test.
| // add support for partial matches | ||
| if (word.length > 2) { | ||
| const escapedWord = _escapeRegExp(word); | ||
| Object.keys(terms).forEach((term) => { | ||
| if (term.match(escapedWord) && !terms[word]) |
There was a problem hiding this comment.
terms[word] is a problem, since if it it's referring to the 0th item, it will have a false value.
This is probably rare in production unless you happen to be unlucky, but can easily happen in a test (see below)
| if (term.match(escapedWord) && !titleTerms[word]) | ||
| arr.push({ files: titleTerms[word], score: Scorer.partialTitle }); | ||
| }); | ||
| if (!terms.hasOwnProperty(word)) { |
There was a problem hiding this comment.
Checking for hasOwnProperty before iterating over all the terms seems like an easy optimization, so I just did it.
| @@ -496,9 +506,13 @@ const Search = { | |||
|
|
|||
| // create the mapping | |||
| files.forEach((file) => { | |||
| if (fileMap.has(file) && fileMap.get(file).indexOf(word) === -1) | |||
There was a problem hiding this comment.
Another fun little bug -- if the file was already present, but the word wasn't there, it would immediately jump to the else clause to reinitialize the map from scratch. Another edge case that would be more likely to happen in a test.
| @@ -21,14 +21,41 @@ describe('Basic html theme search', function() { | |||
| "<no title>", | |||
| "", | |||
| null, | |||
| 2, | |||
| 5, | |||
There was a problem hiding this comment.
The score was artificially low in this test because we were overwriting the main match with a partial match due to this bug:
https://github.com/sphinx-doc/sphinx/pull/11950/files#r1478405652
Fix a number of small bugs that this picked up along the way. Intended to eventually be updated after sphinx-doc#11942 is landed (but seperating out for now for ease-of-review).
wlach
force-pushed
the
html-search-test-search-query
branch
from
8ac1bd0
to
85fb222
Compare
| "index.rst" | ||
| ]]; | ||
| expect(Search.performTermsSearch(searchterms, excluded, terms, titleterms)).toEqual(hits); | ||
| }); | ||
|
|
||
| }); | ||
|
|
||
| describe('query', function() { | ||
| it("should duplicate on title and content", function() { |
There was a problem hiding this comment.
Change this to "should not" later after #11942 lands
|
@jayaddison Another search ticket to look at, probably useful in the context of #11942 and #11944 (no rush obviously) |
|
Moved this out into other PRs, closing. |
Subject: Add a unit test for querying functionality in search
Feature or Bugfix
Purpose
The search query function is currently untested, though it's a key piece of functionality.
Intended to eventually be updated after #11942 is landed (but seperating out for now for ease-of-review).
Detail
On investigation, there were a number of minor bugs in search term functionality, which were fixed along with this PR.
Relates