Add more specific effect matchers #178
Conversation
Codecov Report
@@ Coverage Diff @@
## master #178 +/- ##
==========================================
+ Coverage 94.37% 94.74% +0.37%
==========================================
Files 43 43
Lines 1279 1370 +91
==========================================
+ Hits 1207 1298 +91
Misses 72 72
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Is it just me or is it a bit weird that the Nimble matchers are named have while the MobiusTest matchers are named has? Not a big deal but seems like it would make sense to use the same prefix.
For a future change I wonder if we should rename (as-in add a new method named that, migrate existing code to it and then remove the old method) (has|have)Effects to containsEffect? In order to make it more clear which matcher does what.
I initially thought that as well, but it helps tests read more naturally. I assume that's the original reasoning for it.
Yeah, I struggled with how to best name them. The problem is that |
Ah yeah that makes sense 👍
I think Hamcrest is the odd one out here. As both Nimble and the Swift standard library uses |
|
I think both 'has' and 'contains' mean the same thing - that is, no difference regarding whether it means 'has at least' or 'has exactly' or 'contains at least' and 'contains exactly'. So for precision, a longer name is warranted, in my opinion. |
| var unmatchedActual = next.effects | ||
| var unmatchedExpected = expected | ||
| zip(next.effects, expected).forEach { | ||
| _ = unmatchedActual.firstIndex(of: $1).map { unmatchedActual.remove(at: $0) } | ||
| _ = unmatchedExpected.firstIndex(of: $0).map { unmatchedExpected.remove(at: $0) } | ||
| } |
There was a problem hiding this comment.
@kmcbride I'm trying to finalize #177 (finally!) and this code is causing me troubles. When for example next.effects == [.a, .b] and expected == [.b], then after running this code unmatchedExpected == [.b], which I think is incorrect, as .b could actually be matched.
Could this be implemented this way (which would in my opinion fix the problem):
let unmatchedActual = next.effects.filter { !expected.contains($0) }
let unmatchedExpected = expected.filter { !next.effects.contains($0) }or is there any edge case I'm not seeing here?
There was a problem hiding this comment.
The intention here was to do the minimum work necessary to determine if the expectation could be satisfied, so the arrays don't always contain the "true" diff between the two inputs even though the returned value is correct. Feel free to adjust as needed for computing the actual diff 😬
Introduces a few additional effect matchers to
MobiusTestandMobiusNimble:hasOnlyEffects/haveOnlyEffects: Enforces that only the expected effects are present, regardless of orderhasExactlyEffects/haveExactlyEffects: Enforces that only the expected effects are present, in the specified order