I think it's time we remove Scrutinizer from vanilla/vanilla
I overheard some grumbling about Scrutinizer today and it got me thinking that we should remove it from vanilla/vanilla altogether. Here are my reasons:
- I don't think anyone but me have really looked at it.
- It slows down the build too much.
I think there is a use for static analysis in our workflow. In fact, we just had a catastrophic bug due to a simple == vs === error. That could absolutely been caught via lint rules. Perhaps someone can look into some sort of minimal enforcement we can do in Travis like what we do for javascript?
Comments
-
I am certainly guilty of not checking Scrutinizer. I don't think it ever really "fails" when it detects things we should change, so it doesn't draw a lot of attention to itself. It's similar to our Travis builds: if it doesn't fail, I don't check its logs. And since we started using it, Scrutinizer has amassed over 500 patch recommendations. At this point, it's basically noise.
Scrutinizer can take an unusually long time to run, which also makes people pay less attention to it, since it doesn't block PRs. You want to get a quick "gimme" PR in and after waiting fifteen minutes, Scrutinizer is still chugging along. Part of this seems to be due to our Scrutnizer account only having one worker setup. This means when you have multiple PRs coming in within the span of a few minutes, they're queued up. Build times stack.
When I checked on Scrutinizer, just now, there was "stuck" Scrutinizer job. It was causing all new PRs to be queued, so they're were backed up. The job had been waiting on "external code coverage" for approximately 16 hours (seems like it should've timed out well before then). I canceled the job. It looks like the queue has resumed processing. This was probably why Scrutinizer complaints have been more common in the past couple of days.
If we are going to ditch Scrutinizer, we can look into meeting our code analysis needs using something like Phan. Our PHP code volume is much, much larger than our JavaScript, so running a full scan is likely to take considerably longer. To mitigate that, we could potentially attempt a diff-based solution (Phan doesn't appear to support this out of the box), which may not be as capable, or perhaps only perform the scan during a daily (or at whatever interval) cron job on Travis. Phan supports a "quick" flag, which is not as comprehensive of a scan (seems to mainly be avoiding recursion), but I haven't attempted benchmarking the scans to see just how much "quicker" it actually is.
0 -
YES PLEASE!
1 -
I think if we are going to do static analysis we should enforce a small number of rules. Requiring
===in all new code is a good one to think about.Scrutinizer has certainly helped me find some bugs that the code didn't immediately hit so there are probably some checks that would be productive. Stuff like checking for code that doesn't get hit, type checking based on doc blocks where we don't have type hints yet.
An option could be to continue to use Scrutinizer, but remove the bulk of its tests and move it to a cron. If that's the case we need to get it down to no issues and have devs actually check it once a month or so.
0 -
I'm really surprised but I can't find a code sniffer rule for this anywhere. We'll need to write it ourselves.
0 -
Code sniffer has rules that expose this kind of bug. They check types and warn you when there is a mismatch if you use
==.0 -
I wonder whom you were talking about when you said:
I overheard some grumbling about Scrutinizer today...
0 -
Okay it's gone from the project. I've left Scrutinizer there so tests can be run manually.
1 -
0
-
Do you know what the rule is called? The code sniffer docs are pretty abysmal about specifying what rules do what and what they are called.
0 -
I’ve never really gone through the docs of phpcs. I’ll have a look at some
point, but for now let’s consider this not a massive priority.0
