Skip to content

Commit cd0cdfc

Browse files
simonthumsimonthum
committed
Change rule evaluation to also require positive results by default
The change is intended to make it easier to detect ineffective rules, as you can see in the example unit test. This change obviously breaks some tests, so it can be defeated per rule with a call to .WithoutRequiringPositiveResults(). Should().Exist() and NotExist() rules automatically defeat the check so their error messages should remain stable. However, the logic to check for (non)exitence had to be modified which may cause inconvenience. The implemetation basically treats an empty result set as a non-passing result (i.e. a violation). The change breaks a lot of unit tests, mostly those which iterate over a bunch of test types and are tautologies. This issue is adressed in a subsequent commit. Signed-off-by: Simon Thum <sthum@meddv.de>
1 parent 0fcbb6c commit cd0cdfc

8 files changed

Lines changed: 116 additions & 4 deletions

File tree

‎ArchUnitNET/Fluent/ArchRule.cs‎

Lines changed: 35 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
// SPDX-License-Identifier: Apache-2.0
66

77
using System.Collections.Generic;
8+
using System.Linq;
89
using ArchUnitNET.Domain;
910
using ArchUnitNET.Fluent.Syntax;
1011

@@ -16,14 +17,46 @@ protected ArchRule(IArchRuleCreator<TRuleType> ruleCreator) : base(ruleCreator)
1617
{
1718
}
1819

20+
/// <summary>
21+
/// By default, rules are evaluated so positive results are required to be present.
22+
/// This call defeats this check on the rule.
23+
/// </summary>
24+
public ArchRule<TRuleType> WithoutRequiringPositiveResults()
25+
{
26+
_ruleCreator.RequirePositiveResults = false;
27+
return this;
28+
}
29+
30+
1931
public bool HasNoViolations(Architecture architecture)
2032
{
21-
return _ruleCreator.HasNoViolations(architecture);
33+
if (_ruleCreator.RequirePositiveResults)
34+
{
35+
return Evaluate(architecture).All(e => e.Passed);
36+
}
37+
else
38+
{
39+
return _ruleCreator.HasNoViolations(architecture);
40+
}
41+
2242
}
2343

2444
public IEnumerable<EvaluationResult> Evaluate(Architecture architecture)
2545
{
26-
return _ruleCreator.Evaluate(architecture);
46+
var result = _ruleCreator.Evaluate(architecture).ToList();
47+
48+
// To require positives, we only ever need to add
49+
// a non-passing result if there are no results.
50+
if (_ruleCreator.RequirePositiveResults &&
51+
result.Count == 0)
52+
{
53+
result.Add(new EvaluationResult(
54+
this, new StringIdentifier(Description), false,
55+
$"The rule requires positive evaluation, not just absence of violations. Use {nameof(WithoutRequiringPositiveResults)}() or improve your rule's predicates.",
56+
this, architecture));
57+
}
58+
59+
return result;
2760
}
2861

2962
public CombinedArchRuleDefinition And()

‎ArchUnitNET/Fluent/ArchRuleCreator.cs‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
//
55
// SPDX-License-Identifier: Apache-2.0
66

