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

Ignored Violations should not contribute to the ViolationStore #915

Closed
klu2 opened this issue Jul 14, 2022 · 6 comments · Fixed by #944
Closed

Ignored Violations should not contribute to the ViolationStore #915

klu2 opened this issue Jul 14, 2022 · 6 comments · Fixed by #944

Comments

@klu2
Copy link

klu2 commented Jul 14, 2022

Right now, if you ignore a rule using the archunit_ignore_patterns.txt, and you wrap that into a FreezingArchRule, the ViolationStore is still being populated.

I would have expected that adding ignore patterns to the archunit_ignore_patterns.txt counts more here.

Interestingly, if the ViolationStore has once been created, and you add new violations to the ignored rule, then the tests pass.

In other words: it seems that the code that checks the ignore patterns runs after the freezing logic, but imho it should be the opposite.

If you believe that it should remain as it is, it would be great to add that information to https://www.archunit.org/userguide/html/000_Index.html#_ignoring_violations

In any case, I'd be happy to submit a PR

@codecholeric
Copy link
Collaborator

Honestly, I've never considered using archunit_ignore_patterns.txt and FreezingArchRule together 🤔 Because FreezingArchRule in the end is a more sophisticated version of the former, since FreezingArchRule automatically removes the ignores once they are fixed which archunit_ignore_patterns.txt can't do. And if it's a permanent thing that should be ignored I would likely write it directly into the rule instead, to make it more explicit...

That being said, I see your point, I would also consider the more natural behavior of this to be ignored from the ViolationStore 🤔 I see two ways of doing it:

a) encapsulate the filtering logic for archunit_ignore_patterns.txt, then hooking it into FreezingArchRule as well
b) move the logic into EvaluationResult or FailureReport

Spontaneously, I would think the second might be the better alternative, because it is then handled in one single place, and in the end it makes sense that when you check an ArchRule and look into the EvaluationResult of an ArchRule it should report the same failures 🤔 At the moment, I would guess that EvaluationResult.handleViolations(..) also behaves inconsistently with regards to archunit_ignore_patterns.txt, so maybe it would make sense to filter out ignored violations already there somehow.

If you want to create a PR for that I'd be happy to merge it 🙂

@klu2
Copy link
Author

klu2 commented Jul 16, 2022

Thanks for your quick reply, let me give you context on what we're planning to do, probably there exists a better solution.

We are running a lot of projects at @cloudflightio in parallel, most of them based on the Spring Boot stack. We have already created a couple of ArchUnit rules and want to provide those as a shared library that can be used in any of those projects.

Inside that library, we are having multiple rules for various frameworks (Spring, Spring Boot, JPA, Kotlin) and depending on what we find on the classpath, those rules are evaluated or not (that is already done).

All the the users have to do is do add a single import with Maven/Gradle and then do a

@ArchTest
val cleanCode = ArchTests.`in`(ArchUnitCleanCodeTests::class.java)

And ArchUnitCleanCodeTests then takes care of collecting those rules depending on the current classpath, users don't need to add dozens of individual ArchTests.in(...) statements. Additionally, all rules inside that library are wrapped with FreezingArchRule.

Now it can happen that in certain customer projects we have other rulesets and we i.e. don't want to evaluate rules around JPA entities. In that case we would use the archunit_ignore_patterns.txt to avoid that the ViolationStore is constantly being populated.

@klu2
Copy link
Author

klu2 commented Jul 17, 2022

Meanwhile I've open-sourced parts of our library with an own implementation of the ViolationStore.

The workaround here would then be that you once need to populate the store, then create the archunit_ignore_patterns.txt, remove all violation files, and then you're fine.

@codecholeric
Copy link
Collaborator

Ah, I see, thanks a lot for sharing 😃 I think that this simply is a use case I never came by, to distribute FreezingArchRules at that scale as a library 🤷‍♂️ So we can look into adjusting the behavior like I described above. I think for consistency it would make sense to exclude archunit_ignore_patterns.txt already from the EvaluationResult anyway. The only thing to keep in mind then is, that there are sometimes also "partial results" that might be joined or inverted, so we would need to make sure so not throw out events in the middle of the process, just because they match the patterns, before it's finally evaluated 🤔

@codecholeric
Copy link
Collaborator

I think I'll have some time to look into this this weekend. So ping me if you've already started working on it, otherwise I would try to tackle it 🙂

codecholeric added a commit that referenced this issue Aug 24, 2022
At the moment the handling of `archunit_ignore_patterns.txt` is inconsistent. E.g. `FreezingArchRule` still writes ignored violations to the `ViolationStore`, `EvaluationResult` reports `hasViolations` even if the violations are ignored, `EvaluationResult.getFailureReport()` still contains ignored violations and `EvaluationResult.handleViolations(..)` also handles ignored violations.
We now filter ignored patterns directly on construction of `EvaluationResult`. By that we will solve all these inconsistencies, because `EvaluationResult` as single source of truth already handles ignored violations correctly.

Resolves: #915
@klu2
Copy link
Author

klu2 commented Aug 24, 2022

awesome stuff, thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants