TNG / ArchUnit

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

False positive results when I use ArchRuleDefinition.noClasses() #1310

Closed ramon2 closed 1 month ago

ramon2 commented 1 month ago

Hi, in my company we are using archunit to tests architecture of our microservices. But recently I've detected some issues with the archunit library and I would like to know if it is due to our infrastructure, we are coding microservices in Kotlin and Spring Boot 3 or it is a bug of the archunit library. The case is very simple, I've added here the example that it is giving me a false positive, the test is succeed when it should fail. But if I replace ArchRuleDefinition.noClasses() for ArchRuleDefinition.classes() then it works as I'd expected and it fails.

I'm using version 1.3.0 of archunit-junit5 library.

@AnalyzeClasses(
  packages = ["com.adevinta.testing"],
  importOptions = [ImportOption.DoNotIncludeTests::class]
)
class TestingArchitectureTest {

  @ArchTest
  fun arch_test_example_3(importedClasses: JavaClasses) {
    ArchRuleDefinition.noClasses()
      .that()
      .resideInAnyPackage("..application..")
      .should(alwaysFail)
      .check(importedClasses)
  }

  var alwaysFail: ArchCondition<JavaClass> =
    object : ArchCondition<JavaClass>("This condition should always fail") {
      override fun check(item: JavaClass, events: ConditionEvents) {
        events.add(violated(item, "This is an error for testing purpose ${item.name}"))
      }
    }
}
hankem commented 1 month ago

There seems to be a confusion about the semantics of the condition in these rules (which could be better documented 🙈):

This is why it's actually expected that your test with the "alwaysFail" condition indeed passes perfectly.

Whenever I can't wrap my head around this kind of logic, I imagine a simple example:

You can change from noClasses() to classes() by inverting the condition (e.g. using ArchConditions.not or ArchConditions.never).

ramon2 commented 1 month ago

Thanks @hankem for the response, but I tried all possible scenarios using ArchRuleDefinition.noClasses() with a custom ArchCondition and I couldn't make the tests fail. For example, I tested cases where the condition:

and the result is always successful. Could you provide an example that fails with noClasses() and a custom ArchCondition? I suppose I’m missing something, and maybe an example could help me.

GuiRodriguero commented 1 month ago

Hi @ramon2! I faced the same issue today and for me, changing from events.add(violated...) to events.add(satisfied...) was enough If you're telling ArchUnit "no classes should be like this", then the condition must be satisfied, not violated

Let me know if it works for you :)

hankem commented 1 month ago

@GuiRodriguero is exactly right: Whether the evaluation of an ArchCondition needs to add aConditionEvent that isViolated or not depends only on the semantics of the condition – for example, haveSimpleName("A") adds a SimpleConditionEvent with satisfied = true for every object that has simple name "A" and satisfied = false for every other object. It then depends on the rule which objects pass the test and which fail. (That's why alwaysFail is a misleading name for your demo condition.)

ramon2 commented 1 month ago

Thanks @GuiRodriguero and @hankem , with satisfied condition works perfectly

hankem commented 1 month ago

I'm glad that it works for you! Can we then close this issue?

ramon2 commented 1 month ago

yes, thanks