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.

Sign up for GitHub

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

Jump to bottom

Add rustdoc JS non-std tests #58330

Merged
merged 7 commits into from
Mar 12, 2019
Merged

Add rustdoc JS non-std tests #58330

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
834347d
Add rustdoc JS non-std tests
GuillaumeGomez Feb 9, 2019
aa3ca32
Rename rustdoc js test suites
GuillaumeGomez Feb 23, 2019
be23cd9
Move documentation build into bootstrap
GuillaumeGomez Feb 24, 2019
240fad0
Update to last updates
GuillaumeGomez Feb 25, 2019
405d950
Move rustdoc-js testing into compiletest
GuillaumeGomez Feb 28, 2019
d6add90
Improve code
GuillaumeGomez Mar 6, 2019
37ab3dc
Make js tests work even with resource-suffix option
GuillaumeGomez Mar 10, 2019
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
62 changes: 13 additions & 49 deletions src/bootstrap/test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
//! our CI.

use std::env;
use std::ffi::{OsStr, OsString};
use std::ffi::OsString;
use std::fmt;
use std::fs;
use std::iter;
Expand Down Expand Up @@ -638,52 +638,15 @@ impl Step for RustdocJSNotStd {
}

fn run(self, builder: &Builder<'_>) {
if let Some(ref nodejs) = builder.config.nodejs {
builder.ensure(crate::doc::Std {
if builder.config.nodejs.is_some() {
builder.ensure(Compiletest {
compiler: self.compiler,
target: self.target,
stage: builder.top_stage,
mode: "js-doc-test",
suite: "rustdoc-js",
path: None,
compare_mode: None,
});

let mut tests_to_run = Vec::new();
let out = Path::new("build").join(&*self.host)
.join(&format!("stage{}",
builder.top_stage.to_string().as_str()))
.join("tests")
.join("rustdoc-js");

if let Ok(it) = fs::read_dir("src/test/rustdoc-js/") {
for entry in it {
if let Ok(entry) = entry {
let path = entry.path();
if path.extension() != Some(&OsStr::new("rs")) || !path.is_file() {
continue
}
let path_clone = path.clone();
let file_stem = path_clone.file_stem().expect("cannot get file stem");
let out = out.join(file_stem);
let mut cmd = builder.rustdoc_cmd(self.host);
cmd.arg("-o");
cmd.arg(out);
cmd.arg(path);
if if builder.config.verbose_tests {
try_run(builder, &mut cmd)
} else {
try_run_quiet(builder, &mut cmd)
} {
tests_to_run.push(file_stem.to_os_string());
}
}
}
}
assert!(!tests_to_run.is_empty(), "no rustdoc-js test generated...");

tests_to_run.insert(0, "src/tools/rustdoc-js/tester.js".into());
tests_to_run.insert(1, out.into());

let mut command = Command::new(nodejs);
command.args(&tests_to_run);

builder.run(&mut command);
} else {
builder.info(
"No nodejs found, skipping \"src/test/rustdoc-js\" tests"
Expand Down Expand Up @@ -1070,12 +1033,13 @@ impl Step for Compiletest {
.arg(builder.sysroot_libdir(compiler, target));
cmd.arg("--rustc-path").arg(builder.rustc(compiler));

let is_rustdoc_ui = suite.ends_with("rustdoc-ui");
let is_rustdoc = suite.ends_with("rustdoc-ui") || suite.ends_with("rustdoc-js");
Copy link
Member

Choose a reason for hiding this comment

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

It feels like this isn't needed? The compiletest invocation above is has the mode as js-doc-test, which you've also checked below.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's more convenient to check for instance:

let mut flags = if is_rustdoc {
    Vec::new()
} else {
    vec!["-Crpath".to_string()]
};

or:

if !is_rustdoc {
    if builder.config.rust_optimize_tests {
        flags.push("-O".to_string());
    }
    if builder.config.rust_debuginfo_tests {
        flags.push("-g".to_string());
    }
}


// Avoid depending on rustdoc when we don't need it.
if mode == "rustdoc"
|| (mode == "run-make" && suite.ends_with("fulldeps"))
|| (mode == "ui" && is_rustdoc_ui)
|| (mode == "ui" && is_rustdoc)
|| mode == "js-doc-test"
{
cmd.arg("--rustdoc-path")
.arg(builder.rustdoc(compiler.host));
Expand Down Expand Up @@ -1109,12 +1073,12 @@ impl Step for Compiletest {
cmd.arg("--nodejs").arg(nodejs);
}

let mut flags = if is_rustdoc_ui {
let mut flags = if is_rustdoc {
Vec::new()
} else {
vec!["-Crpath".to_string()]
};
if !is_rustdoc_ui {
if !is_rustdoc {
if builder.config.rust_optimize_tests {
flags.push("-O".to_string());
}
Expand Down
1 change: 1 addition & 0 deletions src/test/rustdoc-js/basic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
/// Foo
pub struct Foo;
3 changes: 3 additions & 0 deletions src/tools/compiletest/src/common.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ pub enum Mode {
Incremental,
RunMake,
Ui,
JsDocTest,
MirOpt,
}

Expand Down Expand Up @@ -59,6 +60,7 @@ impl FromStr for Mode {
"incremental" => Ok(Incremental),
"run-make" => Ok(RunMake),
"ui" => Ok(Ui),
"js-doc-test" => Ok(JsDocTest),
"mir-opt" => Ok(MirOpt),
_ => Err(()),
}
Expand All @@ -82,6 +84,7 @@ impl fmt::Display for Mode {
Incremental => "incremental",
RunMake => "run-make",
Ui => "ui",
JsDocTest => "js-doc-test",
MirOpt => "mir-opt",
};
fmt::Display::fmt(s, f)
Expand Down
29 changes: 25 additions & 4 deletions src/tools/compiletest/src/runtest.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use crate::common::{output_base_dir, output_base_name, output_testname_unique};
use crate::common::{Codegen, CodegenUnits, DebugInfoBoth, DebugInfoGdb, DebugInfoLldb, Rustdoc};
use crate::common::{CompileFail, Pretty, RunFail, RunPass, RunPassValgrind};
use crate::common::{Config, TestPaths};
use crate::common::{Incremental, MirOpt, RunMake, Ui};
use crate::common::{Incremental, MirOpt, RunMake, Ui, JsDocTest};
use diff;
use crate::errors::{self, Error, ErrorKind};
use filetime::FileTime;
Expand Down Expand Up @@ -275,6 +275,7 @@ impl<'test> TestCx<'test> {
RunMake => self.run_rmake_test(),
RunPass | Ui => self.run_ui_test(),
MirOpt => self.run_mir_opt_test(),
JsDocTest => self.run_js_doc_test(),
}
}

Expand All @@ -290,7 +291,7 @@ impl<'test> TestCx<'test> {
fn should_compile_successfully(&self) -> bool {
match self.config.mode {
CompileFail => self.props.compile_pass,
RunPass => true,
RunPass | JsDocTest => true,
Copy link
Member

Choose a reason for hiding this comment

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

Instead of the | could we combine this into separate arms?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure!

Ui => self.props.compile_pass,
Incremental => {
let revision = self.revision
Expand Down Expand Up @@ -1712,7 +1713,8 @@ impl<'test> TestCx<'test> {
}

fn make_compile_args(&self, input_file: &Path, output_file: TargetLocation) -> Command {
let is_rustdoc = self.config.src_base.ends_with("rustdoc-ui");
let is_rustdoc = self.config.src_base.ends_with("rustdoc-ui") ||
self.config.src_base.ends_with("rustdoc-js");
let mut rustc = if !is_rustdoc {
Command::new(&self.config.rustc_path)
} else {
Expand Down Expand Up @@ -1802,7 +1804,7 @@ impl<'test> TestCx<'test> {
rustc.arg(dir_opt);
}
RunFail | RunPassValgrind | Pretty | DebugInfoBoth | DebugInfoGdb | DebugInfoLldb
| Codegen | Rustdoc | RunMake | CodegenUnits => {
| Codegen | Rustdoc | RunMake | CodegenUnits | JsDocTest => {
// do not use JSON output
}
}
Expand Down Expand Up @@ -2710,6 +2712,25 @@ impl<'test> TestCx<'test> {
fs::remove_dir(path)
}

fn run_js_doc_test(&self) {
if let Some(nodejs) = &self.config.nodejs {
Copy link
Member

Choose a reason for hiding this comment

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

Is self.config loaded from config.toml? I'm wondering how the path to the node executable is set.

Copy link
Member Author

Choose a reason for hiding this comment

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

I took the same code I used previously. But iirc, it is from the config.toml file.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, and checked in bootstrap (this is a config "passed through" from bootstrap).

Copy link
Member

Choose a reason for hiding this comment

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

Can we make this call self.fatal("no nodeJS") or some such? If we're in compiletest we should fail if we don't have a nodeJS here I think

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok!

let out_dir = self.output_base_dir();

self.document(&out_dir);

let root = self.config.find_rust_src_root().unwrap();
let res = self.cmd2procres(
Command::new(&nodejs)
.arg(root.join("src/tools/rustdoc-js/tester.js"))
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be rustdoc-js-std?

Either way, I'm confused why we've only migrated one of the suites and not both to compiletest?

Copy link
Member Author

@GuillaumeGomez GuillaumeGomez Mar 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

The std one doesn't require to run rustdoc, just to run some JS. So it's not really required on this side.

.arg(out_dir.parent().expect("no parent"))
.arg(&self.testpaths.file.file_stem().expect("couldn't get file stem")),
);
if !res.status.success() {
self.fatal_proc_rec("rustdoc-js test failed!", &res);
}
}
}

fn run_ui_test(&self) {
// if the user specified a format in the ui test
// print the output to the stderr file, otherwise extract
Expand Down