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

Report line number of test when should_panic test failed #138603

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

xizheyin
Copy link
Contributor

@xizheyin xizheyin commented Mar 17, 2025

Closes #137405


try-job: x86_64-gnu-llvm-19-3
try-job: test-various

@rustbot
Copy link
Collaborator

rustbot commented Mar 17, 2025

Failed to set assignee to joshka: invalid assignee

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

@rustbot rustbot added A-compiletest Area: The compiletest test runner A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 17, 2025
@rustbot
Copy link
Collaborator

rustbot commented Mar 17, 2025

Some changes occurred in src/tools/compiletest

cc @jieyouxu

@xizheyin

This comment was marked as outdated.

@rustbot
Copy link
Collaborator

rustbot commented Mar 17, 2025

Could not assign reviewer from: jieyouxu.
User(s) jieyouxu are either the PR author, already assigned, or on vacation. Please use r? to specify someone else to assign.

@rust-log-analyzer

This comment has been minimized.

@jieyouxu
Copy link
Member

This looks like a libtest change, so r? libs (or testing-devex?)

@jieyouxu

This comment was marked as resolved.

@rustbot rustbot assigned joboet and unassigned Mark-Simulacrum Mar 17, 2025
@xizheyin xizheyin marked this pull request as draft March 17, 2025 14:03
@xizheyin xizheyin marked this pull request as ready for review March 17, 2025 14:23
@rust-log-analyzer

This comment has been minimized.

Copy link
Contributor

@joshka joshka left a comment

Choose a reason for hiding this comment

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

Test names are nits, but make the reason for each test self describing.

I think it probably makes sense to also fix the substring tests if they're also broken (I haven't manually checked those messages to see if there's span info coming from elsewhere in the stack, but these seem like they should be tested with the same tests as are introduced here).

@xizheyin xizheyin force-pushed the issue-137405 branch 2 times, most recently from 21b9d4f to c7b4523 Compare March 18, 2025 01:17
Copy link
Contributor

@joshka joshka left a comment

Choose a reason for hiding this comment

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

Generally LGTM - small nits

(Note: I'm not a member of the rust project, so this likely still requires approval from someone who is)

@rust-log-analyzer

This comment has been minimized.

@jieyouxu
Copy link
Member

(Note: I'm not a member of the rust project, so this likely still requires approval from someone who is)

Though reviews are still very much appreciated 😁

@weihanglo
Copy link
Member

@bors try

Before the change fails snapshots in Cargo's test testsuite

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 7, 2025
Report line number of test when should_panic test failed

Closes rust-lang#137405

---

try-job: x86_64-gnu-llvm-19-3
try-job: test-various
@bors
Copy link
Collaborator

bors commented Apr 7, 2025

⌛ Trying commit 92c0fc4 with merge e3d12c70105892942a1e11612422c4dd5870277f...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Collaborator

bors commented Apr 7, 2025

💔 Test failed - checks-actions

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 7, 2025
@xizheyin
Copy link
Contributor Author

xizheyin commented Apr 7, 2025

Oops, still failed.

maybe //@ exec-env:RUST_BACKTRACE=0, not //@ exec-env: RUST_BACKTRACE=0

@xizheyin
Copy link
Contributor Author

xizheyin commented Apr 7, 2025

//@ run-fail
//@ check-run-results
//@ compile-flags: --test
//@ exec-env:RUST_BACKTRACE=0
//@ normalize-stdout: "finished in \d+\.\d+s" -> "finished in $$TIME"
//@ run-flags: --test-threads=1

Now maybe it's okay. I learned a lot and next time I can look for the appropriate directives in similar tests.

@Zalathar
Copy link
Contributor

Zalathar commented Apr 7, 2025

@bors try

@bors
Copy link
Collaborator

bors commented Apr 7, 2025

⌛ Trying commit 5a13225 with merge 183e9d447d46102eff75d541478457051524346d...

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 7, 2025
Report line number of test when should_panic test failed

Closes rust-lang#137405

---

try-job: x86_64-gnu-llvm-19-3
try-job: test-various
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Collaborator

bors commented Apr 8, 2025

💔 Test failed - checks-actions

@xizheyin
Copy link
Contributor Author

xizheyin commented Apr 8, 2025

I looked at other tests in the same level directory and they usually had needs-threads and no-prefer-dynamic added and were ignored on my failed test execution. So I added this directive as well, not sure if it will help. And, I add --nocapture to make panic message into .run.stderr. @Zalathar

@chenyukang
Copy link
Member

@bors try

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 8, 2025
Report line number of test when should_panic test failed

Closes rust-lang#137405

---

try-job: x86_64-gnu-llvm-19-3
try-job: test-various
@bors
Copy link
Collaborator

bors commented Apr 8, 2025

⌛ Trying commit 4f5edfd with merge 1fa8aae...

Zalathar added a commit to Zalathar/rust that referenced this pull request Apr 8, 2025
compiletest: Trim whitespace from environment variable names

When a test contains a directive like `//@ exec-env: FOO=bar`, compiletest currently includes that leading space in the name of the environment variable, so it is defined as ` FOO` instead of `FOO`.

This is an annoying footgun that is pretty much never intended, especially since most other directives *do* trim whitespace. So let's get rid of it by trimming the environment variable name.

Values remain untrimmed, since there could conceivably be a use-case for values with leading space, but perhaps we'll end up trimming values too in the future.

Recently observed in rust-lang#138603 (comment).

Fixes rust-lang#132990.
Supersedes rust-lang#133148.

r? jieyouxu
@jieyouxu jieyouxu self-assigned this Apr 8, 2025
Signed-off-by: xizheyin <[email protected]>
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 8, 2025
compiletest: Trim whitespace from environment variable names

When a test contains a directive like `//@ exec-env: FOO=bar`, compiletest currently includes that leading space in the name of the environment variable, so it is defined as ` FOO` instead of `FOO`.

This is an annoying footgun that is pretty much never intended, especially since most other directives *do* trim whitespace. So let's get rid of it by trimming the environment variable name.

Values remain untrimmed, since there could conceivably be a use-case for values with leading space, but perhaps we'll end up trimming values too in the future.

Recently observed in rust-lang#138603 (comment).

Fixes rust-lang#132990.
Supersedes rust-lang#133148.

---

try-job: test-various
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-compiletest Area: The compiletest test runner A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tests marked as should_panic should report the line number of the test