Review how global suppressions are made in default config.

[TWiStErRob]

History

Some time ago the only way to suppress was a global filters = '.*/test/.*' or --filter.

Then came

test-pattern:
  active: true
  patterns:
    - '.*/test/.*'
    - '.*Test.kt'
  exclude-rule-sets:
    - 'some-rule-set'
  exclude-rules:
    - 'SomeRule'

Both of these were deprecated in favor of the new:

SomeRule:
  excludes: ['**/test/**', '**/androidTest/**', '**/commonTest/**', '**/jvmTest/**', '**/jsTest/**', '**/iosTest/**']

which we could configure per rule giving us way more flexibility.

Now the latest addition is suppressors.

SomeRule:
  ignoreAnnotated:
    - 'javax.inject.Inject'
  ignoreFunction:
    - org.example.foo(kotlin.String)

Proposal

With every new (more specific) way of suppression the exclusions were migrated to the new solution. I think there might be time for another look at the ** excludes and use the new ignore* solutions where possible. This would help improve the quality of test code bases around the world.

Background

#4921 (comment) triggered me to write this recommendation.

Some arguments

Tests are sometimes even more important to be readable than production code as they define what the production code should be doing. You can have golfed Kotlin code in production, if you have tests, you know what it is doing.

If test break, we should be able to see why it broke in order to know if it's the test or the production code broken, because these require different actions.

Based on this: helper functions, utility classes, fixture factories, etc. shouldn't have long names, or allow for wildcard imports, magic numbers, magic strings, etc.

Tests protect us making mistakes in the production code, but if they're internally broken, they don't help much; to bring an analogy: "one wouldn't want a shield with holes in it".

[cortinico]

With every new (more specific) way of suppression the exclusions were migrated to the new solution. I think there might be time for another look at the ** excludes and use the new ignore* solutions where possible. This would help improve the quality of test code bases around the world.

In theory, I understand the value of your suggestion (i.e. using ignoreAnnotated to skip @Test blocks and so).

Practically, this suffers of two problems:

1. We will make Detekt less platform agnostic. Now we exclude everything which is inside the **/test/** folder, without making any assumption on the test framework. E.g. if you were to use Spek, you won't be using the @Test annotation at all.
2. The new ignoreAnnotated has runtime logic which (partially) depends on Type Resolution. While the exclude relies only on filtering the task input. The latter is simpler and less error prone.

[BraisGabin]

1. We will make Detekt less platform agnostic. Now we exclude everything which is inside the /test/ folder, without making any assumption on the test framework.

100% agree but...

E.g. if you were to use Spek, you won't be using the @test annotation at all.

With Spek you don't have long function names because it works with Strings. So, us excluding**/test/** are adding some "framework specifics" right now... This is a difficult topic. I don't know how to answer this properly...

I can even think on a "solomonic decission": Remove the excludes: from that rule.

[cortinico]

So, us excluding**/test/** are adding some "framework specifics" right now...

To some sense, yes. But this is also easy to adapt to any other custom setup. Plus test is such a generic folder name which I believe it's safe to assume there will be tests inside

[TWiStErRob]

(Found a note in a random file I didn't send 😅, backdate this to 2022 June)

Hmm, interesting one on the folder names. I would like to add to that: test assumes a Maven/Gradle-like conventional project structure (/src/test/java/). My tests could be in the /tests/ folder or even javatests and I could be using Bazel to build (it requires no specific folder structure, prod and tests are in one folder).

The Test annotation is not that far fetched as default support considering the above (just as assuming). Note that Kotlin's first class testing support has that annotation which aliases others. Yes, Spek is special, just like Bazel too.

I think there's two forces in play here: supporting regular users and treating special cases. So as long as the defaults cover most of the Kotlin ecosystem AND it's easily configurable, I think we're good to have any defaults.

[BraisGabin]

I would like to add to this discussion that the current default exclusion method has some issues:

• Baseline incomplete if project located in "test" directory. #3728
• If we have a rule excluded from tests and you don't want that you need to add this to your configuration:

  TheRule
    excludes: []

• If You want to add one more path to the exclude list you need to copy&paste all the list and append yours

[TWiStErRob]

• It's hard to find which rules are excluded in a particular config as the config is built from multiple sources (→ Dump current configuration #4925)