Test rustdoc js by GuillaumeGomez · Pull Request #47250 · rust-lang/rust

GuillaumeGomez · 2018-01-07T15:21:35Z

Add tests for the rustdoc search. It was heavily required because of all the recent breaking changes that happened while I went through improvements in doc search (add search in/for generic search for example).

rust-highfive · 2018-01-07T15:21:45Z

Some changes occurred in HTML/CSS.

cc @GuillaumeGomez

rust-highfive · 2018-01-07T15:21:47Z

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

GuillaumeGomez · 2018-01-07T16:11:34Z

Ah apparently, @Mark-Simulacrum is the one! Could you give me a hand when you have time please?

QuietMisdreavus · 2018-01-07T16:41:29Z

The _ probably needs to be builder for line 457 to work.
This function needs to return Self::Output anyway, for the trait to match. Since Self::Output is PathBuf, that's what this needs to return. However, all of the other Step impls in this file return (), so that's probably what the Output needs to be here as well.

Certainly, I stopped trying to make it work and certainly forgot to commit the version which compiles. The problem is more global anyway. But thanks for taking a look in here. ;)

Mark-Simulacrum

Left some basic feedback. Let me know if you have any questions.

Mark-Simulacrum · 2018-01-07T21:53:37Z

You almost certainly want these arguments to be closer to the above. Something like RustdocJs, "rustdoc-js", "js-tests", Mode::Tool.
But frankly I'm not entirely certain what this tool is doing either way -- you'd probably actually not want it, or at least it seems like at the very least it shouldn't be generated by the macro. If this doesn't make sense, explaining why it's here would help.

I just tried my best understanding what was going on in here. My goal is to test the search in rust documentation. So I need to run this tool once the documentation has been generated. To run it, I need nodejs and I don't really know how to pick up the currently installed one.

Mark-Simulacrum · 2018-01-07T21:54:54Z

This should be something more reasonable, like src/tests/rustdoc-js.

Mark-Simulacrum · 2018-01-07T21:56:47Z

The commands here likely want to use the configured node executable (builder.config.nodejs or something like that).

Mark-Simulacrum · 2018-01-07T21:58:15Z

This (probably) shouldn't be here, unless you want to branch on rustdoc-js as the mode below and call into RustdocJs... I would remove this, and add RustdocJs to the list of tests here
rust/src/bootstrap/builder.rs
Lines 254 to 257 in ee220da
Kind::Test => describe!(check::Tidy, check::Bootstrap, check::DefaultCompiletest,
check::HostCompiletest, check::Crate, check::CrateLibrustc, check::Rustdoc,
check::Linkcheck, check::Cargotest, check::Cargo, check::Rls, check::Docs,
check::ErrorIndex, check::Distcheck, check::Rustfmt, check::Miri, check::Clippy),
.

GuillaumeGomez · 2018-01-08T22:44:34Z

@Mark-Simulacrum: Ok, moved a bit forward (still not compiling). Are you sure I need to put RustdocJS into the tool! macro call?

Mark-Simulacrum · 2018-01-10T13:59:16Z

Sorry for the delay in getting back to you. Since the tool RustdocJS added doesn't require compilation (as far as I can tell) I believe we should not have it in src/bootstrap/tool.rs whatsoever; otherwise I think the code looks good to me. Once you remove the changes to src/bootstrap/tool.rs then I think this should work -- let me know if it doesn't.

GuillaumeGomez · 2018-01-10T23:10:57Z

It requires at least rustdoc compilation in order to have a doc built (I need generated docs). Therefore it requires rustc and everything coming beforehand. :)

Mark-Simulacrum · 2018-01-10T23:44:41Z

