-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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 JsonDocCk Tool for rustdoc-json #81063
Conversation
The job Click to see the possible cause of the failure (guessed by this bot)
|
The job Click to see the possible cause of the failure (guessed by this bot)
|
87d5933
to
9fe5308
Compare
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.
This looks great :) Can you add a test that intentionally fails and post the error output?
Do you mean add one to git, or just write one up and post the results? |
Just write one up and post the results. |
Failure Output
Note that the system won't display syntax errors at the same time as check errors. I could change this, it was just a bit easier to implement this way. Edit: Note, I didn't yet implement it telling you where exactly the path failed, but I added a FIXME to do it later. The current output is about as informative as htmldocck puts out, so I figured it would be alright to start with this, and add the even better output later. |
src/tools/compiletest/src/main.rs
Outdated
@@ -196,6 +197,7 @@ pub fn parse_config(args: Vec<String>) -> Config { | |||
let has_tidy = Command::new("tidy") | |||
.arg("--version") | |||
.stdout(Stdio::null()) | |||
.stderr(Stdio::null()) |
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.
Does tidy write to stderr on your machine?
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.
Yes, and in fact it should be on everyone's machine. The first 3 lines of tidy/src/main.rs
:
let root_path: PathBuf = env::args_os().nth(1).expect("need path to root of repo").into();
let cargo: PathBuf = env::args_os().nth(2).expect("need path to cargo").into();
let output_directory: PathBuf =
env::args_os().nth(3).expect("need path to output directory").into();
Thinking about it, this check is actually broken and always false, because --version
is entirely ignored and tidy just unconditionally panics. This is another change I can back out here and make into its own PR though, as it is unrelated to the function of the json tests.
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.
@CraftSpider there are two different tidy commands. One is src/tools/tidy
, one is https://www.w3.org/People/Raggett/tidy/. This is checking HTML tidy.
Can you revert this change?
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.
Interesting. It was definitely invoking the one I pointed to, I was getting the full panic. I'll revert the change and look into why.
☔ The latest upstream changes (presumably #81089) made this pull request unmergeable. Please resolve the merge conflicts. |
d11c5af
to
29b7b02
Compare
The job Click to see the possible cause of the failure (guessed by this bot)
|
29b7b02
to
728ffc8
Compare
@bors r+ Thanks so much! |
📌 Commit ba6803e has been approved by |
☀️ Test successful - checks-actions |
@rustbot modify labels +A-rustdoc-json |
Implements a new test system for rustdoc JSON output, jsondocck. Modeled after htmldocck, this tool reads directives in the test file and checks them against the output. These directives use JSONPath, a pair to XPath for json. This obsoletes the old strict subset tool, allowing both finer-grained control of what is tested and better errors on failure.
Not sure on the changes to Cargo.lock, I can back that out if needed.
r? @jyn514