FreezingArchRule: No longer violating a rule should under no circumstances break the test

[MSchmidke]

Current behaviour: When there are less-than-frozen violations, FreezingArchRule tries to save the shortened violation list (which undoubtedly basically is a good idea), but when feeze.store.allowStoreUpdate is set to false, this leads to a StoreUpdateFailedException.

Desired behaviour: When using FreezingArchRule, it should always be allowed to generate less-than-frozen violations, no matter which settings are used for freeze.store. So if allowStoreUpdate is false, it should simply do nothing. (Potentially there could be another option, failIfShorteningNotSaved or so for those who like to fail).

Reason:

• Our build manager strictly forbids the build to change any file in the workspace
• The acceptance of tools like SONAR or ArchUnit is not that big that we could make a rule for each developer to check/update ArchUnitTests before committing
• For that kind of developer, it is even less desired to point him too directly to the freezing mechanism so it isn't too simple to work around new violations. ArchUnit should work more in the background.

Extra Reason:

• If we had forbidden any update of Rules from the beginning on (not just since the new build manager detected the modified files in his workspace), we wouldn't have suffered that much from #458 since for years we didn't realize a test wasn't run on CI because it's violation store was silently created on each build.

[timtebeek]

From my experience the desired behaviour you described is what you get when you set feeze.store.allowStoreUpdate to true (the default). That actually allows ArchUnit to remove previous violations, while restricting you from adding new violations. I find it's best to set that to true locally, or by default, so any changes to violation stores are part of the diff when you commit.

Then, to satisfy your build manager requirement of not changing any files, you can override the setting on the build server to disallow freeze store updates. That way you can ensure there are never any pending freeze store changes not part of the commit & build.

Would such a flow solve most of your use case?

I also had to fumble a bit to arrive at this flow, perhaps is can be more clearly documented as a suggested workflow. In particular it's not immediately clear that allowStoreUpdate only allows removal (or creation of new rules store files?).

[codecholeric]

I think that makes sense :+1: But I would propose a new configuration skipStoreUpdate then.

Obviously the user guide should also be improved a little, because these options seem to cause more confusion than they should :joy: For example allowStoreCreation is something that really only needs to be set to true one time in the beginning. It is not needed to add new rules (but allowStoreUpdate is). The reason is that creating a new store by accident in a CI environment would make all tests green, even if they fail.

For FreezingArchRule it is never a "normal" use case to increase the number of violations. Thus, allowStoreUpdate=true will only allow to reduce the store. And allowStoreUpdate=false will demand that you have the correct violation state inside of the violation store. The idea is to override this parameter in a CI environment and set allowStoreUpdate=false so that the build will break if developers haven't also committed the store update while resolving old violations. The option came from a concrete issue in the past: #211

I understand that this is not possible in your environment so we should just add another configuration to disable that automatic reduction. In particular that would be backwards compatible and not break the existing behavior of allowStoreUpdate that people out there might be using.

[jan-car]

I'll try to look into this 🙂