-
Notifications
You must be signed in to change notification settings - Fork 2k
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.
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
[tests] JavaScript: extract searchindex.js-format test fixtures. #12102
base: master
Are you sure you want to change the base?
[tests] JavaScript: extract searchindex.js-format test fixtures. #12102
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
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. |
This comment was marked as outdated.
This comment was marked as outdated.
Which is the reverse of what I'm doing on the test suite where the PRs are huge. So don't worry |
…ot already exist.
…ixtures Conflicts: tests/test_search.py
@jayaddison is there anything more that needs to happen with this one? anything I can do to help get it over line? |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! I left a few comments which are mostly just nits.
* ~~~~~~~~~~~~~~~~ | ||
* | ||
* This script contains the language-specific data used by searchtools.js, | ||
* namely the list of stopwords, stemmer, scorer and splitter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comments in this file seem a little misleading? This is really just test scaffolding AFAICT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
tests/js/roots/multiterm/index.rst
Outdated
Main Page | ||
========= | ||
|
||
Welcome to the... main page! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would personally use this opportunity to add a bit of context about what this file tests.
tests/js/roots/partial/index.rst
Outdated
sphinx_utils module | ||
=================== | ||
|
||
Useful utilities. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same, I would personally use this opportunity to add a bit of context about what this file tests.
function loadFixture(name) { | ||
req = new XMLHttpRequest(); | ||
req.open("GET", `base/tests/js/fixtures/${name}`, false); | ||
req.send(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
titleterms = index.titleterms; | ||
eval(loadFixture("partial/searchindex.js")); | ||
|
||
[/* first-ignored */, searchterms, excluded, /*rest-ignored*/] = Search._parseQuery('sphinx'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would personally use underscores in these cases but it's not that big a deal.
[/* first-ignored */, searchterms, excluded, /*rest-ignored*/] = Search._parseQuery('sphinx'); | |
[_, searchterms, excluded, _] = Search._parseQuery('sphinx'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and yet the last value (the second _ in your suggestion) is assigned all of the remaining items
Technically, no. It would be assigned the remaining items if you were using ...rest
. You can check:
[_, two, _] = [1,2,3,4,5]
console.log(`${_}`) // 3
[_, two, ..._] = [1,2,3,4,5]
console.log(`${_}`) // 3,4,5
It's the same as in Python:
(one, two, *threefourfive) = (1, 2, 3, 4, 5)
…te_js_fixtures.py'
…nstead of comparing their sha256 hash values.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good from my perspective! Obviously you'll need a committer to approve/land.
@picnixz Could you take a look at this? I think it makes sense to merge this before proceeding with any more work on the search system. I reviewed it a couple weeks ago and all looks good to me (not to say I'm infallible but that's gotta count for something) |
|
||
|
||
def build(srcdir: Path) -> None: | ||
cmd = ('sphinx-build', '-E', '-q', '-b', 'html', f'{srcdir}', f'{srcdir}/_build') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use full options here ? It'd be a bit clearer in general (I think we use full options in CI/CD so that's why).
@pytest.mark.parametrize('directory', ( | ||
directory for directory in (TESTS_ROOT / 'js' / 'roots').iterdir() | ||
)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pytest.mark.parametrize('directory', ( | |
directory for directory in (TESTS_ROOT / 'js' / 'roots').iterdir() | |
)) | |
JAVASCRIPT_TEST_ROOTS = list((TESTS_ROOT / 'js' / 'roots').iterdir()) | |
@pytest.mark.parametrize('directory', JAVASCRIPT_TEST_ROOTS) |
For parametrized tests, I prefer having a non-iterator to ensure that we have everything before launching the test.
titleterms = index.titleterms; | ||
eval(loadFixture("partial/searchindex.js")); | ||
|
||
[/* first-ignored */, searchterms, excluded, /*rest-ignored*/] = Search._parseQuery('sphinx'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and yet the last value (the second _ in your suggestion) is assigned all of the remaining items
Technically, no. It would be assigned the remaining items if you were using ...rest
. You can check:
[_, two, _] = [1,2,3,4,5]
console.log(`${_}`) // 3
[_, two, ..._] = [1,2,3,4,5]
console.log(`${_}`) // 3,4,5
It's the same as in Python:
(one, two, *threefourfive) = (1, 2, 3, 4, 5)
@@ -0,0 +1 @@ | |||
Search.setIndex({"alltitles": {"sphinx_utils module": [[0, "sphinx-utils-module"]]}, "docnames": ["index"], "envversion": {"sphinx": 61, "sphinx.domains.c": 3, "sphinx.domains.changeset": 1, "sphinx.domains.citation": 1, "sphinx.domains.cpp": 9, "sphinx.domains.index": 1, "sphinx.domains.javascript": 3, "sphinx.domains.math": 2, "sphinx.domains.python": 4, "sphinx.domains.rst": 2, "sphinx.domains.std": 2}, "filenames": ["index.rst"], "indexentries": {}, "objects": {}, "objnames": {}, "objtypes": {}, "terms": {"also": 0, "ar": 0, "built": 0, "confirm": 0, "document": 0, "function": 0, "html": 0, "i": 0, "includ": 0, "input": 0, "javascript": 0, "known": 0, "match": 0, "partial": 0, "possibl": 0, "prefix": 0, "project": 0, "provid": 0, "restructuredtext": 0, "sampl": 0, "search": 0, "should": 0, "thi": 0, "titl": 0, "us": 0, "when": 0}, "titles": ["sphinx_utils module"], "titleterms": {"modul": 0, "sphinx_util": 0}}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lucky you that I've submitted my thesis and have more free time! |
Feature or Bugfix
Purpose
index
data in the JavaScript HTML search tests to match the format emitted by the Sphinx (Python) HTML builder.Detail
searchindex.js
is a JavaScript code file that runsSearch.setIndex(...)
with the contents of the index as an argument.karma
HTTP server andeval
it.searchindex.js
fixture content does not match the existing fixture files.Relates