[tests] JavaScript: extract searchindex.js-format test fixtures. #12102

jayaddison · 2024-03-15T18:35:38Z

Feature or Bugfix

Refactoring

Purpose

Reformats the test index data in the JavaScript HTML search tests to match the format emitted by the Sphinx (Python) HTML builder.

Detail

The content of searchindex.js is a JavaScript code file that runs Search.setIndex(...) with the contents of the index as an argument.
Instead of loading the index per-test from a variable, retrieve each fixture from the built-in local karma HTTP server and eval it.
Includes fixtures generated from genuine, buildable test Sphinx project sources included within this repository.
Fails the Python test suite when regenerated searchindex.js fixture content does not match the existing fixture files.

Relates

Resolves Tests: generate the indexes in the HTML search JavaScript tests using the application code. #12099.

…chindex.js test fixtures

…roots

jayaddison · 2024-03-15T22:32:06Z

In follow-up pull requests, the plan is to generate fixtures from genuine, buildable test Sphinx project sources included within this repository.

This turned out to be less code than I expected, so I've bundled it in here too, but could separate it again if that's easier for review/understanding.

…able in test cases

tests/js/searchtools.js

…ixtures

picnixz · 2024-03-16T14:27:12Z

I know there are a lot of these small changes / PRs, so it's probably tricky to develop the context for what's changing.

Which is the reverse of what I'm doing on the test suite where the PRs are huge. So don't worry

tests/js/fixtures/multiterm/searchindex.js

tests/js/fixtures/cpp/searchindex.js

tests/js/searchtools.js

… refactor/extract-search-result-gathering

…Terms' variable to 'searchParameters'. Relates-to commit 2ef85ba.

…ndex The Search._index property is assigned during the setIndex(...) function call, as performed by our evaluation of searchindex.js content. Fixup-for / relates-to commit 7370d20.

Post-merge / relates-to commit 94b9fc5.

…h-only. Follows-on-from commit 7370d20.

…ixtures

tests/js/searchtools.js

…o indexed terms.

tests/test_search.py

…ixtures

…ot already exist.

…ixtures

…ixtures Conflicts: tests/test_search.py

…ixtures

wlach · 2024-04-25T11:50:44Z

@jayaddison is there anything more that needs to happen with this one? anything I can do to help get it over line?

jayaddison · 2024-04-25T12:28:03Z

Thanks @wlach! I think I should re-test this manually after the latest merge; any additional code review feedback would be useful. I do remember that there was some JavaScript variable assignment logic that I felt was a bit non-obvious (see the within-line comments attempting to explain it).

jayaddison · 2024-04-25T13:01:36Z

Thanks @wlach! I think I should re-test this manually after the latest merge; any additional code review feedback would be useful. I do remember that there was some JavaScript variable assignment logic that I felt was a bit non-obvious (see the within-line comments attempting to explain it).

Ok, yep - manually testing from 9f5df2c seems good here; I took the opportunity to merge/re-test #12047 after doing that, and it tests correctly too (the Welcome search test case for sphinx.git de-duplicates two top search results into one with the fix in place; the search results for configuration continue to be a bit spammy, but that doesn't appear to be due to title-based duplication).

…ixtures

wlach

Nice work! I left a few comments which are mostly just nits.

CHANGES.rst

wlach · 2024-05-04T13:11:01Z

tests/js/language_data.js

+ * ~~~~~~~~~~~~~~~~
+ *
+ * This script contains the language-specific data used by searchtools.js,
+ * namely the list of stopwords, stemmer, scorer and splitter.


The comments in this file seem a little misleading? This is really just test scaffolding AFAICT

Mm, yep - this is an interesting case - it's an evaluated copy of a template file from the basic theme. That's me attempting to make the JavaScript test environment realistic, but it is duplicative (and a bit confusing).

wlach · 2024-05-04T13:12:04Z

tests/js/roots/multiterm/index.rst

+Main Page
+=========
+
+Welcome to the... main page!


I would personally use this opportunity to add a bit of context about what this file tests.

wlach · 2024-05-04T13:12:16Z

tests/js/roots/partial/index.rst

+sphinx_utils module
+===================
+
+Useful utilities.


Same, I would personally use this opportunity to add a bit of context about what this file tests.

wlach · 2024-05-04T13:18:23Z

tests/js/searchtools.js

