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.

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

Add library path argument for mdbook test #340

Merged
merged 1 commit into from
Jul 8, 2017
Merged

Add library path argument for mdbook test #340

merged 1 commit into from
Jul 8, 2017

Conversation

messense
Copy link
Contributor

Add -L/--library-path argument and pass it into rustdoc should be sufficient.

Closes #339

src/book/mod.rs Outdated
@@ -379,7 +383,7 @@ impl MDBook {

println!("[*]: Testing file: {:?}", path);

let output_result = Command::new("rustdoc").arg(&path).arg("--test").output();
let output_result = Command::new("rustdoc").arg(&path).arg("--test").args(&library_args).output();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this going to work okay when no arguments are passed?

Copy link
Contributor Author

@messense messense Jun 19, 2017

Choose a reason for hiding this comment

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

Yes. With no/empty arguments, it simply returns self.

https://github.com/rust-lang/rust/blob/master/src/libstd/process.rs#L403

@azerupi
Copy link
Contributor

azerupi commented Jun 19, 2017

Thanks!
I just have a simple question before merging, see inline comment. :)

src/book/mod.rs Outdated
// read in the chapters
self.parse_summary()?;
let library_args: Vec<&str> = (0..library_paths.len()).map(|_| "-L")
.zip(library_paths.into_iter())
.flat_map(|x| vec![x.0, x.1])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a little annoying that tuple does not implement IntoIterator trait, causing this vec![x.0, x.1] nonsense.

@messense messense mentioned this pull request Jun 20, 2017
@azerupi azerupi merged commit 55e7e82 into rust-lang:master Jul 8, 2017
@azerupi
Copy link
Contributor

azerupi commented Jul 8, 2017

Thanks! And sorry for the delay :)

Michael-F-Bryan added a commit to Michael-F-Bryan/mdBook that referenced this pull request Jul 9, 2017
@Michael-F-Bryan
Copy link
Contributor

It may be a good idea to thread this through the configuration system so you aren't continually writing the "-L" arguments.

When #371 lands I'll move onto tidying up configuration and see if I can add an extra "tests" table to the book.toml file for adding parameters to tests like external dependencies. We've just got to think of a way to configuration so it's scalable to multiple backends and plugin systems (html, pdf, rustdoc testing, mathjax, etc) and not special casing everything like we are now.

@messense messense deleted the feature/mdbook-test-library-path branch July 9, 2017 12:10
@azerupi
Copy link
Contributor

azerupi commented Jul 9, 2017

@Michael-F-Bryan I don't see this as a permanent solution. The test command is currently just a wrapper around rustdoc, maybe with some additional checks, because it was needed for the Rust book. But ideally we would want this to be configurable and extendable by the user. Because there is no way we can anticipate everything the user would want to "tests".

@Michael-F-Bryan
Copy link
Contributor

Michael-F-Bryan commented Jul 9, 2017

But ideally we would want this to be configurable and extendable by the user.

My thoughts were that you could add something like this to your book.toml:

[test]
runner = "rustdoc"
dependencies = ["error-chain", "rayon"]

To me the test runner is pretty much just an alternate renderer, except instead of rendering content it'll give you a pass or a fail (because a Renderer returns a Result).

Otherwise if you have your own custom test script it might be invoked like this:

[test]
runner = "shell"
command = "test_docs.py"

EDIT: Note that this would be more of a long-term goal. It relies on a more flexible system for rendering and configuration than what currently exists.

@azerupi
Copy link
Contributor

azerupi commented Jul 9, 2017

That would be nice indeed!

@budziq
Copy link
Contributor

budziq commented Jul 9, 2017

On the other hand rust-cookbook uses https://github.com/brson/rust-skeptic/. We never invoke mdbook test just using cargo test (extern crates are handled by cargo dependencies then) which imho is generally better approach if the mdbook is kind of insource/inrepo documentation (it nicely integrates with rest of the tests).

But I understand why mdbook test might be useful in other contexts.

@budziq
Copy link
Contributor

budziq commented Jul 9, 2017

Hmm on the other hand I do not see a better way (I mean this general approach suggested by @Michael-F-Bryan) to support spell checking and link checking which are very much desired. So big 👍

@budziq budziq mentioned this pull request Sep 3, 2017
Ruin0x11 pushed a commit to Ruin0x11/mdBook that referenced this pull request Aug 30, 2020
…brary-path

Add library path argument for `mdbook test`
Ruin0x11 pushed a commit to Ruin0x11/mdBook that referenced this pull request Aug 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants