added --no-run option for rustdoc #83857

ABouttefeux · 2021-04-04T19:03:35Z

add --no-run option for rustdoc for compiling doc test but not running them.
Intended for use with --persist-doctests.

rust-highfive · 2021-04-04T19:03:37Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @CraftSpider (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

src/librustdoc/doctest.rs

src/librustdoc/lib.rs

Co-authored-by: Ivan Tham <[email protected]>

src/librustdoc/doctest.rs

pickfire · 2021-04-06T16:08:10Z

src/librustdoc/config.rs

@@ -121,6 +121,8 @@ crate struct Options {
    /// For example, using ignore-foo to ignore running the doctest on any target that
    /// contains "foo" as a substring
    crate enable_per_target_ignores: bool,
+    /// Compile test but do not run them.


Maybe we could tell others that this is the opposite of should_test?

This is not the opposite, they do different things (and no_run requires should_test to also be set).

pickfire

Looks good to me (but not sure if it's working).

src/librustdoc/lib.rs

src/librustdoc/config.rs

pickfire · 2021-04-08T14:48:55Z

src/test/rustdoc-ui/no-run-flag.stdout

+test $DIR/no-run-flag.rs - f (line 11) ... ok
+test $DIR/no-run-flag.rs - f (line 14) ... ignored
+test $DIR/no-run-flag.rs - f (line 17) ... ok
+test $DIR/no-run-flag.rs - f (line 23) ... ok
+test $DIR/no-run-flag.rs - f (line 28) ... ok
+test $DIR/no-run-flag.rs - f (line 32) ... ok
+test $DIR/no-run-flag.rs - f (line 8) ... ok


If it didn't run why is it "ok"? IIRC that is the same as test passing right?

so adding the --no-run flag change the behavior of the test like it was annotated as ```no_run. Maybe this is unwanted and something else should be displayed.

Ping @CraftSpider @jyn514 should we have a different text? "ok" may be confusing since it didn't run. Also note that we may also want a different color or the same color as "ignored".

Hm. I don't think 'ignored' is quite right, but 'ok' is also a bit confusing. If we can make it something new, maybe something like 'built' or 'checked'? If not, I think 'ok' is more accurate than 'ignored'.
Color-wise, I'm fine with it being ok colored, as it is a requested change to behavior, not implicit, so it's requested to treat it as success. If that makes sense.

After investigations I think that in order to do so, I have to modify the lib test. This will also affect all tests and not just doctests. Do you think this is a worthwhile investment and should this be in a different PR ? Anyway I would be interested in implementing that but the scope is a bit bigger than I anticipated.

no_run by itself is useless.
To summarize:

if you run rustdoc -Z unstable-options --no-run it just build the documentation, it has the same behavior as just rustdoc

rustdoc -Z unstable-options --no-run --test run the doctest set. It behave as if every test had the no_run meaning they won't run. Meaning that test like
```
panic!()
```
will print as ok like
```no_run
panic!()
```
without the --no-run flag

I agree that is confusing. Maybe a error should be raised if no_run is active and not should_test. However I think that if you run rustdoc -Z unstable-options --no-run your intention was probably to run the test instead of documenting your project.

I assume that in the original issue the final goal would be to be able to generate bin (with --persist-doctests) but without running them.

Or maybe a another possible case would be to unify the behavior of cargo test --no-run (provided that down the line cargo is also modified). For instance cargo test --all test both regular test and doctest but cargo test --all --no-run skips doctest cargo test --doc --no-run give an error.

Now if the explicit goal is to harmonize with regular test the output should just be empty.
Another option would for me to change the test lib such that no_run test display something like test foo ... checked, provided that such change is desirable (to discuss thought the RFC process ?).
Moreover test with compile_fail should also print something different maybe checked is also desirable.

I am new contributing for rust so I am not quite sure of what to do to solve this issue.

I agree that is confusing. Maybe a error should be raised if no_run is active and not should_test. However I think that if you run rustdoc -Z unstable-options --no-run your intention was probably to run the test instead of documenting your project.

I think we should give a hard error here; if people meant to run the test they can add --test. Could you suggest adding --test in the error message?

See

rust/src/librustdoc/config.rs

Lines 587 to 593 in 202659a

} else if !out_fmt.is_json() && show_coverage {

diag.struct_err(

"html output format isn't supported for the --show-coverage option",

)

.emit();

return Err(1);

}

for an example of how to give an error if the options are inconsistent.

I think all your other changes can go in a follow-up PR, I don't think we should be changing libtest here.

Ok thank you. I have not change libtest yet I will work on that too :)

Please do not change libtest in this pr. I am not a maintainer and don't know who to ask to approve; finding out will delay landing this.

Sorry I was not clear. I will do another PR for libtest

JohnCSimon · 2021-04-26T05:35:14Z

Ping from triage:
@ABouttefeux assigning back to author, thanks.
@rustbot label: -S-waiting-on-review +S-waiting-on-author

ABouttefeux · 2021-04-26T11:36:48Z

At this moment it is unclear for me what is wanted. Could someone clarify this please ?

jyn514

Looks great, just a couple nits :) r=me with those addressed.

src/librustdoc/config.rs

src/test/rustdoc-ui/no-run-flag-error.rs

Co-authored-by: Joshua Nelson <[email protected]>

jyn514 · 2021-05-01T14:43:54Z

@bors r+

bors · 2021-05-01T14:43:55Z

📌 Commit 03c710b has been approved by jyn514

bors · 2021-05-01T15:36:27Z

⌛ Testing commit 03c710b with merge 5f304a5...

bors · 2021-05-01T18:03:19Z

☀️ Test successful - checks-actions
Approved by: jyn514
Pushing 5f304a5 to master...

Show test type during prints Test output can sometimes be confusing. For example doctest with the no_run argument are displayed the same way than test that are run. During rust-lang#83857 I got the feedback that test output can be confusing. For the moment test output is ``` test $DIR/test-type.rs - f (line 12) ... ignored test $DIR/test-type.rs - f (line 15) ... ok test $DIR/test-type.rs - f (line 21) ... ok test $DIR/test-type.rs - f (line 6) ... ok ``` I propose to change output by indicating the test type as ``` test $DIR/test-type.rs - f (line 12) ... ignored test $DIR/test-type.rs - f (line 15) - compile ... ok test $DIR/test-type.rs - f (line 21) - compile fail ... ok test $DIR/test-type.rs - f (line 6) ... ok ``` by indicating the test type after the test name (and in the case of doctest after the function name and line) and before the "...". ------------ Note: this is a proof of concept, the implementation is probably not optimal as the properties added in `TestDesc` are only use in the display and does not represent actual change of behavior, maybe `TestType::DocTest` could have fields

added --no-run option

db6a916

rust-highfive assigned CraftSpider Apr 4, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 4, 2021

ABouttefeux changed the title ~~added --no-run option~~ added --no-run option for rustdoc Apr 4, 2021

pickfire reviewed Apr 5, 2021

View reviewed changes

src/librustdoc/doctest.rs Outdated Show resolved Hide resolved

pickfire reviewed Apr 5, 2021

View reviewed changes

src/librustdoc/lib.rs Outdated Show resolved Hide resolved

ABouttefeux and others added 2 commits April 5, 2021 17:53

Update src/librustdoc/lib.rs from suggestion

0f3efe2

Co-authored-by: Ivan Tham <[email protected]>

suggestion from review move no run option in run_test call

a6bf81b

This comment has been minimized.

Sign in to view

ABouttefeux and others added 2 commits April 5, 2021 21:11

fix failing build

72f534a

Merge branch 'master' into master

b08b484

pickfire reviewed Apr 6, 2021

View reviewed changes

src/librustdoc/doctest.rs Outdated Show resolved Hide resolved

correction based on review

08c7f97

pickfire reviewed Apr 6, 2021

View reviewed changes

pickfire approved these changes Apr 6, 2021

View reviewed changes

CraftSpider reviewed Apr 6, 2021

View reviewed changes

src/librustdoc/lib.rs Outdated Show resolved Hide resolved

ABouttefeux added 2 commits April 7, 2021 15:45

change based on reviews

3ad3597

add test

4770446

This comment has been minimized.

Sign in to view

pickfire reviewed Apr 7, 2021

View reviewed changes

src/librustdoc/config.rs Show resolved Hide resolved

fix failing tidy check

202659a

pickfire reviewed Apr 8, 2021

View reviewed changes

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 Apr 26, 2021

jyn514 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 Apr 26, 2021

error when --no-run is present without --test

3273d2f

jyn514 approved these changes Apr 30, 2021

View reviewed changes

src/librustdoc/config.rs Outdated Show resolved Hide resolved

src/test/rustdoc-ui/no-run-flag-error.rs Show resolved Hide resolved

Apply suggestions from code review

03c710b

Co-authored-by: Joshua Nelson <[email protected]>

ABouttefeux force-pushed the master branch from abd6dc0 to 03c710b Compare May 1, 2021 09:55

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 1, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label May 1, 2021

bors merged commit 5f304a5 into rust-lang:master May 1, 2021

rustbot added this to the 1.54.0 milestone May 1, 2021

This was referenced May 1, 2021

Generate links to definition in rustdoc source code pages #84176

Merged

rustdoc: Add unstable CLI option to show basic type layout information #83501

Merged

Add --no-source option flag to rustdoc #75522

Closed

ABouttefeux mentioned this pull request May 3, 2021

Show test type during prints #84863

Merged

jyn514 mentioned this pull request Jul 10, 2021

rustdoc: Tracking issue for --no-run #87022

Open

7 tasks

jyn514 mentioned this pull request Aug 19, 2021

rustdoc --test --preserve-doctests --no-run produces empty non-executable files #88110

Closed

lcnr mentioned this pull request Jul 28, 2023

discard default auto trait impls if explicit ones exist (rebase of #85048) #113312

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added --no-run option for rustdoc #83857

added --no-run option for rustdoc #83857

ABouttefeux commented Apr 4, 2021 •

edited

Loading

rust-highfive commented Apr 4, 2021

This comment has been minimized.

pickfire Apr 6, 2021

jyn514 Apr 26, 2021

pickfire left a comment

This comment has been minimized.

pickfire Apr 8, 2021 •

edited

Loading

ABouttefeux Apr 8, 2021

pickfire Apr 8, 2021 •

edited

Loading

CraftSpider Apr 8, 2021

ABouttefeux Apr 9, 2021 •

edited

Loading

ABouttefeux Apr 9, 2021

jyn514 Apr 26, 2021

ABouttefeux Apr 30, 2021

jyn514 Apr 30, 2021

ABouttefeux Apr 30, 2021

JohnCSimon commented Apr 26, 2021

ABouttefeux commented Apr 26, 2021

jyn514 left a comment

jyn514 commented May 1, 2021

bors commented May 1, 2021

bors commented May 1, 2021

bors commented May 1, 2021

	} else if !out_fmt.is_json() && show_coverage {
	diag.struct_err(
	"html output format isn't supported for the --show-coverage option",
	)
	.emit();
	return Err(1);
	}

added --no-run option for rustdoc #83857

added --no-run option for rustdoc #83857

Conversation

ABouttefeux commented Apr 4, 2021 • edited Loading

rust-highfive commented Apr 4, 2021

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pickfire left a comment

Choose a reason for hiding this comment

This comment has been minimized.

pickfire Apr 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pickfire Apr 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ABouttefeux Apr 9, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohnCSimon commented Apr 26, 2021

ABouttefeux commented Apr 26, 2021

jyn514 left a comment

Choose a reason for hiding this comment

jyn514 commented May 1, 2021

bors commented May 1, 2021

bors commented May 1, 2021

bors commented May 1, 2021

ABouttefeux commented Apr 4, 2021 •

edited

Loading

pickfire Apr 8, 2021 •

edited

Loading

pickfire Apr 8, 2021 •

edited

Loading

ABouttefeux Apr 9, 2021 •

edited

Loading