Improve reporting of ignored violations

[codecholeric]

Typically in legacy code bases there will be a ton of violations, when architecture rules are introduced for the first time. ArchUnit supports the 'archunit_ignore_patterns.txt' file, to exclude messages based on regular expressions, but once violations are ignored, there is no further feedback. The following is a list of suggestions, to improve the overview:

• Log a message for each ignored violation, so the process is more transparent and it's easier to trace why a violation isn't reported. I would propose INFO level, because it's neither a warning per se (after all, it was configured that way and behaves as expected), nor is it some mainly unimportant information from a business point of view that you only want on DEBUG. Any objections?
• Improve priorities -> make it configurable, if a priority should already cause a rule to fail, or just log the message (e.g. archunit.properties>failTestsOfPriority=MEDIUM)
• Add a full html report for the current state, including passed, violated and ignored rules. This would be the biggest part, and I wonder if it should be its own module, instead of being part of the core functionality. (my guts tell me yes, since it doesn't feel like ArchUnit's core domain, but an extension)

[bfassbender]

What I did with a similar challenge regarding PMD rules violations in a large legacy project was to introduce thresholds per priority. I believe it was a custom Ant Task back in the days, but it worked.

So we could capture the status quo without ignoring rules. This enabled us to have regular conversations in the team on where to clean up next. After cleaning up, the violations decreased every time. So we celebrated and lowered the thresholds to match the new (lower) values. Step by step we cleaned up until our target was achieved (0 critical and major of our PMD rules being violated).

Of course we had rare cases where a new violation had been introduced while an old one was fixed. But this part of the tradeoff and did not hinder us from reaching our overall goal.

How do you think about this?

I agree with your last point from above, that a generator for a full report would better fit a separate module instead of the core. The concise scope of ArchUnit's core is a thing I really love.

[codecholeric]

Yes, that sounds like a good idea!! Thresholds like this would definitely be useful. I wish I could split myself into several devs, then I'd already be working on this :slightly_frowning_face:

[bfassbender]

Well, we could stage this. First: we look into a simple report generator for all ignored violations as a separate module.

This gives transparency and we see how far this will help.

If we can agree on you giving me quick 1:1 intro on the inner structure of ArchUnit and point me in the right direction, I could give it a try and then come back with a pull request.

[codecholeric]

Sure, for mail check https://github.com/codecholeric or @codecholeric on Twitter. I you want, we can schedule some video conference to check out the code. I don't know, if the report is the 'quickest win' though, or if it would be cooler to pimp archunit.properties to allow priority.HIGH.threshold = 50 or something like that :wink: In the end that might be enough to say "every week we lower the threshold by 10" and start to fix this over time...

[bfassbender]

That would be the sweet solution. I’m optimistic, we‘ll figure something out in the 1:1 I‘ll get back to you.

[codecholeric]

To conclude our video conference:

• it it unfeasible to build this into ArchUnit's evaluation core, because simple JUnit tests are evaluated too locally (e.g. threshold 100, you evaluate a single rule, it won't fail, you evaluate a class it fails, ...) -> ArchUnit only knows about single rules failing, it does not make sense to integrate a threshold for priorities there, because the interesting thing is a complete run
• it should be a build tool or CI server solution (e.g. Maven/Gradle plugin, Jenkins plugin, ...)

Thanks for your time and the brain storming @bfassbender :smiley:

[kapexx]

Would it be possible to report patterns that didn't match anything?

I worked on a large legacy code base where we had a big archunit_ignore_patterns file. It always was a bit hard to maintain the file because of the size and since many developers didn't bother to update it after refactoring, there were a lot unnecessary patterns in there. To remove obsolete ignore patterns, we sometimes deleted all patterns, executed the tests and then converted the log output of violations into patterns with search/replace in a tetx editor and then dumped those patterns into the archunit_ignore_patterns file. If unused patterns were reported too, we could have identified those more easily and remove them by hand more frequently.

I also toyed with the idea to run tests with a special flag which would generate a configurable pattern for each rule violation (e.g. a combination of rule and class name) and then either report all patterns so that they can be copied to the ignore file manually or even automatically re-generate the archunit_ignore_patterns file. That would have been helpful for our workflow but at some point we reduced the amount of ignore patterns to a manageable level so I didn't implement anything like that.

[hankem]

You'll probably like #181. ;)

[codecholeric]

I do agree that there is some room for optimization of the ignore pattern handling. My thoughts when I originally implemented it was, that maybe you have one central ignore file and want to reuse if within different sub projects, no matter if all the patterns are matched. Meanwhile I also don't know if people use it that way. But I guess it wouldn't be too hard to log out the patterns that didn't match anything. The question would be at what level the logging should be done. If you assume it's a misuse, then you could log a warning. But then on the other hand you could also raise an exception. The other possibility would be info or debug logging, just for cases where you want to continuously clean up that file manually.

As @hankem pointed out, instead of investing more energy to create ignore patterns automatically, I would recommend to use the new FreezingArchRule with the next release, which will not only provide a convenient way to ignore the current state of a rule, but moreover automatically reduces the threshold if violations are fixed in time. It was pretty much designed for exactly that use case (it just needs some more testing if it works sufficiently well).