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

Multiplatform tasks should not depend on check #4025

Merged
merged 1 commit into from
Aug 10, 2021

Conversation

cortinico
Copy link
Member

This PR is cleaning up our task dependencies a bit. Specifically the only task that should be a dependency of check is the plain detekt task.

When I worked on the KMP implementation, I accidentally plugged all the KMP tasks to depend on check. That's problematic as we still have some rough edges to fix and people might experience problems with their build because of this. See #3927 #3838

@codecov
Copy link

codecov bot commented Aug 8, 2021

Codecov Report

Merging #4025 (5d042ff) into main (48c1da8) will not change coverage.
The diff coverage is n/a.

❗ Current head 5d042ff differs from pull request most recent head 29ee3d6. Consider uploading reports for the commit 29ee3d6 to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##               main    #4025   +/-   ##
=========================================
  Coverage     83.55%   83.55%           
  Complexity     3181     3181           
=========================================
  Files           459      459           
  Lines          9085     9085           
  Branches       1771     1771           
=========================================
  Hits           7591     7591           
  Misses          561      561           
  Partials        933      933           

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 48c1da8...29ee3d6. Read the comment docs.

Comment on lines 76 to 80
it("detekt plain task will depend on check") {
gradleRunner.runTasksAndCheckResult(":check") { buildResult ->
assertThat(buildResult.tasks.map { it.path }).contains(":detekt")
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
it("detekt plain task will depend on check") {
gradleRunner.runTasksAndCheckResult(":check") { buildResult ->
assertThat(buildResult.tasks.map { it.path }).contains(":detekt")
}
}

I think this is already covered by https://github.com/detekt/detekt/pull/4025/files#diff-449c58285efdbf81c836b65ea85659877bfe46350c808afb25144183041de5f7R32

Copy link
Member Author

Choose a reason for hiding this comment

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

already covered by #4025 (files)

Which file are you referring to? I moved this test inside DetektPlain as we want to verify this behavior only for the plain task

Copy link
Member

Choose a reason for hiding this comment

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

😢 The link does not work properly. I intended to mention that this is already covered by line 32 in the same file. But now it seems that we could merge the two describe() spek together.

I don't think this is a blocking comment to be addressed though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup you're right, this test is unnecessary, as the one above is checking the same condition. I'm removing it.

@cortinico cortinico added this to the 1.18.0 milestone Aug 9, 2021
@schalkms
Copy link
Member

schalkms commented Aug 9, 2021

I assume that this PR fixes both linked issues, right?

@cortinico
Copy link
Member Author

I assume that this PR fixes both linked issues, right?

Nope it does not. It's related to both but the real fix needs to be discussed in the issues.

@BraisGabin BraisGabin merged commit bca4a15 into detekt:main Aug 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants