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

Check for static imports in unused imports rule #3188

Merged
merged 3 commits into from
Nov 1, 2020

Conversation

sowmyav24
Copy link
Contributor

Fixes #3131

@codecov
Copy link

codecov bot commented Oct 31, 2020

Codecov Report

Merging #3188 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3188      +/-   ##
============================================
+ Coverage     79.55%   79.57%   +0.01%     
  Complexity     2631     2631              
============================================
  Files           440      440              
  Lines          7964     7969       +5     
  Branches       1521     1522       +1     
============================================
+ Hits           6336     6341       +5     
  Misses          818      818              
  Partials        810      810              
Impacted Files Coverage Δ Complexity Δ
...lab/arturbosch/detekt/rules/style/UnusedImports.kt 85.29% <100.00%> (+1.16%) 4.00 <0.00> (ø)

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 da6259a...efabb70. Read the comment docs.

@sowmyav24
Copy link
Contributor Author

When I run the task ./gradlew :detekt-rules-style:test in local it passes. In CI it seems to fails for test-snippet compilation.

@BraisGabin
Copy link
Member

Use ./gradlew :detekt-rules-style:test -Pcompile-test-snippets=true

Compile all the snippets each time that we run the tests slow down the test and is kind of meta testing (testing the tests). For that reason we have the compilation disabled by default and we just compile them in CI.

import org.eclipse.persistence.annotations.FetchType is not in the classpath so it fails. compileAndLintWithContext allows multiple Strings that are like different files. This way you can create your own enum in other package and then use it in your test.

Comment on lines 557 to 567
val mainFile = """
import x.y.z.Foo

val x = Foo.LAZY
"""
val additionalFile = """
package x.y.z

enum class FetchType {
LAZY
}
Copy link
Member

Choose a reason for hiding this comment

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

Why does this compile? the enum is called FetchType but the import is for Foo 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wrote the tests with Foo and later renamed them before committing to mimic the issue reported. However, your point of compilation seems to be valid 😮

Copy link
Member

Choose a reason for hiding this comment

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

@schalkms do you have any idea about why does this happen? This doesn't stop the PR but I will like to know why does this happen

Copy link
Member

Choose a reason for hiding this comment

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

@BraisGabin Yes, I do. Please take a look at the following lines and closely check line number 68.

fun BaseRule.compileAndLintWithContext(
environment: KotlinCoreEnvironment,
@Language("kotlin") content: String,
@Language("kotlin") vararg additionalContents: String,
): List<Finding> {
if (shouldCompileTestSnippets && additionalContents.isEmpty()) {
KotlinScriptEngine.compile(content)
}
return lintWithContext(environment, content, *additionalContents)
}

@arturbosch arturbosch added this to the 1.15.0 milestone Oct 31, 2020
@schalkms schalkms merged commit e7d1ddd into detekt:master Nov 1, 2020
arturbosch pushed a commit that referenced this pull request Nov 15, 2020
* Check for static imports in unused imports rule

* Fix test compilation

* Fix test variable names
arturbosch pushed a commit that referenced this pull request Dec 21, 2020
* Check for static imports in unused imports rule

* Fix test compilation

* Fix test variable names
arturbosch pushed a commit that referenced this pull request Jan 16, 2021
* Check for static imports in unused imports rule

* Fix test compilation

* Fix test variable names
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UnusedImports false positive for enums in annotation attributes (with type resolution)
5 participants