add a --chapter option to mdbook test. #1741

clovett · 2022-02-09T21:48:47Z

Sometimes when working on large books it is handy to be able to run mdbook on a single chapter of a book. For example:

mdbook test --chapter build

mdbook test --chapter build
2022-02-09 13:48:17 [INFO] (mdbook::book): Skipping chapter 'Introduction'...
2022-02-09 13:48:17 [INFO] (mdbook::book): Skipping chapter 'Installation'...
2022-02-09 13:48:17 [INFO] (mdbook::book): Skipping chapter 'Reading Books'...
2022-02-09 13:48:17 [INFO] (mdbook::book): Skipping chapter 'Creating a Book'...
2022-02-09 13:48:17 [INFO] (mdbook::book): Skipping chapter 'Command Line Tool'...
2022-02-09 13:48:17 [INFO] (mdbook::book): Skipping chapter 'init'...
2022-02-09 13:48:17 [INFO] (mdbook::book): Testing chapter 'build': "d:\\git\\clovett\\mdBook\\guide\\src\\cli/build.md"
2022-02-09 13:48:17 [INFO] (mdbook::book): Skipping chapter 'watch'...
2022-02-09 13:48:17 [INFO] (mdbook::book): Skipping chapter 'serve'...
2022-02-09 13:48:17 [INFO] (mdbook::book): Skipping chapter 'test'...
2022-02-09 13:48:18 [INFO] (mdbook::book): Skipping chapter 'clean'...
2022-02-09 13:48:18 [INFO] (mdbook::book): Skipping chapter 'completions'...
2022-02-09 13:48:18 [INFO] (mdbook::book): Skipping chapter 'Format'...
2022-02-09 13:48:18 [INFO] (mdbook::book): Skipping chapter 'SUMMARY.md'...
2022-02-09 13:48:18 [INFO] (mdbook::book): Skipping chapter 'Configuration'...
2022-02-09 13:48:18 [INFO] (mdbook::book): Skipping chapter 'General'...
2022-02-09 13:48:18 [INFO] (mdbook::book): Skipping chapter 'Preprocessors'...
2022-02-09 13:48:18 [INFO] (mdbook::book): Skipping chapter 'Renderers'...
2022-02-09 13:48:18 [INFO] (mdbook::book): Skipping chapter 'Environment Variables'...
2022-02-09 13:48:18 [INFO] (mdbook::book): Skipping chapter 'Theme'...
2022-02-09 13:48:18 [INFO] (mdbook::book): Skipping chapter 'index.hbs'...
2022-02-09 13:48:18 [INFO] (mdbook::book): Skipping chapter 'Syntax highlighting'...
2022-02-09 13:48:18 [INFO] (mdbook::book): Skipping chapter 'Editor'...
2022-02-09 13:48:18 [INFO] (mdbook::book): Skipping chapter 'MathJax Support'...
2022-02-09 13:48:18 [INFO] (mdbook::book): Skipping chapter 'mdBook-specific features'...
2022-02-09 13:48:18 [INFO] (mdbook::book): Skipping chapter 'Markdown'...
2022-02-09 13:48:18 [INFO] (mdbook::book): Skipping chapter 'Continuous Integration'...
2022-02-09 13:48:18 [INFO] (mdbook::book): Skipping chapter 'For Developers'...
2022-02-09 13:48:18 [INFO] (mdbook::book): Skipping chapter 'Preprocessors'...
2022-02-09 13:48:18 [INFO] (mdbook::book): Skipping chapter 'Alternative Backends'...
2022-02-09 13:48:18 [INFO] (mdbook::book): Skipping chapter 'Contributors'...
```

…ic to the name of the rustdoc tool.

ehuss

Thanks! This seems like it could be useful.

I'd like to suggest that perhaps the -c flag could take either a chapter name or file path. In most cases, I know the file path but not the chapter name. File paths also support command-line completion, whereas chapter names need to be typed exactly (and deal with shell quoting and such).

Can you also update the documentation at https://github.com/rust-lang/mdBook/blob/master/guide/src/cli/test.md?

src/book/mod.rs

ehuss · 2022-03-27T23:45:19Z

src/theme/index.hbs

+                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('"')) {


These changes seem unrelated to the PR?

they were null reference bugs I ran into while testing this PR.

I would prefer unrelated changes to not be included.

this is a bit different as I couldn't get the tests to pass without this.

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.

Hmmm, trying to repro it I can't find it any more and I see there's a try{ } catch (e) { } around this whole thing, so it's possible I caught the exception in the debugger and mistook it for a bug when it was expecting the try/catch block to catch it and continue. So I reverted these changes.

lovettchris · 2022-05-03T00:13:59Z

I'd like to suggest that perhaps the -c flag could take either a chapter name or file path. In most cases, I know the file path but not the chapter name. File paths also support command-line completion, whereas chapter names need to be typed exactly (and deal with shell quoting and such).

Done.

ehuss · 2022-07-16T19:02:45Z

@clovett Just checking in if you are still interested in this. It looks like there are some test failures.

lovettchris · 2022-08-01T18:18:17Z

I am still interested in this PR, but I'm kinda stumped by the test failures. Clearly the test output changes, and I tried to fix that, but I'm not sure why it didn't work.

ehuss · 2022-08-01T22:39:26Z

The PR changes the output to use chapter_path instead of path. path is absolute whereas chapter_path is relative. That means that the pattern Testing chapter [^:]*: "([^"]+)[\\/]README.md needs to be updated, as there is no leading slash in front of the README.md file.

lovettchris · 2022-08-01T23:38:50Z

The PR changes the output to use chapter_path instead of path. path is absolute whereas chapter_path is relative. That means that the pattern Testing chapter [^:]*: "([^"]+)[\\/]README.md needs to be updated, as there is no leading slash in front of the README.md file.

Ah, thanks for the hint - I think I got it now.

clovett · 2022-08-04T05:56:24Z

@ehuss - yay the tests are passing.

ehuss

I think this will need some tests.

Also, can you add some kind of error message if the --chapter value doesn't match anything?

ehuss · 2022-08-09T19:30:57Z

src/book/mod.rs

-    /// 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.


Suggested change

/// and optionally testing only a signal named chapter while skipping the others.

/// and optionally testing only a single named chapter while skipping the others.

ehuss · 2022-08-09T19:32:21Z

src/book/mod.rs

-    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<()> {


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 test call it.

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".

ehuss · 2022-08-09T19:41:36Z

src/book/mod.rs

+                if chapter.is_some()
+                    && ch.name != chapter.unwrap()
+                    && chapter_path.to_str().unwrap() != chapter.unwrap()


I'd prefer to avoid using unwrap here. Perhaps more like this:

Suggested change

if chapter.is_some()

&& ch.name != chapter.unwrap()

&& chapter_path.to_str().unwrap() != chapter.unwrap()

if let Some(chapter) = chapter {

if ch.name != chapter && chapter_path.to_str() != Some(chapter) {

However, this logic seems wrong. It seems to require that the --chapter value passed in equals both the chapter name and its filename. I would expect it to match one or the other. Perhaps something more like?

!(ch.name == chapter || chapter_path.to_str() == Some(chapter))

it works, notice this is the inverse of the test !(ch.name == chapter || chapter_path == chapter)

ehuss · 2022-08-09T19:42:55Z

src/theme/index.hbs

+                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('"')) {


I would prefer unrelated changes to not be included.

test: add unit tests for test_chapter

lovettchris · 2022-08-11T00:17:19Z

Can you also update the documentation at https://github.com/rust-lang/mdBook/blob/master/guide/src/cli/test.md?

I added:

--chapter

The --chapter (-c) option allows you to test a specific chapter of the
book using the chapter name or the relative path to the chapter.

ehuss · 2022-08-13T18:50:59Z

src/book/mod.rs

+                if chapter_name != "" && ch.name != chapter_name && cp != chapter_name {
+                    info!("Skipping chapter '{}'...", ch.name);


This all seemed to have reverted back to using an empty string as a sentinel instead of an Option. Is there any particular reason for that?

sorry this might be my Rust ignorance, I couldn't figure out how to compare chapter : Option<&str> and ch.name: String. Rust complains: "can't compare std::string::String with std::option::Option<&str>". So, moving them both to &str solved the problem, but I guess I could instead convert ch.name to Option<&str> using:

let cn: Option<&str> = Some(&ch.name);

I pushed this change to see if this is what you prefer...

ehuss · 2022-08-13T18:51:39Z

src/book/mod.rs

+        self.test_chapter(library_paths, None)
+    }
+
+    /// Run `rustdoc` tests on a specific chapter of the book, linking against the provided libraries.


This is a little unclear to me on what happens if chapter is None. Can it include a little more explanation?

Sure thing, I added clarifying comments // test_chapter with chapter:None will run all tests. and on the test_chapter function itself the doc comment: /// If chapterisNone, all tests will be run.

…r names.

…vett/chapters

lovettchris · 2022-08-17T17:45:24Z

@ehuss I think I resolved all the excellent issues you raised.

ehuss

Thanks! Just some code style suggestions.

ehuss · 2022-08-24T19:31:39Z

src/book/mod.rs

+                let cp = chapter_path.to_str();
+                let cn: Option<&str> = Some(&ch.name);
+
+                if chapter.is_some() && cn != chapter && cp != chapter {
+                    info!("Skipping chapter '{}'...", ch.name);
+                    continue;
+                };


Instead of using local variables like this, I still think it would be clearer to use something like the suggestion I posted before. The local variables create an indirection where you then have to refer to other parts of the code to figure out what they mean. Using the values directly I think makes it a little clearer as to what is being checked.

Also, after testing this out a bit, I feel like the output is overwhelmed with the "skipping chapter" text. I'm thinking it might be better to reduce that to a debug message to reduce the noise.

Perhaps something like this:

Suggested change

let cp = chapter_path.to_str();

let cn: Option<&str> = Some(&ch.name);

if chapter.is_some() && cn != chapter && cp != chapter {

info!("Skipping chapter '{}'...", ch.name);

continue;

};

if let Some(chapter) = chapter {

if ch.name != chapter && chapter_path.to_str() != Some(chapter) {

debug!("Skipping chapter '{}'...", ch.name);

continue;

}

}

The skipping chapter prompts are how you find the chapter names so you can write mdbook test -c ? for example then you find the chapter name you want to test and run the specific command. But I could make it so that it looks for the specific string "?" so it only prints in that case.

I pushed the if-check looking for command line arg "?" instead and I added that to the help text to make it discoverable.

Oh, and I see now how you are using if let Some to remove the need for local variables, so I pushed that fix too.

ehuss · 2022-08-24T19:34:43Z

src/book/mod.rs

+        if chapter.is_some() && !chapter_found {
+            bail!(format!("Chapter not found: {:?}", chapter));
+        }


This produces some strange output. Generally it is recommended to not use debug displays for user output. Also, the bail! macro can take formatted strings directly, there is no need to use format!. I think it might be better to extract the name through a pattern and use a display impl. Something like this:

Suggested change

if chapter.is_some() && !chapter_found {

bail!(format!("Chapter not found: {:?}", chapter));

}

if let Some(chapter) = chapter {

if !chapter_found {

bail!("Chapter not found: {}", chapter);

}

}

Much better thanks, I merged your fix. I was just copying the use of bail from a few lines earlier: bail!("One or more tests failed");, so unless there is a better error formatter available, I'll leave the bail as you have written it.

ehuss

Thanks!

clovett added 4 commits February 9, 2022 13:45

add a --chapter option to mdbook test.

f0189cd

fix unit tests

4d78119

fix expected output

bf6d452

move command line parsing code and make test output comparison agnost…

71e490d

…ic to the name of the rustdoc tool.

ehuss reviewed Mar 27, 2022

View reviewed changes

ehuss added the S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. label Apr 15, 2022

clovett added 2 commits May 2, 2022 17:11

Merge branch 'master' into clovett/chapters

6e3d275

CR feedback, switch to Option<&str> for chatper parameter.

99a8c0d

Fix tests

f1c07b0

Merge branch 'master' into clovett/chapters

ed0ee85

fix: unit test expected output has changed.

96245d2

fix: fmt --check

7c55869

clovett requested a review from ehuss August 4, 2022 05:55

ehuss reviewed Aug 9, 2022

View reviewed changes

lovettchris and others added 2 commits August 10, 2022 17:06

fix: add test_chapter method so test can remain api compatible.

0926c52

test: add unit tests for test_chapter

doc: add documentation of new --chapter option.

05ed667

clovett added 2 commits August 10, 2022 17:17

doc: fix typo.

601b6df

fix: remove redundant check.

20cca38

ehuss reviewed Aug 13, 2022

View reviewed changes

clovett added 4 commits August 15, 2022 12:56

doc: add clarifying comments.

5ec488c

Merge branch 'master' into clovett/chapters

9172005

fix: revert changes from code review.

c1004f7

fix: cr feedback no need to unwrap Option<&str> when comparing chapte…

e979012

…r names.

Merge branch 'clovett/chapters' of github.com:clovett/mdBook into clo…

d383ae0

…vett/chapters

ehuss reviewed Aug 24, 2022

View reviewed changes

clovett added 2 commits August 24, 2022 13:25

fix: code review comments.

4ead38f

fix: code review cleanup

1747094

ehuss approved these changes Aug 26, 2022

View reviewed changes

ehuss merged commit 74eb405 into rust-lang:master Aug 26, 2022

lovettchris mentioned this pull request Sep 2, 2022

rebase on latest mdbook from https://github.com/rust-lang/mdBook leanprover/mdBook#12

Closed

	/// and optionally testing only a signal named chapter while skipping the others.
	/// and optionally testing only a single named chapter while skipping the others.

		if chapter_name != "" && ch.name != chapter_name && cp != chapter_name {
		info!("Skipping chapter '{}'...", ch.name);

-        if chapter.is_some() && !chapter_found {
-            bail!(format!("Chapter not found: {:?}", chapter));
-        }
+        if let Some(chapter) = chapter {
+            if !chapter_found {
+                bail!("Chapter not found: {}", chapter);
+            }
+        }

add a --chapter option to mdbook test. #1741

add a --chapter option to mdbook test. #1741

Conversation

clovett commented Feb 9, 2022

ehuss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lovettchris commented May 3, 2022

ehuss commented Jul 16, 2022

lovettchris commented Aug 1, 2022

ehuss commented Aug 1, 2022

lovettchris commented Aug 1, 2022

clovett commented Aug 4, 2022

ehuss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lovettchris Aug 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lovettchris commented Aug 11, 2022 • edited Loading

--chapter

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lovettchris commented Aug 17, 2022

ehuss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehuss left a comment

Choose a reason for hiding this comment

lovettchris Aug 11, 2022 •

edited

Loading

lovettchris commented Aug 11, 2022 •

edited

Loading