Skip to content

Align advice to discourage issue-number-only names #2323

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

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

Conversation

smanilov
Copy link
Contributor

https://rustc-dev-guide.rust-lang.org/tests/best-practices.html#test-naming specifically asks devs to "Avoid using only issue numbers as test names".

This commit introduces a minor change to https://rustc-dev-guide.rust-lang.org/tests/compiletest.html#crashes-tests that changes the advice to use only issue numbers as names to the advice to append them at the end of the file name.

https://rustc-dev-guide.rust-lang.org/tests/best-practices.html#test-naming specifically asks devs to "Avoid using only issue numbers as test names".

This commit introduces a minor change to https://rustc-dev-guide.rust-lang.org/tests/compiletest.html#crashes-tests that changes the advice to use only issue numbers as names to the advice to append them at the end of the file name.
Slight rewording without changing the meaning.
@BoxyUwU
Copy link
Member

BoxyUwU commented Apr 10, 2025

Do we have consensus on this from somewhere? I kind of also prefer people to not write tests named what-my-test-is-183842.rs

@BoxyUwU
Copy link
Member

BoxyUwU commented Apr 10, 2025

oh wait this is crashes tests

@BoxyUwU
Copy link
Member

BoxyUwU commented Apr 10, 2025

I think the advice for crashes tests tends to actually be to just name the test after the issue no. (so the current docs seem in line with what is done in practice), if you look at the tests/crashes directory every single test is named something like 289743.rs (except for two...)

@smanilov
Copy link
Contributor Author

smanilov commented Apr 10, 2025

I think the advice for crashes tests tends to actually be to just name the test after the issue no. (so the current docs seem in line with what is done in practice), if you look at the tests/crashes directory every single test is named something like 289743.rs (except for two...)

Oh, I see, thanks for pointing this out.

So more or less I would close the issue... if it weren't for another "conflicting" advice: https://rustc-dev-guide.rust-lang.org/tests/ui.html#test-organization says "Avoid including the issue number in the test name.", but then points to the best practices, which says "Avoid using issue numbers alone as test names." and "Avoid starting the test name with issue-xxxxx prefix as it degrades auto-completion.".

Perhaps a note for "crashes" tests that it explicitly goes agains the general best practices is still warranted?

Is the convention for ui tests also specific (and ok to conflict with best practices), in your opinion?

@jieyouxu
Copy link
Member

jieyouxu commented Apr 13, 2025

Hi, the page there is explicitly for ui tests, which notably does not include crashes tests. Probably just remark that for crashes tests it is canonical that tests are named only after issue numbers because its purpose is to track snippets from which issues no longer ICE/crash, and they would either be removed or converted into proper ui/other tests in the fix PRs.

@jieyouxu jieyouxu added S-waiting-on-author Status: this PR is waiting for additional action by the OP A-test-suite Area: rust-lang/rust test suites T-compiler Relevant to compiler team labels Apr 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-test-suite Area: rust-lang/rust test suites S-waiting-on-author Status: this PR is waiting for additional action by the OP T-compiler Relevant to compiler team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants