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

compiletest: ./x test <test-name>.stderr (or existing non-.rs file) should warn or error #126601

Open
jieyouxu opened this issue Jun 17, 2024 · 3 comments
Assignees
Labels
A-compiletest Area: The compiletest test runner C-bug Category: This is a bug. E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

Comments

@jieyouxu
Copy link
Member

jieyouxu commented Jun 17, 2024

Noticed by @WaffleLapkin, if you write

./x test tests/ui/parser/bad-let-else-statement.stderr

then compiletest will silently pass but run 0 tests, because bad-let-else-statement.stderr is a tracked stderr file associated with the actual .rs ui test but not a test itself. This might be surprising or give a false sense of security to the person who's running the test suite thinking that the test passed, whereas it actually didn't even run.

  • If the path doesn't exist, compiletest will report the non-existent test error.
  • However, if the path does exist, but it's not an actual test (whatever consitutes a "test" file under the relevant test suite), compiletest currently seems to silently ignore and pass.

We should probably warn or error if the path exists (and has an extension) but is not a valid test file.

@jieyouxu jieyouxu added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) C-bug Category: This is a bug. A-compiletest Area: The compiletest test runner labels Jun 17, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jun 17, 2024
@jieyouxu jieyouxu removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jun 17, 2024
@veera-sivarajan
Copy link
Contributor

@rustbot claim

@jieyouxu
Copy link
Member Author

Unfortunately as mentioned in #127175, because compiletest is a wrapper around libtest, but the actual test filtering logic is in libtest, compiletest does not have info from libtest as to how many tests actually run, so this isn't a simple one-liner fix.

@jieyouxu jieyouxu added E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. and removed E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. labels Oct 17, 2024
@jieyouxu
Copy link
Member Author

jieyouxu commented Oct 17, 2024

What we can do when we rework compiletest test discovery and directive handling is to not naively rely on libtest to do the filtering (libtest doesn't even to prefix matching, it just does something whacky like substring contains, which I found extremely unintuitive), otherwise we don't have that info in compiletest. In general, I want compiletest to have better control over how tests are discovered, prepared and packaged to libtest, especially surrounding when tests are ignored/filtered out.

@jieyouxu jieyouxu removed the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 17, 2024
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 C-bug Category: This is a bug. E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
Development

Successfully merging a pull request may close this issue.

3 participants