-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
Fix (more) test directives that were accidentally ignored #137540
Fix (more) test directives that were accidentally ignored #137540
Conversation
This PR modifies cc @jieyouxu |
@@ -1,6 +1,5 @@ | |||
// @has foobar/fn.ok.html '//*[@class="docblock scraped-example-list"]//*[@class="prev"]' '' |
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.
@GuillaumeGomez IIUC, this specific directive, checking for the a prev button, was broken by #129796 , and is no longer needed?
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.
The prev button is generated by the JS, so it cannot be checked here indeed.
@@ -1,3 +1,3 @@ | |||
//@ check-pass | |||
//@ aux-crate:panic_item=panic-item.rs | |||
// @has unused_extern_crate/index.html | |||
//####@ has unused_extern_crate/index.html |
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.
Not sure what to do about this one
This test is run in ui
mode, and as such, doesn't recognize any htmldocck
directives...
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.
There are a couple of options, the way I see it:
- Just remove the
has
directive. It hasn't actually been effective in a while, and according to some my limited archeological excavation, seems this test was created in response to an ICE that was solved, socheck-pass
should be enough for making sure this doesn't regress. - Make this a
rustdoc
-mode test? Not sure if it supports auxiliary crates. - Run
htmldocck
forui
-mode tests? Doesn't sound feasible, nor does it make much sense.
Tagging @GuillaumeGomez and @jyn514 because you two seem to have worked on this test/issue (cc #68427 , I think?).
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 should probably have been a rustdoc
test to start, not rustdoc-ui
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.
i would be ok with either moving it to rustdoc
or just deleting the @has
directive. like you said, it's just testing that we don't ICE.
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.
Cool, thanks for taking a look!
Moved the test to rustdoc
, arbitrarily picked one of the two options 😅
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.
Yeah, this one was definitely wrong.
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.
I'm not actually sure what rustdoc-ui
is supposed to do. Last I checked I seem to recall rustdoc-ui
is a Mode::Ui
test suite but it uses rustdoc instead of rustc? 🤔
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.
I'm really new to all this, but AFAIU, yeah, that's exactly what it is.
Seems most tests there are for checking the (error) output of rustdoc
. Not the HTML output.
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.
rustdoc-ui
tests are all tests for the CLI output.
https://rustc-dev-guide.rust-lang.org/tests/compiletest.html#rustdoc-test-suites
These seem like rustdoc tests (modulo the one ui test), so r? rustdoc |
5c5b7d6
to
0881dba
Compare
This PR modifies cc @jieyouxu |
@bors r+ |
…directives, r=notriddle Fix (more) test directives that were accidentally ignored Continuation of rust-lang#137099 , caught by rust-lang#137103 (and needed to unblock that one). These test directives were accidentally using the old (`// `@`)` syntax
…directives, r=notriddle Fix (more) test directives that were accidentally ignored Continuation of rust-lang#137099 , caught by rust-lang#137103 (and needed to unblock that one). These test directives were accidentally using the old (`// ``@`)`` syntax
Rollup of 10 pull requests Successful merges: - rust-lang#134943 (Add FileCheck annotations to mir-opt/issues) - rust-lang#137017 (Don't error when adding a staticlib with bitcode files compiled by newer LLVM) - rust-lang#137197 (Update some comparison codegen tests now that they pass in LLVM20) - rust-lang#137540 (Fix (more) test directives that were accidentally ignored) - rust-lang#137551 (import `simd_` intrinsics) - rust-lang#137599 (tests: use minicore more) - rust-lang#137673 (Fix Windows `Command` search path bug) - rust-lang#137676 (linker: Fix escaping style for response files on Windows) - rust-lang#137693 (Re-enable `--generate-link-to-defintion` for tools internal rustdoc) - rust-lang#137770 (Fix sized constraint for unsafe binder) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#137540 - yotamofek:pr/more-deprecated-test-directives, r=notriddle Fix (more) test directives that were accidentally ignored Continuation of rust-lang#137099 , caught by rust-lang#137103 (and needed to unblock that one). These test directives were accidentally using the old (`// ```@`)``` syntax
Continuation of #137099 , caught by #137103 (and needed to unblock that one).
These test directives were accidentally using the old (
// @
) syntax