TNG / ArchUnit

A Java architecture test library, to specify and assert architecture rules in plain Java
http://archunit.org
Apache License 2.0
3.2k stars 290 forks source link

Ignored Violations should not contribute to the ViolationStore #915

Closed klu2 closed 2 years ago

klu2 commented 2 years ago

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 commented 2 years ago

Honestly, I've never considered using archunit_ignore_patterns.txt and FreezingArchRule together :thinking: 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 :thinking: 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 :thinking: 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 :slightly_smiling_face:

klu2 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Ah, I see, thanks a lot for sharing :smiley: I think that this simply is a use case I never came by, to distribute FreezingArchRules at that scale as a library :man_shrugging: 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 :thinking:

codecholeric commented 2 years ago

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 :slightly_smiling_face:

klu2 commented 2 years ago

awesome stuff, thanks a lot!