-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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 a --chapter option to mdbook test. #1741
Changes from 10 commits
f0189cd
4d78119
bf6d452
71e490d
6e3d275
99a8c0d
f1c07b0
ed0ee85
96245d2
7c55869
0926c52
05ed667
601b6df
20cca38
5ec488c
9172005
c1004f7
e979012
d383ae0
4ead38f
1747094
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -244,8 +244,9 @@ impl MDBook { | |||||||||||
self | ||||||||||||
} | ||||||||||||
|
||||||||||||
/// Run `rustdoc` tests on the book, linking against the provided libraries. | ||||||||||||
pub fn test(&mut self, library_paths: Vec<&str>) -> Result<()> { | ||||||||||||
/// Run `rustdoc` tests on the book, linking against the provided libraries | ||||||||||||
/// and optionally testing only a signal named chapter while skipping the others. | ||||||||||||
pub fn test(&mut self, library_paths: Vec<&str>, chapter: Option<&str>) -> Result<()> { | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately we can't change the API as that would be a semver breaking change. I think you will need to add a new method, and have the old There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done, I added "test_chapter", but instead of duplicating the code I just make test call test_chapter and I added a couple new tests for "test_chapter". |
||||||||||||
let library_args: Vec<&str> = (0..library_paths.len()) | ||||||||||||
.map(|_| "-L") | ||||||||||||
.zip(library_paths.into_iter()) | ||||||||||||
|
@@ -270,8 +271,15 @@ impl MDBook { | |||||||||||
_ => continue, | ||||||||||||
}; | ||||||||||||
|
||||||||||||
let path = self.source_dir().join(&chapter_path); | ||||||||||||
info!("Testing file: {:?}", path); | ||||||||||||
if chapter.is_some() | ||||||||||||
&& ch.name != chapter.unwrap() | ||||||||||||
&& chapter_path.to_str().unwrap() != chapter.unwrap() | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd prefer to avoid using unwrap here. Perhaps more like this:
Suggested change
However, this logic seems wrong. It seems to require that the !(ch.name == chapter || chapter_path.to_str() == Some(chapter)) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it works, notice this is the inverse of the test |
||||||||||||
{ | ||||||||||||
debug!("Skipping chapter '{}'...", ch.name); | ||||||||||||
continue; | ||||||||||||
}; | ||||||||||||
|
||||||||||||
info!("Testing chapter '{}': {:?}", ch.name, chapter_path); | ||||||||||||
|
||||||||||||
// write preprocessed file to tempdir | ||||||||||||
let path = temp_dir.path().join(&chapter_path); | ||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -67,11 +67,11 @@ | |
var theme = localStorage.getItem('mdbook-theme'); | ||
var sidebar = localStorage.getItem('mdbook-sidebar'); | ||
|
||
if (theme.startsWith('"') && theme.endsWith('"')) { | ||
if (theme && theme.startsWith('"') && theme.endsWith('"')) { | ||
localStorage.setItem('mdbook-theme', theme.slice(1, theme.length - 1)); | ||
} | ||
|
||
if (sidebar.startsWith('"') && sidebar.endsWith('"')) { | ||
if (sidebar && sidebar.startsWith('"') && sidebar.endsWith('"')) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These changes seem unrelated to the PR? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. they were null reference bugs I ran into while testing this PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would prefer unrelated changes to not be included. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is a bit different as I couldn't get the tests to pass without this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Which tests aren't passing? What kind of errors are you seeing? I don't believe there are any javascript tests, so it is not clear to me how this could have any impact on anything. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmmm, trying to repro it I can't find it any more and I see there's a |
||
localStorage.setItem('mdbook-sidebar', sidebar.slice(1, sidebar.length - 1)); | ||
} | ||
} catch (e) { } | ||
|
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.
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.
fixed