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

Fix (more) test directives that were accidentally ignored #137540

Merged
merged 2 commits into from
Mar 1, 2025

Conversation

yotamofek
Copy link
Contributor

Continuation of #137099 , caught by #137103 (and needed to unblock that one).
These test directives were accidentally using the old (// @) syntax

@rustbot
Copy link
Collaborator

rustbot commented Feb 24, 2025

r? @jieyouxu

rustbot has assigned @jieyouxu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-run-make Area: port run-make Makefiles to rmake.rs S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Feb 24, 2025
@rustbot
Copy link
Collaborator

rustbot commented Feb 24, 2025

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

@@ -1,6 +1,5 @@
// @has foobar/fn.ok.html '//*[@class="docblock scraped-example-list"]//*[@class="prev"]' ''
Copy link
Contributor Author

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?

Copy link
Member

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
Copy link
Contributor Author

@yotamofek yotamofek Feb 24, 2025

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...

Copy link
Contributor Author

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:

  1. 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, so check-pass should be enough for making sure this doesn't regress.
  2. Make this a rustdoc-mode test? Not sure if it supports auxiliary crates.
  3. Run htmldocck for ui-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?).

Copy link
Member

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

Copy link
Member

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.

Copy link
Contributor Author

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 😅

Copy link
Member

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.

Copy link
Member

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? 🤔

Copy link
Contributor Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jieyouxu
Copy link
Member

These seem like rustdoc tests (modulo the one ui test), so r? rustdoc

@rustbot rustbot assigned notriddle and unassigned jieyouxu Feb 24, 2025
@yotamofek yotamofek force-pushed the pr/more-deprecated-test-directives branch from 5c5b7d6 to 0881dba Compare February 27, 2025 13:23
@rustbot rustbot added the T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output. label Feb 27, 2025
@rustbot
Copy link
Collaborator

rustbot commented Feb 27, 2025

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

@notriddle
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Feb 28, 2025

📌 Commit 0881dba has been approved by notriddle

It is now in the queue for this repository.

@bors 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-review Status: Awaiting review from the assignee but also interested parties. labels Feb 28, 2025
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Feb 28, 2025
…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
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Feb 28, 2025
…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
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 1, 2025
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
@bors bors merged commit d09523d into rust-lang:master Mar 1, 2025
6 checks passed
@rustbot rustbot added this to the 1.87.0 milestone Mar 1, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 1, 2025
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
@yotamofek yotamofek deleted the pr/more-deprecated-test-directives branch March 3, 2025 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-run-make Area: port run-make Makefiles to rmake.rs S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants