Use constants for config keys in tests

[amitd291]

No description provided.

[codecov-io]

Codecov Report

Merging #2070 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #2070   +/-   ##
=========================================
  Coverage     80.59%   80.59%           
  Complexity     1988     1988           
=========================================
  Files           332      332           
  Lines          5705     5705           
  Branches       1042     1042           
=========================================
  Hits           4598     4598           
  Misses          554      554           
  Partials        553      553

Impacted Files	Coverage Δ	Complexity Δ
...rbosch/detekt/rules/complexity/NestedBlockDepth.kt	93.47% <0%> (ø)	5% <0%> (ø)	⬇️
...n/kotlin/io/gitlab/arturbosch/detekt/api/Entity.kt	92.3% <0%> (ø)	5% <0%> (ø)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f5a8086...aa0a0b1. Read the comment docs.

[3flex]

I understand and appreciate this PR, but I'm against it because changing a constant in a rule can then break user config files since that won't then be detected in the tests. Using literals protects against that (at least for the way most rule tests have been written).

We could use the constants though if there were other tests checking the constant value was as expected, but that also seems a bit wrong, since we should ultimately be testing that the correct config keys affect the behaviour or the rule.

But at the moment I think this PR introduces some risk.

[amitd291]

I understand and appreciate this PR, but I'm against it because changing a constant in a rule can then break user config files since that won't then be detected in the tests. Using literals protects against that (at least for the way most rule tests have been written).

We could use the constants though if there were other tests checking the constant value was as expected, but that also seems a bit wrong, since we should ultimately be testing that the correct config keys affect the behaviour or the rule.

But at the moment I think this PR introduces some risk.

Agreed that we generally test using literals ideally. I was aiming towards standardisation, as half of the tests are using constants declared in the rules to test them.

An agreeable approach could probably be introducing the constants within the test file for private access. What do you folks think? @3flex @schalkms @arturbosch

[3flex]

I think that's a good idea, I'd approve that PR (would need another maintainer to sign off though so maybe wait for another reply). Thanks!

[amitd291]

I think that's a good idea, I'd approve that PR (would need another maintainer to sign off though so maybe wait for another reply). Thanks!

Okay let's wait for other maintainers' replies and then I can go ahead and do the required changes here. Thanks @3flex

[schalkms]

I highly appreciate this PR.
It looks good and really improves the code base. However, I'm against introducing a constant in the product just for the unit test. So this 1 constant should be removed. Then we can merge this.