Fix @Verify and @VerifyAll method compilation (#2150)

[Vampire]

Fixes #2150

[Vampire]

• Fix @Verify and @VerifyAll method compilation (#2150) #2249 👈 (View in Graphite)
• master

This stack of pull requests is managed by Graphite. Learn more about stacking.

[Vampire]

This hopefully fixes the mess with the @Verify and @VerifyAll methods

[codecov]

Codecov Report

❌ Patch coverage is 95.72650% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.13%. Comparing base (158e2e4) to head (4a031cd).

Files with missing lines	Patch %	Lines
...kframework/compiler/AbstractDeepBlockRewriter.java	50.00%	2 Missing ⚠️
.../spockframework/compiler/model/BlockParseInfo.java	50.00%	2 Missing ⚠️
...r/condition/DefaultConditionRewriterResources.java	80.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #2249      +/-   ##
============================================
+ Coverage     82.07%   82.13%   +0.05%     
- Complexity     4754     4786      +32     
============================================
  Files           465      468       +3     
  Lines         14872    14942      +70     
  Branches       1877     1888      +11     
============================================
+ Hits          12206    12272      +66     
- Misses         1978     1983       +5     
+ Partials        688      687       -1     

Files with missing lines	Coverage Δ
...g/spockframework/compiler/AbstractSpecVisitor.java	68.75% <100.00%> (+2.08%)	⬆️
...java/org/spockframework/compiler/AstNodeCache.java	100.00% <100.00%> (ø)
...org/spockframework/compiler/DeepBlockRewriter.java	98.24% <100.00%> (+0.05%)	⬆️
...g/spockframework/compiler/InteractionRewriter.java	91.79% <100.00%> (ø)
...n/java/org/spockframework/compiler/SpecParser.java	95.38% <100.00%> (+0.64%)	⬆️
...java/org/spockframework/compiler/SpecRewriter.java	94.17% <100.00%> (+0.41%)	⬆️
...org/spockframework/compiler/SpecialMethodCall.java	93.80% <100.00%> (+0.11%)	⬆️
...va/org/spockframework/compiler/SpockTransform.java	86.95% <100.00%> (ø)
...rg/spockframework/compiler/WhereBlockRewriter.java	94.77% <100.00%> (ø)
...k/compiler/condition/BaseVerifyMethodRewriter.java	100.00% <100.00%> (ø)
... and 11 more

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[leonard84]

These don't apply outside of a spec, though.

[Vampire]

That's correct for captureOldValue and getMockInvocationMatcher, but I wanted to reuse the DeepBlockRewriter as it does exactly what we want and trying to reimplement part of its logic is exactly why the current state is as broken as it is. The DeepBlockRewriter needs those and I didn't want to rip apart the whole DeepBlockRewriter into two halves.

For the non-spec method case, these just throw UnsupportedOperationException and return null respectiviely, which is fine, as the code paths in the DeepBlockRewriter that call these methods can never be hit from those verify method transforms, as captureOldValue can only be triggered from then: block and getMockInvocationMatcher is only called in feature methods and on rewriting interactions, which are now explicitly forbidden in those verification methods just like in expect blocks.

Another possibility would be to have them in a sub-interface and make an instance-of check and cast in the DeepBlockRewriter, but I thought this solution is better and also more performant.

[leonard84]

Shouldn't we at least use a more useful error message in that case, i.e., that it is only supported by SpecRewriter/ within a Specification?

[Vampire]

For whom?
If the error happens, it is a Spock implementation bug.
The user is not able to trigger this error from a verification method.
If this is ever changed - which is pretty unlikely - then this spot will most probably be adapted anyway.

[leonard84]

For future us 🙂

[Vampire]

Ok, I changed it to throw new UnsupportedOperationException("This should only be called when processing a then-block"); for the old-case.

For the mock invocation matcher I didn't because I lied. :-)
That I did not already throw an UOE there is, because it does get called, but then the result is ignored in case of a VerifyBlock.
And the order cannot be swapped, because the result is used for an up-front check whether it is an interaction at all, before then complaining that you must not have one in expect or verify.

[AndreasTu]

LGTM.
Thank you.