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

Always update the baseline if it exists #4445

Merged
merged 7 commits into from
Jan 5, 2022
Merged

Always update the baseline if it exists #4445

merged 7 commits into from
Jan 5, 2022

Conversation

BraisGabin
Copy link
Member

If you have a module with some issues in the baseline and you fix all of them, when you update the baseline detekt does nothing. This PR fixes that. If there is a baseline file, we always update it.

This PR also fixes a really annoying flaky test related with baseline.

Fix #3736

@BraisGabin BraisGabin added this to the 1.20.0 milestone Jan 3, 2022
@BraisGabin BraisGabin changed the title Fix 3736 Always update the baseline if it exist Jan 3, 2022
@codecov
Copy link

codecov bot commented Jan 3, 2022

Codecov Report

Merging #4445 (2a47b8a) into main (21a751a) will increase coverage by 0.00%.
The diff coverage is 81.81%.

Impacted file tree graph

@@            Coverage Diff            @@
##               main    #4445   +/-   ##
=========================================
  Coverage     84.34%   84.34%           
- Complexity     3299     3300    +1     
=========================================
  Files           473      473           
  Lines         10532    10534    +2     
  Branches       1885     1885           
=========================================
+ Hits           8883     8885    +2     
  Misses          671      671           
  Partials        978      978           
Impacted Files Coverage Δ
...osch/detekt/core/baseline/BaselineResultMapping.kt 78.57% <ø> (-2.68%) ⬇️
.../arturbosch/detekt/core/baseline/BaselineFacade.kt 85.71% <81.81%> (+5.71%) ⬆️

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 21a751a...2a47b8a. Read the comment docs.

@schalkms
Copy link
Member

schalkms commented Jan 3, 2022

@BraisGabin shall we also explain the new behavior in the FAQ section on the homepage?
https://detekt.github.io/detekt/baseline.html

@BraisGabin
Copy link
Member Author

I don't think so. There wasn't any explanation about when the baseline was created or not. And what this PR does have more sense than the previous behavior.

@chao2zhang chao2zhang added the notable changes Marker for notable changes in the changelog label Jan 4, 2022
Comment on lines 28 to 31
if (baseline.isNotEmpty() || exists) {
baselineFile.parent?.let { Files.createDirectories(it) }
BaselineFormat().write(baseline, baselineFile)
}
Copy link
Member

Choose a reason for hiding this comment

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

If the content stays the same, always writing to the file will cause the Detekt Gradle task to always not up-to-date.

Do you think it's worth it to not update the file when the content does not change?

Copy link
Member Author

Choose a reason for hiding this comment

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

The content is always the same so it doesn't affect gradle and the up-to-date. But I'm going to do the check so we avoid an unnecessary write to disk.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, I think that the code now is even better.

Copy link
Member

Choose a reason for hiding this comment

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

Ah nice. I had thought the file last access timestamp is considered in the input cache key calculation.

@BraisGabin
Copy link
Member Author

Is this a notable change? I feel it as a bug-fix.

@chao2zhang
Copy link
Member

I think it is - There could be additional tooling that reads those baseline.xml and generates a customized report (My previous team does that)

@BraisGabin BraisGabin merged commit 4f6793f into main Jan 5, 2022
@BraisGabin BraisGabin deleted the fix-3736 branch January 5, 2022 17:26
@cortinico cortinico changed the title Always update the baseline if it exist Always update the baseline if it exists Feb 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notable changes Marker for notable changes in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update baseline when there is no more issues
3 participants