7+
using System;
78
using System.Collections.Generic;
89
using System.Linq;
910
using ArchUnitNET.Domain;
@@ -16,6 +17,7 @@ public class ArchRuleCreator<TRuleType> : IArchRuleCreator<TRuleType> where TRul
1617
{
1718
private readonly ConditionManager<TRuleType> _conditionManager;
1819
private readonly PredicateManager<TRuleType> _predicateManager;
20+
private bool? _requirePositiveResults;
1921

2022
public ArchRuleCreator(BasicObjectProvider<TRuleType> basicObjectProvider)
2123
{
@@ -93,6 +95,20 @@ public void SetCustomConditionDescription(string description)
9395
_conditionManager.SetCustomDescription(description);
9496
}
9597

98+
private void SetRequirePositiveResults(bool requirePositive)
99+
{
100+
if (_requirePositiveResults != null &&
101+
_requirePositiveResults != requirePositive)
102+
throw new InvalidOperationException("conflicting positive expectation");
103+
_requirePositiveResults = requirePositive;
104+
}
105+
106+
public bool RequirePositiveResults
107+
{
108+
get => _requirePositiveResults ?? true;
109+
set => SetRequirePositiveResults(value);
110+
}
111+
96112
private bool HasNoViolations(IEnumerable<TRuleType> filteredObjects, Architecture architecture)
97113
{
98114
return EvaluateConditions(filteredObjects, architecture).All(result => result.Passed);

‎ArchUnitNET/Fluent/CombinedArchRuleCreator.cs‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
//
55
// SPDX-License-Identifier: Apache-2.0
66

7+
using System;
78
using System.Collections.Generic;
89
using System.Linq;
910
using ArchUnitNET.Domain;
@@ -18,6 +19,7 @@ public class CombinedArchRuleCreator<TRuleType> : IArchRuleCreator<TRuleType> wh
1819
private readonly ArchRuleCreator<TRuleType> _currentArchRuleCreator;
1920
private readonly LogicalConjunction _logicalConjunction;
2021
private readonly ICanBeEvaluated _oldRule;
22+
private bool? _requirePositiveResults;
2123

2224
public CombinedArchRuleCreator(ICanBeEvaluated oldRule, LogicalConjunction logicalConjunction,
2325
BasicObjectProvider<TRuleType> basicObjectProvider)
@@ -99,6 +101,21 @@ public void SetCustomConditionDescription(string description)
99101
_currentArchRuleCreator.SetCustomConditionDescription(description);
100102
}
101103

104+
private void SetRequirePositiveResults(bool requirePositive)
105+
{
106+
if (_requirePositiveResults != null &&
107+
_requirePositiveResults != requirePositive)
108+
throw new InvalidOperationException("conflicting positive expectation");
109+
_requirePositiveResults = requirePositive;
110+
}
111+
112+
public bool RequirePositiveResults
113+
{
114+
get => _requirePositiveResults ?? true;
115+
set => SetRequirePositiveResults(value);
116+
}
117+
118+
102119
public override string ToString()
103120
{
104121
return Description;

‎ArchUnitNET/Fluent/ConditionManager.cs‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -90,9 +90,10 @@ public IEnumerable<EvaluationResult> EvaluateConditions(IEnumerable<T> filteredO
9090
Architecture architecture, ICanBeEvaluated archRuleCreator)
9191
{
9292
var filteredObjectsList = filteredObjects.ToList();
93-
if (filteredObjectsList.IsNullOrEmpty())
93+
if (filteredObjectsList.IsNullOrEmpty() &&
94+
!CheckEmpty())
9495
{
95-
yield return new EvaluationResult(null, new StringIdentifier(""), CheckEmpty(),
96+
yield return new EvaluationResult(null, new StringIdentifier(""), false,
9697
"There are no objects matching the criteria", archRuleCreator, architecture);
9798
yield break;
9899
}

‎ArchUnitNET/Fluent/IArchRuleCreator.cs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,5 +31,7 @@ void ContinueComplexCondition<TRelatedType>(IPredicate<TRelatedType> predicate)
3131

3232
void SetCustomPredicateDescription(string description);
3333
void SetCustomConditionDescription(string description);
34+
35+
bool RequirePositiveResults { get; set; }
3436
}
3537
}

‎ArchUnitNET/Fluent/Syntax/Elements/ObjectsShould.cs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ protected ObjectsShould(IArchRuleCreator<TRuleType> ruleCreator) : base(ruleCrea
2727

2828
public TRuleTypeShouldConjunction Exist()
2929
{
30+
_ruleCreator.RequirePositiveResults = false;
3031
_ruleCreator.AddCondition(ObjectConditionsDefinition<TRuleType>.Exist());
3132
return Create<TRuleTypeShouldConjunction, TRuleType>(_ruleCreator);
3233
}
@@ -545,6 +546,7 @@ public ShouldRelateToAttributesThat<TRuleTypeShouldConjunction, TRuleType> OnlyH
545546

546547
public TRuleTypeShouldConjunction NotExist()
547548
{
549+
_ruleCreator.RequirePositiveResults = false;
548550
_ruleCreator.AddCondition(ObjectConditionsDefinition<TRuleType>.NotExist());
549551
return Create<TRuleTypeShouldConjunction, TRuleType>(_ruleCreator);
550552
}

‎ArchUnitNETTests/Fluent/RuleEvaluationTests.cs‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,10 @@ public class RuleEvaluationTests
5151
private static readonly IArchRule WrongArchRule1AndWrongArchRule3 = WrongArchRule1.And(WrongArchRule3);
5252
private static readonly IArchRule WrongArchRule4AndWrongArchRule8 = WrongArchRule4.And(WrongArchRule8);
5353

54+
private static readonly IArchRule WrongArchRule9 =
55+
Classes().That().HaveName(NoClassName).Should().BePrivate();
56+
57+
5458
private readonly string _expectedWrongArchRule1AndWrongArchRule3ErrorMessage =
5559
"\"Classes that are \"ArchUnitNETTests.Domain.PublicTestClass\" should be private\" failed:" +
5660
NewLine + "\tArchUnitNETTests.Domain.PublicTestClass is public" + NewLine +
@@ -108,6 +112,10 @@ public class RuleEvaluationTests
108112
NewLine + "\tArchUnitNETTests.Domain.PublicTestClass does exist and is public" +
109113
NewLine + NewLine;
110114

115+
private readonly string _expectedWrongArchRule9ErrorMessage = "\"Classes that have name \"NotTheNameOfAnyClass_1592479214\" should be private\" failed:" + NewLine +
116+
"\tThe rule requires positive evaluation, not just absence of violations. Use WithoutRequiringPositiveResults() or improve your rule's predicates." +
117+
NewLine + NewLine;
118+
111119
[Fact]
112120
public void AssertArchRuleTest()
113121
{
@@ -133,6 +141,8 @@ public void AssertArchRuleTest()
133141
ArchRuleAssert.CheckRule(Architecture, WrongArchRule1AndWrongArchRule3));
134142
var exception4And8 = Assert.Throws<FailedArchRuleException>(() =>
135143
ArchRuleAssert.CheckRule(Architecture, WrongArchRule4AndWrongArchRule8));
144+
var exception9 =
145+
Assert.Throws<FailedArchRuleException>(() => ArchRuleAssert.CheckRule(Architecture, WrongArchRule9));
136146

137147
Assert.Equal(_expectedWrongArchRule1ErrorMessage, exception1.Message);
138148
Assert.Equal(_expectedWrongArchRule2ErrorMessage, exception2.Message);
@@ -144,6 +154,8 @@ public void AssertArchRuleTest()
144154
Assert.Equal(_expectedWrongArchRule8ErrorMessage, exception8.Message);
145155
Assert.Equal(_expectedWrongArchRule1AndWrongArchRule3ErrorMessage, exception1And3.Message);
146156
Assert.Equal(_expectedWrongArchRule4AndWrongArchRule8ErrorMessage, exception4And8.Message);
157+
Assert.Equal(_expectedWrongArchRule9ErrorMessage, exception9.Message);
158+
147159
}
148160

149161
[Fact]

‎ExampleTest/ExampleArchUnitTest.cs‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,9 @@ public class ExampleArchUnitTest
4040
private readonly IObjectProvider<Interface> ForbiddenInterfaces =
4141
Interfaces().That().HaveFullNameContaining("forbidden").As("Forbidden Interfaces");
4242

43+
private readonly IObjectProvider<IType> TypoLayer =
44+
Types().That().ResideInNamespace("TypoNamespace").As("Non-existing Typo Layer");
45+
4346

4447
//write some tests
4548
[Fact]
@@ -70,6 +73,32 @@ public void ExampleLayerShouldNotAccessForbiddenLayer()
7073
exampleLayerShouldNotAccessForbiddenLayer.Check(Architecture);
7174
}
7275

76+
[Fact]
77+
public void TypoLayerShouldViolateByDefault()
78+
{
79+
// test the new default case
80+
IArchRule typoLayerShouldFailByDefault = Types().That().Are(TypoLayer).Should()
81+
.NotDependOnAny(ForbiddenLayer).Because("typo layer does not exist");
82+
83+
Assert.False(typoLayerShouldFailByDefault.HasNoViolations(Architecture));
84+
85+
86+
// test the active assertion (with implicit exemption)
87+
IArchRule typoLayerShouldDefinitelyNotExist = Types().That().Are(TypoLayer).Should()
88+
.NotExist().Because("typo layer simply is not there");
89+
90+
Assert.True(typoLayerShouldDefinitelyNotExist.HasNoViolations(Architecture));
91+
92+
93+
// test the explicit exemption
94+
IArchRule typoLayerCanCauseNoViolations = Types().That().Are(TypoLayer).Should()
95+
.NotDependOnAny(ForbiddenLayer)
96+
.Because("typo layer does not exist and causes no violation")
97+
.WithoutRequiringPositiveResults();
98+
99+
Assert.True(typoLayerCanCauseNoViolations.HasNoViolations(Architecture));
100+
}
101+
73102
[Fact]
74103
public void ForbiddenClassesShouldHaveCorrectName()
75104
{

0 commit comments

Comments
 (0)