Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.
Sign upConsider linting test code on CI #1143
Comments
killercup
added
the
discussion desired
label
Sep 4, 2017
willmurphyscode
referenced this issue
Sep 4, 2017
Merged
Add underscores to long literals in test code #1144
This comment has been minimized.
|
I'm actually wondering if we should just remove clippy from |
This comment has been minimized.
|
True. On the other hand, it's frustrating to see tests pass locally only to have CI fail on clippy. Do we mention `bin/check` enough in the Contributing Guide?
… Am 05.09.2017 um 18:38 schrieb Sean Griffin ***@***.***>:
I'm actually wondering if we should just remove clippy from bin/test as it increases build times quite a bit. I don't think linting the tests extensively adds nearly as much value
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
This comment has been minimized.
|
If we removed it from |
This comment has been minimized.
|
Yep. But if people *only* use bin/test, they wont ever see clippy's wonderful suggestions for their changes
… Am 05.09.2017 um 19:55 schrieb Sean Griffin ***@***.***>:
If we removed it from bin/test I would say that we should not lint tests on CI either. Only lint the main code, which is what CI does currently.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
This comment has been minimized.
|
Given that it's test code, I don't think that's a huge loss |
This comment has been minimized.
|
I'm also talking about regular code. With this change, the only way to get clippy to check anything is `bin/check` or being surprised by CI fails
… Am 05.09.2017 um 21:20 schrieb Sean Griffin ***@***.***>:
Given that it's test code, I don't think that's a huge loss
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
This comment has been minimized.
|
I guess it's a question of whether people are running |
This comment has been minimized.
|
Well, let's just hope people find |
killercup
closed this
Sep 7, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
killercup commentedSep 4, 2017
We are currently telling contributors to use the
bin/testscript to test their changes. This script runs a ALL thecargo tests—with clippy enabled. Which means it catches more than our CI setup does, as the Travis only runs the equivalent of cargo clippy.I think we can tell Travis to compile in a way so code in
#[cfg(test)]gets included, but this may conflict with compiling all backends at once.