+  function loadFixture(name) {
+      req = new XMLHttpRequest();
+      req.open("GET", `base/tests/js/fixtures/${name}`, false);
+      req.send(null);


I'm guessing you used XMLHttpRequest() because you want to make a synchronous? Should probably add a comment about that.

Apparently using XMLHttpRequest this way is discouraged and might be deprecated at some point, but they've been saying that since 2014. 😉 It's probably fine.

Ok, that's good to know :) Yep; we don't want the test to proceed until the fixture has definitely loaded -- I'm inclined to use a straightforward (even if seemingly dated) approach here.

wlach · 2024-05-04T13:19:50Z

tests/js/searchtools.js

-      titleterms = index.titleterms;
+      eval(loadFixture("partial/searchindex.js"));
+
+      [/* first-ignored */, searchterms, excluded, /*rest-ignored*/] = Search._parseQuery('sphinx');


I would personally use underscores in these cases but it's not that big a deal.

Suggested change

[/* first-ignored */, searchterms, excluded, /*rest-ignored*/] = Search._parseQuery('sphinx');

[_, searchterms, excluded, _] = Search._parseQuery('sphinx');

JavaScript's unpacking behaviour here seems somewhat inconsistent to me; the value on the left is a single item from the value on the right-hand-side, and yet the last value (the second _ in your suggestion) is assigned all of the remaining items. I was hoping for a way to retain that information for future readers/copiers in case they attempted to make adjustments that wouldn't work as they expect.

Ah ok, you learn something new every day. ;) I might use an underscore just for the first element then. But as I said above, not a big deal.

tests/test_search.py

…ion.

…rojects.

…te_js_fixtures.py'

…nstead of comparing their sha256 hash values.

jayaddison · 2024-05-04T20:11:01Z

Thank you @wlach for the code review! I've left a couple of not-entirely-resolved items open, and let me know what you think of the adjustments.

wlach

Looks good from my perspective! Obviously you'll need a committer to approve/land.

[tests] JavaScript: extract searchindex.js-format test fixtures

cf53a7c

jayaddison added html search type:tests internals:refactoring javascript Pull requests that update Javascript code labels Mar 15, 2024

jayaddison mentioned this pull request Mar 15, 2024

Tests: generate the indexes in the HTML search JavaScript tests using the application code. #12099

Open

jayaddison marked this pull request as draft March 15, 2024 19:37

This comment was marked as resolved.

Sign in to view

jayaddison added 2 commits March 15, 2024 22:23

[tests] JavaScript: write Sphinx sources that produce acceptable sear…

311c4f0

…chindex.js test fixtures

[tests] linting: add a ruff exclude filter for the JS fixture source …

e0bee37

…roots

jayaddison marked this pull request as ready for review March 15, 2024 22:33

jayaddison added 2 commits March 16, 2024 10:34

[tests] nitpick: reduce the changeset diff by retaining an index vari…

4340bbe

…able in test cases

Add CHANGES.rst entry

40fc985

jayaddison commented Mar 16, 2024

View reviewed changes

tests/js/searchtools.js Outdated Show resolved Hide resolved

jayaddison marked this pull request as draft March 16, 2024 10:43

jayaddison and others added 3 commits March 16, 2024 10:48

[tests] fixup: regenerate multiterm test search index

7e22d78

[tests] refactor: use a separate index fixture directory per root

8f753a8

Merge branch 'master' into issue-12099-prep/extract-js-search-index-f…

dd1fde8

…ixtures

jayaddison marked this pull request as ready for review March 16, 2024 10:59

jayaddison mentioned this pull request Mar 16, 2024

HTML Search: omit anchor reference from document titles in the search index. #12047

Draft

[search] Refactor search code to improve testability

695037d

jayaddison mentioned this pull request Mar 16, 2024

[search] Refactor: make the search code more testable. #12107

Closed

[tests] expect duplicate result due to sphinx-doc#11961

58b5e90

This comment was marked as outdated.

Sign in to view

picnixz reviewed Mar 16, 2024

View reviewed changes

tests/js/fixtures/multiterm/searchindex.js Outdated Show resolved Hide resolved

tests/js/fixtures/cpp/searchindex.js Outdated Show resolved Hide resolved

tests/js/searchtools.js Show resolved Hide resolved

tests/js/searchtools.js Outdated Show resolved Hide resolved

jayaddison added 3 commits March 16, 2024 15:08

[tests] run 'js-beautify -r ...' on each searchindex.js file

c37bb53

[tests] Fixup: regenerate cpp searchindex.js

8ac1b6c

Merge branch 'issue-12099-prep/extract-js-search-index-fixtures' into…

e31a7bd

… refactor/extract-search-result-gathering

jayaddison added 6 commits April 4, 2024 18:37

[search] Rectification: test cases: rename misleadingly-named 'search…

cd1a368

…Terms' variable to 'searchParameters'. Relates-to commit 2ef85ba.

