Skip to content

HTML Search: Add test for querying #11950

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

HTML Search: Add test for querying #11950

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 30 additions & 16 deletions sphinx/themes/basic/static/searchtools.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,8 @@ const Search = {
if (Search._queued_query !== null) {
const query = Search._queued_query;
Search._queued_query = null;
Search.query(query);
let { results, searchTerms, highlightTerms } = Search.query(query);
_displayNextItem(results, results.length, searchTerms, highlightTerms);
}
},

Expand Down Expand Up @@ -235,8 +236,12 @@ const Search = {
Search.startPulse();

// index already loaded, the browser was quick!
if (Search.hasIndex()) Search.query(query);
else Search.deferQuery(query);
if (Search.hasIndex()) {
let { results, searchTerms, highlightTerms } = Search.query(query);
_displayNextItem(results, results.length, searchTerms, highlightTerms);
} else {
Search.deferQuery(query);
}
},

/**
Expand Down Expand Up @@ -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 };
Copy link
Contributor Author

@wlach wlach Feb 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored this to be seperate from displaying the results, which was necessary to add the test.

},

/**
Expand Down Expand Up @@ -463,17 +467,23 @@ const Search = {
{ files: terms[word], score: Scorer.term },
{ files: titleTerms[word], score: Scorer.title },
];

// add support for partial matches
if (word.length > 2) {
const escapedWord = _escapeRegExp(word);
Object.keys(terms).forEach((term) => {
if (term.match(escapedWord) && !terms[word])
Copy link
Contributor Author

@wlach wlach Feb 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

arr.push({ files: terms[term], score: Scorer.partialTerm });
});
Object.keys(titleTerms).forEach((term) => {
if (term.match(escapedWord) && !titleTerms[word])
arr.push({ files: titleTerms[word], score: Scorer.partialTitle });
});
if (!terms.hasOwnProperty(word)) {
Copy link
Contributor Author

@wlach wlach Feb 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking for hasOwnProperty before iterating over all the terms seems like an easy optimization, so I just did it.

Object.keys(terms).forEach((term) => {
if (term.match(escapedWord)) {
arr.push({ files: terms[term], score: Scorer.partialTerm });
}
});
}
if (!titleTerms.hasOwnProperty(word)) {
Object.keys(titleTerms).forEach((term) => {
if (term.match(escapedWord))
arr.push({ files: titleTerms[word], score: Scorer.partialTitle });
});
}
}

// no match but word was a required one
Expand All @@ -496,9 +506,13 @@ const Search = {

// create the mapping
files.forEach((file) => {
if (fileMap.has(file) && fileMap.get(file).indexOf(word) === -1)
Copy link
Contributor Author

@wlach wlach Feb 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

fileMap.get(file).push(word);
else fileMap.set(file, [word]);
if (fileMap.has(file)) {
if (fileMap.get(file).indexOf(word) === -1) {
fileMap.get(file).push(word);
}
} else {
fileMap.set(file, [word]);
}
});
});

Expand Down
8 changes: 8 additions & 0 deletions tests/js/documentation_options.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,9 @@
const DOCUMENTATION_OPTIONS = {};

// stub Stemmer / stopWords
function Stemmer() {
this.stemWord = function (word) {
return word;
}
};
let stopwords = [];
29 changes: 28 additions & 1 deletion tests/js/searchtools.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,41 @@ describe('Basic html theme search', function() {
"<no title>",
"",
null,
2,
5,
Copy link
Contributor Author

@wlach wlach Feb 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"index.rst"
]];
expect(Search.performTermsSearch(searchterms, excluded, terms, titleterms)).toEqual(hits);
});

});

describe('query', function() {
it("should duplicate on title and content", function() {
Copy link
Contributor Author

@wlach wlach Feb 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change this to "should not" later after #11942 lands

index = {
alltitles: {
'Main Page': [[0, 'main-page']],
},
docnames:["index"],
filenames:["index.rst"],
indexentries:{},
objects:{},
objtypes: {},
objnames: {},
terms:{main:0, page:0},
titles:["Main Page"],
titleterms:{ main:0, page:0 }
}
Search.setIndex(index);
let { results } = Search.query('main page');
// currently the search result is duplicated, see
// https://github.com/sphinx-doc/sphinx/pull/11942
expect(results).toEqual([
[ 'index', 'Main Page', '', null, 15, 'index.rst' ],
[ 'index', 'Main Page', '#main-page', null, 100, 'index.rst' ]
]);
});
})

});

// This is regression test for https://github.com/sphinx-doc/sphinx/issues/3150
Expand Down