add E-needs-test walkthrough

[jyn514]

per #2979 (comment):

"write an RFC" is not a good first contribution; it condenses multiple months of work into what looks like a few days; and most people are working on docs, tools, or the standard library, not new language features.

add a much smaller example that serves as a good blueprint for people just getting started.

r? rustc-dev-guide

[fmease]

CC #t-compiler > Label for "issue fixed but analysis still needed"? @ 💬

TL;DR: Q: How much work/thoroughness do we expect from authors+reviewers of PRs that try to address E-needs-test issues? Ideally ideally, they'd scour the test suite first (to avoid needless duplication), even perform a fail->pass bisection if needed, reconstruct the original root cause and add a meaningful description to the added tests. But realistically+practically speaking, the standards are much lower, and that's okay, maybe..

View changes since the review

[jyn514]

they'd scour the test suite first (to avoid needless duplication), even perform a fail->pass bisection if needed, reconstruct the original root cause

do we actually expect people to do this? we have something like 19000 tests ... i think in practice if that's the bar, then people will just close their PR. (of course, LLMs are really good at this, but people have repeatedly said that they don't want beginner issues to be done by an LLM.)

anyway, i don't think we should block this walkthrough on that decision; if we want to have higher standards for E-needs-test we should document it elsewhere.

[fmease]

Well, it's unclear what the majority of the r-l/r people thinks. Some people have vaguely answered in the linked Zulip thread but I don't think there's official consensus.

do we actually expect people to do this? we have something like 19000 tests ..

That's why a bisection to the original PR can be super helpful as you'd only need to look at the tests added/modified in the patch to see if it's potentially duplicating (well, you might need domain knowledge to determine that correctly...). And you're not really looking at all the tests, you're most likely narrowing down the set of files by grepping for diagnostic messages, feature names, CLI flags, keywords etc. [editors note: added more things after "diagnostic messages"].

However that might be too much to expect from a complete newcomer 🤷 Fortunately, we still have a reviewer that could double-check things in a rough manner but not too thoroughly, of course, otherwise there'd be a drastic imbalance in work.

[jyn514]

you're most likely narrowing down the set of files by grepping for diagnostic messages etc.

that doesn't work for check-pass tests, like the one here.

in general i think this is a very hard problem and not one we should be expecting new contributors to solve: rust-lang/rust#104939

[fmease]

I agree, if the goal is to provide something that beginners can do as a first contribution, that's acceptable. I guess my other take is too idealistic.

At the bare minimum, I expect all new tests to have a description (I guess that's not too controversial): At least once sentence that's not superficial (so no "this once used to ICE, see this issue"). Otherwise, it makes it harder and harder to understand a test's intention in the future (as you alluded to by linking to rust-lang/rust#104939).

The reviewer could / should help the author here in providing such a description.

With a description, it's a lot easier for someone with limited~okay domain knowledge to re-triage the issue in the future and decide if we should yeet it or not.

By not having "any" standards, this just compounds problems like rust-lang/rust#104939 as we'd allow quantity over quality. Hmm, I guess I'm hyperbolizing.

[jyn514]

note that 104939 was not caused by a test change, it was caused by an implementation change in rustdoc. note also that the linked test already had a description, it was just wrong after the implementation change.

With a description, it's a lot easier for someone with limited~okay domain knowledge to re-triage the issue in the future and decide if we should yeet it or not.

👍 i'm happy to add "add a description" to the walkthrough.