Skip to content

Commit f0acb19

Browse files
committed
test-exercise: Detect Clippy failure by exit code, not by grep
The previous grep-based approach was too fragile: On beta, Clippy isn't even installed. Therefore, all attempts to run it simply show that Clippy wasn't installed, without actually running Clippy. This failure went completely undetected because the grep for `tests/` isn't going to match that. It's much less fragile to insert a `deny(clippy:all)` directive and let the exit code speak. As a side-effect, now Clippy *errors* for example solutions will cause the CI to fail, whereas Clippy *warnings* in them will still be accepted. We can see this as a value-add, assuming that if Clippy defaults to treating a particular lint as an error that it really should be fixed. However this does require that some examples be fixed in this PR. When merging this PR I plan to keep the commits separate, but will remove the proof commit and its reversion before merging.
1 parent 2d8ec8f commit f0acb19

File tree

2 files changed

+7
-11
lines changed

2 files changed

+7
-11
lines changed

.github/workflows/tests.yml

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -138,17 +138,11 @@ jobs:
138138
toolchain: ${{ matrix.rust }}
139139
default: true
140140

141-
- name: Clippy tests
141+
- name: Clippy
142142
env:
143-
CLIPPY: tests/
143+
CLIPPY: true
144144
run: ./_test/check-exercises.sh
145145

146-
# Our examples currently have too much Clippy output to consider this.
147-
# But maybe in the future.
148-
#- name: Clippy examples
149-
# env:
150-
# CLIPPY: src/lib.rs
151-
# run: ./_test/check-exercises.sh
152146
nightly-compilation:
153147
name: Check exercises on nightly (benchmark enabled)
154148
runs-on: ubuntu-latest

bin/test-exercise

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -83,10 +83,12 @@ done
8383
(
8484
cd $exercise
8585
if [ -n "$CLIPPY" ]; then
86+
# Consider any Clippy to be an error in tests only.
87+
# For now, not the example solutions since they have many Clippy warnings,
88+
# and are not shown to students anyway.
89+
sed -i -e '1i #![deny(clippy::all)]' tests/*.rs
8690
cargo clippy --all-targets --color always "$@" 2>clippy.log
87-
# Only consider it a failure if output contains the string in CLIPPY.
88-
# For example, if we're only looking in tests, CLIPPY contains tests/
89-
if grep -q $CLIPPY clippy.log; then
91+
if [ $? -ne 0 ]; then
9092
cat clippy.log
9193
exit 1
9294
else

0 commit comments

Comments
 (0)