So if you need specific generated docs, you can call builder.ensure(doc::Std { ... }) or if you need whatever the "default docs" are (please say you don't need this) then builder.default_docs(None) or something like that... if you need rustdoc built, then builder.ensure(tool::Rustdoc { ... }) is the invocation, I believe. You can look at how we pass it to cargo in builder.rs's fn cargo.

Does that help?

GuillaumeGomez · 2018-01-12T22:40:48Z

Yes, thanks a lot!

This is now ready on my side.

Mark-Simulacrum · 2018-01-13T01:12:18Z

Looks like we'll need to install nodejs for this to work. cc @alexcrichton -- are we okay with a default test requiring this (new) dependency?

You can see examples of how we do it in the emscripten script: https://github.com/rust-lang/rust/blob/master/src/ci/docker/scripts/emscripten.sh#L50-L53.

alexcrichton · 2018-01-13T18:09:34Z

Could we perhaps not run these tests by default but run them on one builder or when node is already detected? I think it'd be best to not pick up a new dependency for all builds but just some (opt-in) builds.

GuillaumeGomez · 2018-01-13T19:29:44Z

Sure. Let's go for testing only when node is present.

GuillaumeGomez · 2018-01-13T22:34:53Z

Done!

GuillaumeGomez · 2018-01-15T09:24:22Z

I'll put the explanation in this comment, don't hesitate to copy/paste it anywhere you might want.

This PR is pretty "simple" and "just" add tests for the rustdoc search. It was heavily required because of all the recent breaking changes that happened while I went through improvements in doc search (add search in/for generic search for example).

Mark-Simulacrum · 2018-01-15T15:00:21Z

r=me if you're ready and don't want docs team review; I have not looked over the tests themselves to verify that they make sense. It is also be worth checking that we have at least one builder on CI that would run these tests, and if not, probably adding nodejs to one of our test-running builders. My impression is that wasm32-unknown, dist-various-2, and asmjs builders all install nodejs, but only the asmjs builder today will run the test you want. I think that should be sufficient, but let me know if you disagree and we can discuss which builder would be best to add it to.

GuillaumeGomez · 2018-01-15T22:10:22Z

The doc team already gave its go through @QuietMisdreavus so I suppose it's fine. For me it seems good enough for a first step. We can always add more later on. :) Thanks a lot for your help in here @Mark-Simulacrum!

@bors: r=Mark-Simulcram

bors · 2018-01-15T22:10:23Z

📌 Commit 026c749 has been approved by Mark-Simulcram

Mark-Simulacrum · 2018-01-15T22:16:30Z

@bors r- r+

bors · 2018-01-15T22:16:30Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: Update Cargo and its dependencies #47280

bors · 2018-01-15T22:16:31Z

📌 Commit 026c749 has been approved by Mark-Simulacrum

…ark-Simulacrum Test rustdoc js Add tests for the rustdoc search. It was heavily required because of all the recent breaking changes that happened while I went through improvements in doc search (add search in/for generic search for example).

GuillaumeGomez · 2018-01-16T14:35:06Z

Apparently windows uses an old version:

02:28:22] C:\projects\rust\src\tools\rustdoc-js\tester.js:118
[02:28:22] let value = data[i][key];
[02:28:22] ^^^
[02:28:22] [02:28:22] SyntaxError: Block-scoped declarations (let, const, function, class) not yet supported outside strict mode
[02:28:22] at exports.runInThisContext (vm.js:53:16)
[02:28:22] at Module._compile (module.js:373:25)
[02:28:22] at Object.Module._extensions..js (module.js:416:10)
[02:28:22] at Module.load (module.js:343:32)
[02:28:22] at Function.Module._load (module.js:300:12)
[02:28:22] at Function.Module.runMain (module.js:441:10)
[02:28:22] at startup (node.js:140:18)
[02:28:22] at node.js:1043:3

GuillaumeGomez · 2018-01-16T14:38:57Z

@bors: r=Mark-Simulacrum

bors · 2018-01-16T14:38:57Z

📌 Commit 3a7e247 has been approved by Mark-Simulacrum

…ark-Simulacrum Test rustdoc js Add tests for the rustdoc search. It was heavily required because of all the recent breaking changes that happened while I went through improvements in doc search (add search in/for generic search for example).

Rollup of 6 pull requests - Successful merges: #47250, #47302, #47387, #47398, #47436, #47444 - Failed merges:

…ark-Simulacrum Test rustdoc js Add tests for the rustdoc search. It was heavily required because of all the recent breaking changes that happened while I went through improvements in doc search (add search in/for generic search for example).

Rollup of 6 pull requests - Successful merges: #47250, #47313, #47398, #47468, #47471, #47520 - Failed merges:

rust-highfive assigned alexcrichton Jan 7, 2018

QuietMisdreavus reviewed Jan 7, 2018
View reviewed changes

Mark-Simulacrum requested changes Jan 7, 2018
View reviewed changes

Mark-Simulacrum assigned Mark-Simulacrum and unassigned alexcrichton Jan 7, 2018

kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 8, 2018

kennytm 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 10, 2018

GuillaumeGomez added 5 commits January 12, 2018 22:51

Start adding js tests
f18c52b

Prepare main.js for tests
53d8ec0

Add tester and a few tests
43acd23

Move forward to add rustdoc test
50bb6ba

End of rustdoc-js tool add into builder
6952199

GuillaumeGomez force-pushed the test-rustdoc-js branch from 7edd379 to 6952199Compare January 12, 2018 22:40

