Require positive results by default

[simonthum]

This is a breaking change that requires rules to yield positive results by default. The intent is to protect against misformulated rules.

This is the first public iteration. It still loses type information on WithoutRequiringPositiveResults(), but otherwise I think it's fine.

Resolves #189

[simonthum]

The new code properly highlights the fact that the example tests do not test anything:

[xUnit.net 00:00:02.13]     ExampleTest.ExampleArchUnitTest.ExampleClassesShouldNotCallForbiddenMethods [FAIL]
  Failed ExampleTest.ExampleArchUnitTest.ExampleLayerShouldNotAccessForbiddenLayer [566 ms]
  Error Message:
   ArchUnitNET.xUnit.FailedArchRuleException : "Types that are Example Layer should not depend on Forbidden Layer because it's forbidden" failed:
	The rule requires positive evaluation, not just absence of violations. Use WithoutRequiringPositiveResults() or improve your rule's predicates.

Although something needs to be done about it, I view this as an encouraging sign.

I'll include the signoffs.

[fgather]

Hey @simonthum ,
thanks for the PR, it looks really promising. @MSinstein , @fholger , what do you think?

[fholger]

Hey @simonthum , thanks for the PR, it looks really promising. @MSinstein , @fholger , what do you think?

Looks good to me, though it is a breaking change and would probably benefit from some beta-testing by other real-world users.

[simonthum]

I agree with the concern. Is there an update site for CI results or qualified nuget builds available?

[fgather]

I will try to public some beta-release on nuget

[simonthum]

Hey, is there any progress to be aware of?

[WietseDeKoninckPXL]

This is an absolutely required addition (in this form or another) in my opinion.

As a new user I was confused that rules still passed even though absolutely nothing was modeled in my architecture yet, which means

A) There were technically no violations
B) But also no reason that warrants the existence of the rule

So if a user writes a rule that doesn't apply to the architecture nor presents any violations, the rule does not test anything.

[simonthum]

Thanks for merging!

However, it probably makes sense for a maintainer to stop by and look at the loss of type information I mentioned.

[sdepouw]

Great addition, but this behavior needs documented beyond the release notes. The failure message does tell you how to manually bypass it (by adding WithoutRequiringPositiveResults()) but does not mention what a "positive result." As a completely new user to ArchUnit, I thought I wasn't doing an assertion correctly or something. Not that the class collection under test was empty.

[simonthum]

Good point, I'll do something about it.

[simonthum]

@sdepouw How does that sound:

$"The rule requires positive evaluation, not a mere absence of violations. Use {nameof(WithoutRequiringPositiveResults)}() or write your rule's predicates such that they evaluate to true on actually existing code."

Or from scratch:

"There seems to be no code this rule could test. Please check the rule's predicates or use WithoutRequiringPositiveResults() if that is intentional."

[sdepouw]

@simonthum The in-code error could be the second one, although I'd probably clarify that "no code this rule could test" means that the predicate itself found no items.

And then somewhere in documentation for this specific error, it can be elaborated with stuff like this:

If your rule's predicate returns no results (e.g. IObjectProvider<Class> myPredicate = Classes().That().ImplementInterface("IDoNotExist").As("This Object Provider Will Be Empty")) then the rule is not evaluating against anything, hence the error.

You can verify your predicate's contents by writing int numberOfItems = _sorterClasses.GetObjects(Architecture).Count();

(Can be formatted nicer of course in documentation.)