Skip to content

consider linting test/run-pass/*.rs to ensure all have assert/assert_eq #23113

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

Closed
pnkfelix opened this issue Mar 6, 2015 · 6 comments
Closed
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc C-enhancement Category: An issue proposing an enhancement or a PR with one.

Comments

@pnkfelix
Copy link
Member

pnkfelix commented Mar 6, 2015

spawned off of the story in #23112

We should consider making the compiletest system reject run-pass tests that contain no calls to assert! nor assert_eq!

(Better still would be to also reject if no such call is reached during an execution of the test, but that would probably require we add testing-specific assertion macros.)

We could add a comment flag to allow tests to opt out of this requirement (e.g. if they are regression tests that are indeed just checking that we do not segfault and do not have a clear assertion to include).

@pnkfelix
Copy link
Member Author

pnkfelix commented Mar 6, 2015

(of course, one cannot stop people from writing weak tests via such requirements, since they may just put in silly assertions. but then again, I think a silly assertion will stick out more during the review process than the absence of any assertion will.)

@pnkfelix pnkfelix added the A-testsuite Area: The testsuite used to check the correctness of rustc label Mar 6, 2015
@steveklabnik
Copy link
Member

Triage: no changes.

@tommyip
Copy link
Contributor

tommyip commented May 16, 2017

I want to take a stab at doing this. Should I use regex to test the presence of assertions or is there a better way?

@Mark-Simulacrum
Copy link
Member

I would either add this to compiletest (src/tools/compiletest) or make it a tidy check (src/tools/tidy). If in compile test, I would probably look for the code that loads in tests. I'm somewhat preferring using tidy for this, but would be fine with either. I'm also not entirely sure we even want this -- tests without assert_eq/assert seem fine to me, if for example they're testing for a lack of segfault or similar. We also don't have a compile-pass test suite so some run-pass tests are maybe just compile-pass tests (again, without assert_eq/assert). So I'm not sure that this is entirely the best issue to work on; if you're interested in this area I would look at one of these: #40713 or #33581. I can probably provide some details on thoughts on either issue, just leave a comment if interested.

If you want to implement this, feel free though. I think that we'll run into a lot of false positives, but I could be wrong. Let me know if there's anything we can help you with!

@Mark-Simulacrum Mark-Simulacrum added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jul 22, 2017
@steveklabnik
Copy link
Member

Triage: I don't think anyone took mark up on his offer, as far as I know.

@Mark-Simulacrum
Copy link
Member

I'm going to close this as I'm still not sure that this is entirely a good idea, in particular since we have ~1597 such files today.

Generated the list (https://gist.github.com/Mark-Simulacrum/cb65bb73dcc43914f2aae416370ec83b) with:

rg --multiline --multiline-dotall 'run-pass.*assert' src/test/ui -l | sort > /tmp/with-assert
rg --multiline --multiline-dotall 'run-pass' src/test/ui -l | sort > /tmp/all
diff -U0 /tmp/all /tmp/with-assert | rg -v '@@'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc C-enhancement Category: An issue proposing an enhancement or a PR with one.
Projects
None yet
Development

No branches or pull requests

4 participants