Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Changes from 60 commits
cf53a7c
311c4f0
e0bee37
4340bbe
40fc985
7e22d78
8f753a8
dd1fde8
695037d
58b5e90
c37bb53
8ac1b6c
e31a7bd
fbaff49
d619a52
b33d331
55b6072
ee4316d
0634502
0bc558a
a74bc15
a037867
68a6504
564de92
64ecd41
34752b1
1addb80
76cc8d5
d66b7a2
01a4509
262dcec
52ee7c4
916e5af
de6f7a0
ef8e4b0
450aafe
94b9fc5
d885288
6987c48
fac7694
a464bdf
5b6a479
fc76aef
e9614b9
4d62b15
38bfc34
51f9a4d
8c6c669
5827576
9773c0c
66328bc
7370d20
2ef85ba
cd1a368
7e4b562
ec4f236
9b709d7
3d108c1
1b59d03
d0125c7
fa64b98
0086771
3a6c784
14f1b98
d7e0e82
08460ca
2297164
9f5df2c
59d497d
cc8ee12
2cc1476
7e92842
9c0f2b2
7acb3cd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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).
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.
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.
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.
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.
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.