GitHub Actions: Run Clippy on tests#1006
GitHub Actions: Run Clippy on tests#1006petertseng merged 7 commits intoexercism:masterfrom petertseng:clippytests
Conversation
just want to make sure the exit-code propagation still works
This reverts commit dbed275.
My assessment is: That is possible, just like how previously-succeeding CI might fail when a new Rust toolchain release gets promoted to beta. We are not doing anything special to deal with that situation either, just fixing them as they come, so I suppose we'd do the same here and not do anything special. |
coriolinus
left a comment
coriolinus
left a comment
There was a problem hiding this comment.
Is it possible that an upstream Clippy update will cause previously-succeeding CI to fail on the same commit because of new Clippy lints that have been added?
Yes, and in fact is fairly common. For example, the lint warning about redundant 'static in const A_STR: &'static str was added shortly after it became legal to omit the explicit lifetime from that sort of declaration. The best thing I can think of to mitigate this is to tailor precisely what jobs are run:
- when GHA runs on a PR, it should only run clippy on files modified by that PR
- when GHA runs on
masterbranch, it should run clippy on all files
In both cases, we probably want to run it both on stable and beta rustc so that we get some forewarning when a new lint is going to become a problem. We may wish to configure it that it's not a failing error if clippy reports something on beta, but if the build doesn't actually break, I'm not sure we'd actually notice if beta started reporting new issues.
| PR_NUMBER: ${{ github.event.pull_request.number }} | ||
| run: echo "TRAVIS_PULL_REQUEST=${PR_NUMBER:-false}" >> $GITHUB_ENV | ||
|
|
||
| - name: Install Clippy |
There was a problem hiding this comment.
Right now this is under the compilation key, which runs it on all steps in the matrix. I don't think that's necessary.
Probably the best solution is adding it to its own build job, kind of like the benchmarking on nightly. We might in the future decide that we want to run it on its own matrix, so we can get warnings when new lints are introduced on the beta channel before they hit stable.
There was a problem hiding this comment.
OK. I just went ahead and added the matrix. stable and beta.
There was a problem hiding this comment.
... which makes me realise and I never actually removed this Install clippy item. and the fact that Clippy still succeeded despite that means that it's being installed by default anyway.
All right, that seems like a good idea. The "violate clippy by adding return" and subsequent revert give me assurance that the first bullet point has been achieved, and I believe that since it's going through _test/check-exercises.sh that the second bullet point will also be achieved, but 100% certainty on that will only be gained after merging. |
|
If we want either or both of these two new checks "Clippy (stable)" or "Clippy (beta)" to be required, we will need to remember to ask an admin to make them required after this is merged. ... though I see that no checks are currently marked as required, so maybe we don't need to do that and the current situation works just fine for us |
DENYWARNINGS or CLIPPY Co-authored-by: Peter Goodspeed-Niklaus <coriolinus@users.noreply.github.com>
This reverts commit 615aa01.
Nope, didn't happen, because of TRAVIS_PULL_REQUEST. That has convinced me it's time to expunge that. PR coming very soon. |
When moving off of Travis in #975, there were some instances of TRAVIS_PULL_REQUEST left over as a transitionary strategy. Now we have been hurt by leaving TRAVIS_PULL_REQUEST in: In #1006 a new GitHub Actions job was added that forgot to set TRAVIS_PULL_REQUEST and caused incorrect behaviour. So let's remove TRAVIS_PULL_REQUEST and instead change it to GITHUB_EVENT_NAME. Documentation: https://docs.github.com/en/free-pro-team@latest/actions/reference/environment-variables
| # Only consider it a failure if output contains the string in CLIPPY. | ||
| # For example, if we're only looking in tests, CLIPPY contains tests/ | ||
| if grep -q $CLIPPY clippy.log; then |
There was a problem hiding this comment.
Notice! I have discovered that this has proven error-prone. I will be submitting a PR to correct that in a few hours.
2 participants
Supports #659
Note that if we merge this PR, it implies we accept the burden of keeping ourselves in line with Clippy so long as we don't revert it. So we should make sure we want to accept that burden before merging.
Should discuss: Is it possible that an upstream Clippy update will cause previously-succeeding CI to fail on the same commit because of new Clippy lints that have been added? If so, what, if anything, do we want to do about that?
A PR that breaks compilation is included because the behavior of bin/test-exercise changed and it's important to check that it still propagates its exit code.
This will also fail until being rebased on #1003, but that is just as well - it verifies that the Clippy check works correctly.