Skip to content
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

Jump to bottom

Simplify rustdoc search test #92570

Merged
merged 1 commit into from
May 17, 2022
Merged

Simplify rustdoc search test #92570

merged 1 commit into from
May 17, 2022

Conversation

jsha
Copy link
Contributor

@jsha jsha commented Jan 5, 2022

Previously, rustdoc search attempted to parse search.js and extract out only certain methods and variables.

This change makes search.js and search-index.js loadable as CommonJS modules, so they can be loaded directly.

As part of that change, I had to separate execSearch from interacting with the DOM. This wound up being a nice cleanup that made more explicit what inputs it was taking.

I removed search.js' dependency on storage.js by moving hasOwnPropertyRustdoc directly into search.js, and replacing onEach with forEach in a path that is called by the tester.

r? @GuillaumeGomez

Demo: https://rustdoc.crud.net/jsha/rustdoc-search-refactor/std/?search=foo

@jsha jsha added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-rustdoc-search Area: Rustdoc's search feature labels Jan 5, 2022
@rust-highfive
Copy link
Collaborator

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez,@Folyd

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 5, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@jsha
Copy link
Contributor Author

jsha commented Jan 5, 2022

ESLint is failing because we need to update it to accept es6, which I do in #92490.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Jan 9, 2022

☔ The latest upstream changes (presumably #92690) made this pull request unmergeable. Please resolve the merge conflicts.

@camelid
Copy link
Member

camelid commented Jan 25, 2022

triage: @jsha could you please fix the merge conflicts?

@camelid
Copy link
Member

camelid commented Jan 25, 2022

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 25, 2022
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 13, 2022
@JohnCSimon
Copy link
Member

Triage
@jsha Can you please post the status of this PR?
@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 13, 2022
@GuillaumeGomez
Copy link
Member

I think @jsha is waiting for #90630 to be merged (please correct me if I'm wrong!).

@jsha
Copy link
Contributor Author

jsha commented Feb 16, 2022

That's correct.

@JohnCSimon JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 20, 2022
@jsha jsha force-pushed the rustdoc-search-refactor branch from c01f48d to 3c1034d Compare May 15, 2022 23:21
@jsha
Copy link
Contributor Author

jsha commented May 15, 2022

I've fixed the merge conflicts and this passes tests locally. Still need to do two things:

  • This seems to have picked up some unintended indentation changes from the code formatter in my IDE. I'll fix those.
  • I need to rebase to eliminate the merge commit

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@jsha jsha force-pushed the rustdoc-search-refactor branch 4 times, most recently from c8a73ce to 3faa18a Compare May 16, 2022 14:55
@jsha
Copy link
Contributor Author

jsha commented May 16, 2022

This is now ready for review.

@jsha jsha added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 16, 2022
GuillaumeGomez
GuillaumeGomez reviewed May 17, 2022
View reviewed changes
src/tools/rustdoc-js/tester.js
@@ -381,22 +150,20 @@ function valueCheck(fullPath, expected, result, error_text, queryName) {
}
}
}

function runParser(query, expected, loaded, loadedFile, queryName) {
function runParser(query, expected, parseQuery, queryName) {
Copy link
Member

Choose a reason for hiding this comment

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

Little nit: please add an empty line before this function.

@GuillaumeGomez
Copy link
Member

This is really great! All the hacks have been removed thanks to this, it's really awesome! Just one nit then it's good for me.

@jsha
rustdoc: make search.js a module
4539794 
Previously, search.js relied on the DOM and the `window` object. It can now be
loaded in the absence of the DOM, for instance by Node. The same is true of
search-index.js.

This allows removing a lot of code from src/tools/rustdoc-js/tester.js that
tried to parse search.js and extract specific functions that were needed for
testing.
@jsha jsha force-pushed the rustdoc-search-refactor branch from 3faa18a to 4539794 Compare May 17, 2022 16:26
@GuillaumeGomez
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented May 17, 2022

📌 Commit 4539794 has been approved by GuillaumeGomez

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 17, 2022
@bors
Copy link
Contributor

bors commented May 17, 2022

⌛ Testing commit 4539794 with merge 4c5f6e6...

@bors
Copy link
Contributor

bors commented May 17, 2022

☀️ Test successful - checks-actions
Approved by: GuillaumeGomez
Pushing 4c5f6e6 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 17, 2022
@bors bors merged commit 4c5f6e6 into rust-lang:master May 17, 2022
@rustbot rustbot added this to the 1.63.0 milestone May 17, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (4c5f6e6): comparison url.

Summary: This benchmark run did not return any relevant results.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

@matthiaskrgr matthiaskrgr mentioned this pull request Jul 22, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GuillaumeGomez GuillaumeGomez GuillaumeGomez approved these changes

Assignees

@GuillaumeGomez GuillaumeGomez

Labels
A-rustdoc-search Area: Rustdoc's search feature merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.63.0
Development

Successfully merging this pull request may close these issues.
9 participants
@jsha @rust-highfive @rust-log-analyzer @bors @camelid @JohnCSimon @GuillaumeGomez @rust-timer @rustbot