Increase Groovy version to 3.0.6 and give condition closures access to the specification instance

[Vampire]

fixes #1177

---

This change is 

[codecov]

Codecov Report

Merging #1204 into master will increase coverage by 0.01%.
The diff coverage is 76.92%.

@@             Coverage Diff              @@
##             master    #1204      +/-   ##
============================================
+ Coverage     76.49%   76.50%   +0.01%     
- Complexity     3751     3754       +3     
============================================
  Files           405      405              
  Lines         11432    11437       +5     
  Branches       1396     1396              
============================================
+ Hits           8745     8750       +5     
  Misses         2199     2199              
  Partials        488      488              

Impacted Files	Coverage Δ	Complexity Δ
...runtime/extension/builtin/PreconditionContext.java	65.21% <33.33%> (-1.45%)	9.00 <0.00> (ø)
...untime/extension/builtin/ConditionalExtension.java	92.68% <100.00%> (+0.37%)	11.00 <4.00> (+3.00)
...untime/extension/builtin/RetryBaseInterceptor.java	89.18% <100.00%> (+0.61%)	16.00 <0.00> (ø)
...ntime/extension/builtin/RetryConditionContext.java	100.00% <100.00%> (ø)	3.00 <2.00> (ø)
...src/main/java/org/spockframework/util/MopUtil.java	24.56% <100.00%> (+0.42%)	10.00 <0.00> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 90d555d...6c8c49f. Read the comment docs.

[Vampire]

The failing test is, because this does not work anymore in 3.0.5 which worked fine in 3.0.4:

[Vampire]

https://issues.apache.org/jira/browse/GROOVY-9665

[Vampire]

Actually this was an intended change.
And unfortunately rightfully even if a bit breaking for a minor release maybe.
I see two problematic cases if I didn't miss something.
@IgnoreIf/@Requires/@PendingFeatureIf (hereafter @Requires case) and @Retry.

For both we create the closure instances without owner and thisObject, with the context object as delegate and with DELEGATE_ONLY as resolve strategy.

For the @Requires case there is usually no instance of the specification class available yet, so to make the syntax work again, we would need to set the owner to the test class and the resolve strategy to DELEGATE_FIRST.

For the @Retry case we could even be fancier.
When this is evaluated, we have the current test instance available as this happens only after test failure.
So we could set the actual test instance as owner and thisObject and the resolve strategy to DELEGATE_FIRST.
This would enable the condition to reference the static fields like it was up to now and also the fields of the specification instance actually.

We could also make @Retry only be able to resolve the static fields like before with using the specification class as owner like we could for @Requires, effectively blocking access to the specification fields even if we could provide it.

Or actually we could make the @Requires cases more fancy and allow access to the specification fields there too.
Similar to how I made the data variables of an iteration available in the @Requires closures we could ues a similar strategy for specification fields, delaying the evaluation of the condition until the values are available.

So to summarize the multiple-choice options we have are:

• keep it like it is, if someone wants to use a static field, he has to qualify the access with ClassName.FIELD, no access to instance fields
• change the @Requires handling to allow simple access to static fields only
• change the @Requires handling to allow access to static fields and instance fields
• change the @Retry handling to allow simple access to static fields only
• change the @Retry handling to allow access to static fields and instance fields

@leonard84 & @szpak what do you think?

[leonard84]

IMHO we should only set the class as owner, doing it for the instance causes the setup phase to run, e.g., starting a spring context, only to abort again. And you'd either have to scan the whole class for valid properties/methods beforehand or you'd continue only to fail later anyway.

[Vampire]

In the @Retry case when the condition is evaluated, the instance is ready already anyway, as it is only evaluated after a failed execution of the feature.

For the @Requires case I would make it like with the data-variable access.
Evaluate the closure early like now without the instance available, and if failed due to a MissingPropertyException, then re-evaluate later when the instance actually is available using a feature or iteration interceptor.

Actually the logic is already present almost completely due to the shiny new data variable support.
I think the only missing parts would be to rehydrate the closure with the test instance as owner and thisObject, changing the resolve strategy to DELEGATE_FIRST and lifting the restriction a bit to fail on more or all MissingPropertyExceptions instead of only when a data variable name was tried.

[leonard84]

So you would not inspect the class to see if the missing property would even be available? Same with missing method?

[Vampire]

No, I think that would be unnecessary complex, superfluous, wasted time and too error prone.

If it fails due to missing property or method, then it could be that it was not a valid call to the test instance, then it will do the check again later and fail again. In that case this means it is a programming error in the test, so it has to be fixed to use a valid property or method. Once the test is fixed, it will only fail with missing property or missing method exception if an correct access was made. Checking that it was a valid access before checking again later would only be a waste of processing, besides that it is too error prone with the dynamic capabilities of Groovy.

For the data variables the check was easy and reliable, as we know the data variables that exist and there is not much magic that could happen additionally.

[leonard84]

Any other thoughts @spockframework/supporter

[leonard84]

If the implementation is analogous to @Retry, i.e., that the instance variables are available via instance.var, then I'm ok with it. Furthermore, we could reliably determine if the MissingPropertyExceptions was targeted at instance.

[henrik242]

(Latest groovy is 3.0.6, by the way)

[Vampire]

You are right @henrik242, but there is no reason to depend on a newer Groovy version if there is not something fixed that is essential for Spock imho.
3.0.5 fixes #1177, so it is better to depend on it.
@leonard84 what do you think, should we depend on the latest, or on the oldest compatible?

[szpak]

For me, the newest version of Groovy the better. Usually there are more bugs fixed than new introduced ;). Some people just add Spock and have Groovy as a transitive dependency. Therefore, having that PR merged one day, I would also prefer 3.0.L(atest)

Nevertheless, it is rather more important to remember to do the dependency update right before the release. To keep them up-to-date.

Btw, if there is a suspicion that Spock depends on some Groovy feature added/fixed in some particular (minor) Groovy version there could be extra regression tests added. However, as I use them in my Gradle plugins (for Gradle versions, not Groovy), I'm definitely not convinced that Spock needs the same approach.

[leonard84]

we should mention that instance will require the test to execute to that point, similar as with the data-driven variables.

[leonard84]

Why do you pass the invocation and not the invocation.getInstance() here?

[Vampire]

Because it is done the same for RetryConditionContext, so I thought I just follow the same pattern here.
But I can of course also change that if you prefer.
If you do, should I change it for both?