[tests] search: add end-to-end / search-result-aggregation test.

7e4b562

[tests] search: fixup: initialize index-term variables from Search._i…

ec4f236

…ndex The Search._index property is assigned during the setIndex(...) function call, as performed by our evaluation of searchindex.js content. Fixup-for / relates-to commit 7370d20.

[tests] search: migrate partial-match search to use fixture data.

9b709d7

Post-merge / relates-to commit 94b9fc5.

[tests] search: re-constrain cpp and partial test cases to term-searc…

3d108c1

…h-only. Follows-on-from commit 7370d20.

Merge branch 'master' into issue-12099-prep/extract-js-search-index-f…

1b59d03

…ixtures

jayaddison commented Apr 4, 2024

View reviewed changes

tests/js/searchtools.js Show resolved Hide resolved

[tests] search: use Search.parseQuery function to match query terms t…

d0125c7

…o indexed terms.

jayaddison commented Apr 5, 2024

View reviewed changes

tests/test_search.py Outdated Show resolved Hide resolved

jayaddison and others added 4 commits April 5, 2024 10:28

Merge branch 'master' into issue-12099-prep/extract-js-search-index-f…

fa64b98

…ixtures

Merge branch 'master' into issue-12099-prep/extract-js-search-index-f…

0086771

…ixtures

[utils] js fixtures: create destination parent directory if it does n…

3a6c784

…ot already exist.

Merge branch 'master' into issue-12099-prep/extract-js-search-index-f…

14f1b98

…ixtures

jayaddison mentioned this pull request Apr 9, 2024

7.3.0 release plan #12242

Closed

jayaddison and others added 4 commits April 10, 2024 10:51

Merge branch 'master' into issue-12099-prep/extract-js-search-index-f…

d7e0e82

…ixtures

Merge branch 'master' into issue-12099-prep/extract-js-search-index-f…

08460ca

…ixtures Conflicts: tests/test_search.py

Merge branch 'master' into issue-12099-prep/extract-js-search-index-f…

2297164

…ixtures

Merge branch 'master' into issue-12099-prep/extract-js-search-index-f…

9f5df2c

…ixtures

Merge branch 'master' into issue-12099-prep/extract-js-search-index-f…

59d497d

…ixtures

wlach reviewed May 4, 2024

View reviewed changes

jayaddison added 5 commits May 4, 2024 17:49

Fixup: relocate CHANGES entry for this feature to in-development sect…

cc8ee12

…ion.

Fixup: add self-credit to CHANGES entry.

2cc1476

Tests: attempt to add explanatory content to JS test fixture source p…

7e92842

…rojects.

Tests: regenerate JavaScript fixtures by running 'python utils/genera…

9c0f2b2

…te_js_fixtures.py'

Tests: compare regenerated-vs-existing search indices byte-for-byte i…

7acb3cd

…nstead of comparing their sha256 hash values.

wlach approved these changes May 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[tests] JavaScript: extract searchindex.js-format test fixtures. #12102

[tests] JavaScript: extract searchindex.js-format test fixtures. #12102

jayaddison commented Mar 15, 2024 •

edited

This comment was marked as resolved.

jayaddison commented Mar 15, 2024

This comment was marked as outdated.

picnixz commented Mar 16, 2024

wlach commented Apr 25, 2024

jayaddison commented Apr 25, 2024

jayaddison commented Apr 25, 2024

wlach left a comment

wlach May 4, 2024

jayaddison May 4, 2024

wlach May 4, 2024

wlach May 4, 2024

wlach May 4, 2024

jayaddison May 4, 2024

wlach May 4, 2024

jayaddison May 4, 2024

wlach May 6, 2024

jayaddison commented May 4, 2024

wlach left a comment

	[/* first-ignored /, searchterms, excluded, /rest-ignored*/] = Search._parseQuery('sphinx');
	[_, searchterms, excluded, _] = Search._parseQuery('sphinx');

[tests] JavaScript: extract searchindex.js-format test fixtures. #12102

Are you sure you want to change the base?

[tests] JavaScript: extract searchindex.js-format test fixtures. #12102

Conversation

jayaddison commented Mar 15, 2024 • edited

Feature or Bugfix

Purpose

Detail

Relates

This comment was marked as resolved.

jayaddison commented Mar 15, 2024

This comment was marked as outdated.

picnixz commented Mar 16, 2024

wlach commented Apr 25, 2024

jayaddison commented Apr 25, 2024

jayaddison commented Apr 25, 2024

wlach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayaddison commented May 4, 2024

wlach left a comment

Choose a reason for hiding this comment

jayaddison commented Mar 15, 2024 •

edited