Mark-Simulacrum approved these changes Jan 13, 2018
View reviewed changes

Only run rustdoc-js test suite when nodejs is available
026c749

GuillaumeGomez 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 Jan 14, 2018

GuillaumeGomez mentioned this pull request Jan 16, 2018
Rollup of 6 pull requests #47485
Closed

Fix for older JS versions
3a7e247

GuillaumeGomez mentioned this pull request Jan 16, 2018
Rollup of 6 pull requests #47492
Closed

bors added a commit that referenced this pull request Jan 16, 2018
Auto merge of#47492- GuillaumeGomez:rollup, r=GuillaumeGomez
047132d
Rollup of 6 pull requests - Successful merges: #47250, #47302, #47387, #47398, #47436, #47444 - Failed merges:

bors added a commit that referenced this pull request Jan 17, 2018
Auto merge of#47492- GuillaumeGomez:rollup, r=GuillaumeGomez
2d3558e
Rollup of 6 pull requests - Successful merges: #47250, #47302, #47387, #47398, #47436, #47444 - Failed merges:

kennytm mentioned this pull request Jan 17, 2018
Rollup of 19 pull requests #47513
Closed

GuillaumeGomez mentioned this pull request Jan 17, 2018
Rollup of 6 pull requests #47528
Merged

bors added a commit that referenced this pull request Jan 18, 2018
Auto merge of#47528- GuillaumeGomez:rollup, r=GuillaumeGomez
3bd4af8
Rollup of 6 pull requests - Successful merges: #47250, #47313, #47398, #47468, #47471, #47520 - Failed merges:

bors merged commit 3a7e247 into rust-lang:masterJan 18, 2018

GuillaumeGomez deleted the test-rustdoc-js branch January 18, 2018 18:02

GuillaumeGomez mentioned this pull request Feb 10, 2018
Add JS tests for rustdoc #46689
Closed

QuietMisdreavus mentioned this pull request May 31, 2018
Tests for rustdoc search indices #13444
Closed

	Kind::Test => describe!(check::Tidy, check::Bootstrap, check::DefaultCompiletest,
	check::HostCompiletest, check::Crate, check::CrateLibrustc, check::Rustdoc,
	check::Linkcheck, check::Cargotest, check::Cargo, check::Rls, check::Docs,
	check::ErrorIndex, check::Distcheck, check::Rustfmt, check::Miri, check::Clippy),

Uh oh!

Uh oh!

Conversation

GuillaumeGomez commented Jan 7, 2018• edited by Mark-Simulacrum Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Jan 7, 2018

Uh oh!

rust-highfive commented Jan 7, 2018

Uh oh!

GuillaumeGomez commented Jan 7, 2018

Uh oh!

QuietMisdreavusJan 7, 2018

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomezJan 7, 2018

Choose a reason for hiding this comment

Uh oh!

Mark-Simulacrum left a comment

Choose a reason for hiding this comment

Uh oh!

Mark-SimulacrumJan 7, 2018

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomezJan 8, 2018

Choose a reason for hiding this comment

Uh oh!

Mark-SimulacrumJan 7, 2018

Choose a reason for hiding this comment

Uh oh!

Mark-SimulacrumJan 7, 2018

Choose a reason for hiding this comment

Uh oh!

Mark-SimulacrumJan 7, 2018

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez commented Jan 8, 2018

Uh oh!

Mark-Simulacrum commented Jan 10, 2018

Uh oh!

GuillaumeGomez commented Jan 10, 2018

Uh oh!

Mark-Simulacrum commented Jan 10, 2018

Uh oh!

GuillaumeGomez commented Jan 12, 2018

Uh oh!

Mark-Simulacrum commented Jan 13, 2018

Uh oh!

alexcrichton commented Jan 13, 2018

Uh oh!

GuillaumeGomez commented Jan 13, 2018

Uh oh!

GuillaumeGomez commented Jan 13, 2018

Uh oh!

GuillaumeGomez commented Jan 15, 2018

Uh oh!

Mark-Simulacrum commented Jan 15, 2018

Uh oh!

GuillaumeGomez commented Jan 15, 2018

Uh oh!

bors commented Jan 15, 2018

Uh oh!

Mark-Simulacrum commented Jan 15, 2018

Uh oh!

bors commented Jan 15, 2018

Uh oh!

bors commented Jan 15, 2018

Uh oh!

GuillaumeGomez commented Jan 16, 2018

Uh oh!

GuillaumeGomez commented Jan 16, 2018

Uh oh!

bors commented Jan 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

GuillaumeGomez commented Jan 7, 2018•
edited by Mark-Simulacrum
Loading