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

Run CLI sanity checks with Gradle #4186

Merged
merged 2 commits into from
Oct 20, 2021
Merged

Run CLI sanity checks with Gradle #4186

merged 2 commits into from
Oct 20, 2021

Conversation

3flex
Copy link
Member

@3flex 3flex commented Oct 16, 2021

I find that I occasionally prep a PR, run gradlew build successfully then submit. Then there are failures because there are some CI jobs that aren't captured when running the build task.

This PR migrates these jobs to Gradle tasks that are run on check so a full successful build run will also pass on CI.

@3flex 3flex added the housekeeping Marker for housekeeping tasks and refactorings label Oct 16, 2021
@codecov
Copy link

codecov bot commented Oct 16, 2021

Codecov Report

Merging #4186 (5e59b8d) into main (e0fa7a3) will increase coverage by 0.07%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #4186      +/-   ##
============================================
+ Coverage     84.12%   84.19%   +0.07%     
- Complexity     3201     3233      +32     
============================================
  Files           467      468       +1     
  Lines         10102    10185      +83     
  Branches       1773     1786      +13     
============================================
+ Hits           8498     8575      +77     
+ Misses          673      671       -2     
- Partials        931      939       +8     
Impacted Files Coverage Δ
...osch/detekt/rules/exceptions/SwallowedException.kt 75.40% <0.00%> (-0.46%) ⬇️
...ain/kotlin/io/gitlab/arturbosch/detekt/api/Rule.kt 62.96% <0.00%> (ø)
.../kotlin/io/gitlab/arturbosch/detekt/api/RuleSet.kt 75.00% <0.00%> (ø)
...kotlin/io/gitlab/arturbosch/detekt/api/Location.kt 81.48% <0.00%> (ø)
...ain/kotlin/io/github/detekt/metrics/LinesOfCode.kt 95.45% <0.00%> (ø)
...otlin/io/gitlab/arturbosch/detekt/api/MultiRule.kt 100.00% <0.00%> (ø)
...tlin/io/gitlab/arturbosch/detekt/cli/JCommander.kt 100.00% <0.00%> (ø)
...n/kotlin/io/github/detekt/report/html/HtmlUtils.kt 98.33% <0.00%> (ø)
.../arturbosch/detekt/rules/style/ForbiddenComment.kt 100.00% <0.00%> (ø)
...rturbosch/detekt/rules/style/NewLineAtEndOfFile.kt 100.00% <0.00%> (ø)
... and 6 more

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 e0fa7a3...5e59b8d. Read the comment docs.

Copy link
Member

@BraisGabin BraisGabin left a comment

Choose a reason for hiding this comment

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

I know that this task exactly the same as running the Java -jar but wouldn't it be better to keep the ci as it was before? I mean, adding this to the check task seems a good idea, but we don't need to change ci.

@3flex
Copy link
Member Author

3flex commented Oct 16, 2021

We could, but to me it's neater to have everything configured & run from Gradle directly, and using the same tasks in CI. If we add the tasks to the Gradle build but leave CI alone then it's one method to run this in Gradle, and another in CI... why not keep it consistent?

Copy link
Member

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

Definitely an improvement. I think we would just lose a bit on the E2E testing here, but overall is worth to merge. What I mean is that now we're effectively testing that a detekt-cli-all.jar is built and that can be invoked. Using only gradle we might end up in situation where say the .jar output path changed and we don't test this scenario. Not a problem overall.

detekt-cli/build.gradle.kts Outdated Show resolved Hide resolved
@BraisGabin
Copy link
Member

My idea was just to keep it as similar as how our users use it. I know, it should be the same but just to be sure. I'm not against this change. It was just my 2 cents. If you think that it is better to keep the ci more consistent I'm fine with it too.

@chao2zhang
Copy link
Member

In my opinion, keep the ci more consistent has the benefits of easily applying gradle.properties to all gradle commands. This is particularly useful for other repos that we might need to increase max heap size or switch to G1GC to resolve CI flakiness.

@3flex 3flex merged commit b427095 into detekt:main Oct 20, 2021
@cortinico cortinico added this to the 1.19.0 milestone Oct 31, 2021
@3flex 3flex deleted the gradle-sanity branch November 19, 2021 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
housekeeping Marker for housekeeping tasks and refactorings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants