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 option to display warnings in rustdoc #41678

Merged
merged 2 commits into from
May 6, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/librustdoc/core.rs
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,8 @@ pub fn run_core(search_paths: SearchPaths,
externs: config::Externs,
input: Input,
triple: Option<String>,
maybe_sysroot: Option<PathBuf>) -> (clean::Crate, RenderInfo)
maybe_sysroot: Option<PathBuf>,
allow_warnings: bool) -> (clean::Crate, RenderInfo)
{
// Parse, resolve, and typecheck the given crate.

Expand All @@ -119,7 +120,7 @@ pub fn run_core(search_paths: SearchPaths,
maybe_sysroot: maybe_sysroot,
search_paths: search_paths,
crate_types: vec![config::CrateTypeRlib],
lint_opts: vec![(warning_lint, lint::Allow)],
lint_opts: if !allow_warnings { vec![(warning_lint, lint::Allow)] } else { vec![] },
lint_cap: Some(lint::Allow),
externs: externs,
target_triple: triple.unwrap_or(config::host_triple().to_string()),
Expand Down
11 changes: 8 additions & 3 deletions src/librustdoc/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,7 @@ pub fn opts() -> Vec<RustcOptGroup> {
or `#![doc(html_playground_url=...)]`",
"URL")),
unstable(optflag("", "enable-commonmark", "to enable commonmark doc rendering/testing")),
unstable(optflag("", "display-warnings", "to print code warnings when testing doc")),
Copy link
Member

Choose a reason for hiding this comment

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

Can you open a tracking issue for this as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean besides the one already opened? (in the PR description)

Copy link
Member

Choose a reason for hiding this comment

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

Yes that can suffice, but then this PR should not close that issue.

]
}

Expand Down Expand Up @@ -279,14 +280,16 @@ pub fn main_args(args: &[String]) -> isize {
let crate_name = matches.opt_str("crate-name");
let playground_url = matches.opt_str("playground-url");
let maybe_sysroot = matches.opt_str("sysroot").map(PathBuf::from);
let display_warnings = matches.opt_present("display-warnings");

match (should_test, markdown_input) {
(true, true) => {
return markdown::test(input, cfgs, libs, externs, test_args, maybe_sysroot, render_type)
return markdown::test(input, cfgs, libs, externs, test_args, maybe_sysroot, render_type,
display_warnings)
}
(true, false) => {
return test::run(input, cfgs, libs, externs, test_args, crate_name, maybe_sysroot,
render_type)
render_type, display_warnings)
}
(false, true) => return markdown::render(input,
output.unwrap_or(PathBuf::from("doc")),
Expand Down Expand Up @@ -388,13 +391,15 @@ where R: 'static + Send, F: 'static + Send + FnOnce(Output) -> R {

let cr = PathBuf::from(cratefile);
info!("starting to run rustc");
let display_warnings = matches.opt_present("display-warnings");

let (tx, rx) = channel();
rustc_driver::monitor(move || {
use rustc::session::config::Input;

let (mut krate, renderinfo) =
core::run_core(paths, cfgs, externs, Input::File(cr), triple, maybe_sysroot);
core::run_core(paths, cfgs, externs, Input::File(cr), triple, maybe_sysroot,
display_warnings);

info!("finished with rustc");

Expand Down
5 changes: 4 additions & 1 deletion src/librustdoc/markdown.rs
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ pub fn render(input: &str, mut output: PathBuf, matches: &getopts::Matches,
/// Run any tests/code examples in the markdown file `input`.
pub fn test(input: &str, cfgs: Vec<String>, libs: SearchPaths, externs: Externs,
mut test_args: Vec<String>, maybe_sysroot: Option<PathBuf>,
render_type: RenderType) -> isize {
render_type: RenderType, display_warnings: bool) -> isize {
let input_str = match load_string(input) {
Ok(s) => s,
Err(LoadStringError::ReadFail) => return 1,
Expand All @@ -166,6 +166,9 @@ pub fn test(input: &str, cfgs: Vec<String>, libs: SearchPaths, externs: Externs,
old_find_testable_code(&input_str, &mut collector, DUMMY_SP);
find_testable_code(&input_str, &mut collector, DUMMY_SP);
test_args.insert(0, "rustdoctest".to_string());
if display_warnings {
test_args.insert(1, "--display-stdout".to_string());
Copy link
Member

Choose a reason for hiding this comment

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

How come this was added? I thought rustc output went to stderr?

Copy link
Member Author

Choose a reason for hiding this comment

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

The name of the tag is up to debate. I'm using it to make the difference between warnings and errors, but it's clearly misleading. What name should we give it?

Copy link
Member

Choose a reason for hiding this comment

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

I think I may not even understand what this is doing. What is it doing?

Copy link
Member Author

Choose a reason for hiding this comment

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

So for now, when running rustdoc --test, warnings aren't displayed. Some people want them to be displayed anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Then why not call it --display-warnings

Copy link
Member

Choose a reason for hiding this comment

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

@GuillaumeGomez that's not too helpful of a description, though? You're basically just restating the title and description of this PR.

This flag is implemented inside of libtest which means that it has nothing to do with doctest warnings, and is an instantly stable interface to all test binaries that we generate, hence my hesitation. If we can avoid it that would be great, but I do not understand what this flag is doing inside of the test harness.

Can you explain what's going on inside of libtest to why this is being passed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Aaaaah. My bad. So basically, in libtest, if I have the display-stdout option enabled, for every non-failing test, I display what the test sent to the output.

Copy link
Member

Choose a reason for hiding this comment

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

Can this be done without exposing a new flag from libtest? Just by modifying internal data structures or using a custom api from libtest?

}
testing::test_main(&test_args, collector.tests);
0
}
6 changes: 5 additions & 1 deletion src/librustdoc/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,8 @@ pub fn run(input: &str,
mut test_args: Vec<String>,
crate_name: Option<String>,
maybe_sysroot: Option<PathBuf>,
render_type: RenderType)
render_type: RenderType,
display_warnings: bool)
-> isize {
let input_path = PathBuf::from(input);
let input = config::Input::File(input_path.clone());
Expand Down Expand Up @@ -125,6 +126,9 @@ pub fn run(input: &str,
}

test_args.insert(0, "rustdoctest".to_string());
if display_warnings {
test_args.insert(1, "--display-stdout".to_string());
}

testing::test_main(&test_args,
collector.tests.into_iter().collect());
Expand Down
49 changes: 46 additions & 3 deletions src/libtest/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -304,13 +304,14 @@ pub fn test_main_static(tests: &[TestDescAndFn]) {
test_main(&args, owned_tests)
}

#[derive(Copy, Clone)]
#[derive(Copy, Clone, Debug)]
pub enum ColorConfig {
AutoColor,
AlwaysColor,
NeverColor,
}

#[derive(Debug)]
pub struct TestOpts {
pub list: bool,
pub filter: Option<String>,
Expand All @@ -324,6 +325,7 @@ pub struct TestOpts {
pub quiet: bool,
pub test_threads: Option<usize>,
pub skip: Vec<String>,
pub display_stdout: bool,
}

impl TestOpts {
Expand All @@ -342,6 +344,7 @@ impl TestOpts {
quiet: false,
test_threads: None,
skip: vec![],
display_stdout: false,
}
}
}
Expand Down Expand Up @@ -369,7 +372,8 @@ fn optgroups() -> Vec<getopts::OptGroup> {
getopts::optopt("", "color", "Configure coloring of output:
auto = colorize if stdout is a tty and tests are run on serially (default);
always = always colorize output;
never = never colorize output;", "auto|always|never")]
never = never colorize output;", "auto|always|never"),
getopts::optflag("", "display-stdout", "to print stdout even if the test succeeds")]
}

fn usage(binary: &str) {
Expand Down Expand Up @@ -481,6 +485,7 @@ pub fn parse_opts(args: &[String]) -> Option<OptRes> {
quiet: quiet,
test_threads: test_threads,
skip: matches.opt_strs("skip"),
display_stdout: matches.opt_present("display-stdout"),
};

Some(Ok(test_opts))
Expand Down Expand Up @@ -521,7 +526,9 @@ struct ConsoleTestState<T> {
measured: usize,
metrics: MetricMap,
failures: Vec<(TestDesc, Vec<u8>)>,
not_failures: Vec<(TestDesc, Vec<u8>)>,
max_name_len: usize, // number of columns to fill when aligning names
display_stdout: bool,
}

impl<T: Write> ConsoleTestState<T> {
Expand All @@ -547,7 +554,9 @@ impl<T: Write> ConsoleTestState<T> {
measured: 0,
metrics: MetricMap::new(),
failures: Vec::new(),
not_failures: Vec::new(),
max_name_len: 0,
display_stdout: opts.display_stdout,
})
}

Expand Down Expand Up @@ -703,9 +712,38 @@ impl<T: Write> ConsoleTestState<T> {
Ok(())
}

pub fn write_outputs(&mut self) -> io::Result<()> {
self.write_plain("\nsuccesses:\n")?;
let mut successes = Vec::new();
let mut stdouts = String::new();
for &(ref f, ref stdout) in &self.not_failures {
successes.push(f.name.to_string());
if !stdout.is_empty() {
stdouts.push_str(&format!("---- {} stdout ----\n\t", f.name));
let output = String::from_utf8_lossy(stdout);
stdouts.push_str(&output);
stdouts.push_str("\n");
}
}
if !stdouts.is_empty() {
self.write_plain("\n")?;
self.write_plain(&stdouts)?;
}

self.write_plain("\nsuccesses:\n")?;
successes.sort();
for name in &successes {
self.write_plain(&format!(" {}\n", name))?;
}
Ok(())
}

pub fn write_run_finish(&mut self) -> io::Result<bool> {
assert!(self.passed + self.failed + self.ignored + self.measured == self.total);

if self.display_stdout {
self.write_outputs()?;
}
let success = self.failed == 0;
if !success {
self.write_failures()?;
Expand Down Expand Up @@ -824,7 +862,10 @@ pub fn run_tests_console(opts: &TestOpts, tests: Vec<TestDescAndFn>) -> io::Resu
st.write_log_result(&test, &result)?;
st.write_result(&result)?;
match result {
TrOk => st.passed += 1,
TrOk => {
st.passed += 1;
st.not_failures.push((test, stdout));
}
TrIgnored => st.ignored += 1,
TrMetrics(mm) => {
let tname = test.name;
Expand Down Expand Up @@ -901,6 +942,8 @@ fn should_sort_failures_before_printing_them() {
max_name_len: 10,
metrics: MetricMap::new(),
failures: vec![(test_b, Vec::new()), (test_a, Vec::new())],
display_stdout: false,
not_failures: Vec::new(),
};

st.write_failures().unwrap();
Expand Down
1 change: 1 addition & 0 deletions src/tools/compiletest/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,7 @@ pub fn test_opts(config: &Config) -> test::TestOpts {
test_threads: None,
skip: vec![],
list: false,
display_stdout: false,
}
}

Expand Down