Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite Suppressions.kt tests #7108

Merged
merged 4 commits into from Apr 6, 2024
Merged

Rewrite Suppressions.kt tests #7108

merged 4 commits into from Apr 6, 2024

Conversation

BraisGabin
Copy link
Member

The reason of this rewrite is #7101. The previous tests relay on how the Rule works to test what isSuppressedBy those. The new implementation depends on nothing else than what they are testing. This helps to unblocks #7101 a bit.

Also these test should be more exhaustive and easier to read and maintain.

Copy link

codecov bot commented Mar 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.85%. Comparing base (dc77d52) to head (108aa9f).
Report is 1 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #7108      +/-   ##
============================================
- Coverage     83.86%   83.85%   -0.01%     
  Complexity     3949     3949              
============================================
  Files           578      578              
  Lines         12155    12155              
  Branches       2493     2493              
============================================
- Hits          10194    10193       -1     
- Misses          732      733       +1     
  Partials       1229     1229              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

}
private fun KtFile.getMethod(): KtElement {
return findChildByClass(KtClass::class.java)!!
.findFunctionByName("function")!!
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor but should "function" be a parameter of this KtFile.getMethod(functionName: String = "function")? It reads a bit awkward to me

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a bit awkward because this functions only work if the KtFile is more less as the ones that I defined in the different tests. If you check them they are very similar, the only change is where is the annotation. Adding a parameter to this function has little sense because, anyway, this function only work with that structure of KtFile. I changed this a bit to remove the hardcode "function" but the idea is still the same.

I'm going to merge this now because it unblocks me for other PRs but I you think that it's not good enough tell me and I can look at it and find a better solution on other PR.

}
private fun KtFile.getMethodParameter(): KtElement {
return findChildByClass(KtClass::class.java)!!
.findFunctionByName("function")!!
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

BraisGabin and others added 2 commits April 6, 2024 23:18
@detekt-ci
Copy link
Collaborator

detekt-ci commented Apr 6, 2024

Warnings
⚠️ This PR is approved with no milestone set. If merged, it won't appear in the detekt release notes.

Generated by 🚫 dangerJS against 108aa9f

@BraisGabin BraisGabin enabled auto-merge (squash) April 6, 2024 21:35
@BraisGabin BraisGabin merged commit 04bc262 into main Apr 6, 2024
20 of 21 checks passed
@BraisGabin BraisGabin deleted the supressions-tests branch April 6, 2024 21:37
@cortinico cortinico added this to the 2.0.0 milestone Apr 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api core housekeeping Marker for housekeeping tasks and refactorings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants