Add Checkstyle rule for broken switch cases #88739
Conversation
We use `between(x, y)` calls with `switch` statements in tests to randomize test behaviour. However, some usages define `case` statements that can never execute, because the `case` value is outside the range defined by the `between` call. Write a rule that inspects the switches and the cases, and fails on the broken cases.
pugnascotia
added
>enhancement
>test
|
@breskeby how hard would it be to add some benchmarks for this kind of thing or does the fact that our existing benchmark suite does |
|
@mark-vieira this implementation does at least limit itself to switch statements. It's not like the long-lines rules rule. But, I don't have any perf number for you, so 🤷♂️ |
Yeah, I don't suspect it'll be a problem, but we have infrastructure for generating performance metrics on the effect of any given build change so I think it's worth leveraging that for stuff like this (or any additional static analysis checks) to give a number to the cost of such a thing. |
|
We also use |
|
@mark-vieira we do indirectly check the performance with the precommit checks we have. But given all the stuff we run there, I can imagine it would need to be really really significant for us to catch here. We could setup checkstyle specific performance checks to detect performance losses here early. I'm just not sure its worth it |
|
@DaveCTurner I also handled |
I think it might be worth adding a scenario for checkstyle specifically at some point. We are increasingly adding more complex checkstyle rules and we don't really have any idea of how costly they are. Checkstyle as a whole we know is pretty expensive but it's not clear if that is just the AST generation or the rules, or what. |
|
@elasticmachine update branch |
|
Pinging @elastic/es-delivery (Team:Delivery) |
|
@joegallo @mark-vieira the dependent issues are now closed, can I get a review please? |
There was a problem hiding this comment.
+100 on the net effect here, and the code looks reasonable to me. ❤️ for taking a look at this, @pugnascotia.
We use `between(x, y)` calls with `switch` statements in tests to randomize test behaviour. However, some usages define `case` statements that can never execute, because the `case` value is outside the range defined by the `between` call. Write a rule that inspects the switches and the cases, and fails on the broken cases. This rule checks `between`, `randomIntBetween` and `randomInt`.
7 participants
We use
between(x, y)calls withswitchstatements in tests torandomize test behaviour. However, some usages define
casestatementsthat can never execute, because the
casevalue is outside the rangedefined by the
betweencall.Write a rule that inspects the switches and the cases, and fails on the
broken cases.
The following issues cover the changes required to fix this problem.
ConvertProcessorTeststests have switches with impossible branches #88744KeywordFieldMapperTestshas switch with impossible branches #88746