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

Xml Report Merger now merges duplicate smells across input report files #5033

Merged
merged 3 commits into from Jul 7, 2022

Conversation

timothyolt
Copy link
Contributor

@timothyolt timothyolt commented Jul 3, 2022

Xml Report Merger can now merge duplicate smells across multiple input report files. This is useful especially in Android, where running Detekt on multiple flavors identities many of the same errors on different flavors. Also cuts down on the amount of noise when these reports are submitted to Sonar.

I tried to make this as readable as possible, even though this process is complicated. Let me know if there is anything to improve on.

@github-actions github-actions bot added the gradle label Jul 3, 2022
}

/** A list of checkstyle xml files written by Detekt */
private class DetektCheckstyleReports(private val files: Collection<File>) {
Copy link
Contributor Author

@timothyolt timothyolt Jul 3, 2022

Choose a reason for hiding this comment

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

Using classes here to help call out preconditons and assumptions about the input data. These functions started out as extension functions, but I didn't like the idea of applying something so specific to broad types such as Colleciton<File> and List<Node>Thoughs?

Copy link
Sponsor Member

@chao2zhang chao2zhang Jul 7, 2022

Choose a reason for hiding this comment

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

I don't have strong opinions, so I would just conform to the pattern in this file (Wrapping inside a class seems to be the pattern)

@codecov
Copy link

codecov bot commented Jul 3, 2022

Codecov Report

Merging #5033 (53d0a92) into main (6edf6bb) will increase coverage by 84.86%.
The diff coverage is n/a.

Current head 53d0a92 differs from pull request most recent head 047fe40. Consider uploading reports for the commit 047fe40 to get more accurate results

@@             Coverage Diff             @@
##             main    #5033       +/-   ##
===========================================
+ Coverage        0   84.86%   +84.86%     
- Complexity      0     3555     +3555     
===========================================
  Files           0      500      +500     
  Lines           0    11753    +11753     
  Branches        0     2189     +2189     
===========================================
+ Hits            0     9974     +9974     
- Misses          0      692      +692     
- Partials        0     1087     +1087     
Impacted Files Coverage Δ
...ekt/rules/style/RedundantVisibilityModifierRule.kt 98.24% <0.00%> (ø)
...urbosch/detekt/rules/performance/ArrayPrimitive.kt 74.19% <0.00%> (ø)
...ab/arturbosch/detekt/rules/bugs/HasPlatformType.kt 72.00% <0.00%> (ø)
...in/io/gitlab/arturbosch/detekt/api/SingleAssign.kt 83.33% <0.00%> (ø)
...lab/arturbosch/detekt/rules/style/MaxLineLength.kt 95.00% <0.00%> (ø)
...gitlab/arturbosch/detekt/rules/KtCallExpression.kt 33.33% <0.00%> (ø)
...detekt/rules/style/UnderscoresInNumericLiterals.kt 89.36% <0.00%> (ø)
...io/github/detekt/report/sarif/SarifOutputReport.kt 91.17% <0.00%> (ø)
...ekt/rules/style/SpacingBetweenPackageAndImports.kt 75.00% <0.00%> (ø)
...bosch/detekt/rules/complexity/LongParameterList.kt 85.71% <0.00%> (ø)
... and 490 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 6edf6bb...047fe40. Read the comment docs.

@timothyolt
Copy link
Contributor Author

timothyolt commented Jul 3, 2022

Build failure is :detekt-report-md:test in MdOutputReportSpec.kt asserts that the generated HTML is the same even if we change the order of the findings

Comparing the differences between the expected/actual reports, the difference I found was on the last line of the MD report:

generated with [detekt version 1.21.0-RC2](https://detekt.dev/) on 2022-07-03 15:04:34 UTC

vs

generated with [detekt version 1.21.0-RC2](https://detekt.dev/) on 2022-07-03 15:04:35 UTC

The difference is time? Is this a known flake?

@timothyolt
Copy link
Contributor Author

timothyolt commented Jul 3, 2022

Build failure is :detekt-report-md:test in MdOutputReportSpec.kt asserts that the generated HTML is the same even if we change the order of the findings

Comparing the differences between the expected/actual reports, the difference I found was on the last line of the MD report:

generated with [detekt version 1.21.0-RC2](https://detekt.dev/) on 2022-07-03 15:04:34 UTC

vs

generated with [detekt version 1.21.0-RC2](https://detekt.dev/) on 2022-07-03 15:04:35 UTC

The difference is time? Is this a known flake?

Pushed a commit mocking the time for more consistent MarkdownReportSpec results

@cortinico
Copy link
Member

cortinico commented Jul 5, 2022

The difference is time? Is this a known flake?

Thanks for spotting this. Nope it was not known, but as the Markdown report was introduced recently, this was most likely the root cause.

@BraisGabin BraisGabin requested a review from chao2zhang Jul 6, 2022
* <file>
* <error>
* </file>
Copy link
Sponsor Member

@chao2zhang chao2zhang Jul 7, 2022

Choose a reason for hiding this comment

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

:+100: on this documentation

}

/** A list of checkstyle xml files written by Detekt */
private class DetektCheckstyleReports(private val files: Collection<File>) {
Copy link
Sponsor Member

@chao2zhang chao2zhang Jul 7, 2022

Choose a reason for hiding this comment

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

I don't have strong opinions, so I would just conform to the pattern in this file (Wrapping inside a class seems to be the pattern)

@BraisGabin BraisGabin added this to the 1.21.0 milestone Jul 7, 2022
@BraisGabin BraisGabin merged commit b6448e3 into detekt:main Jul 7, 2022
18 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants