Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Groovy 3 compatibility #1075

Merged
merged 23 commits into from Feb 10, 2020
Merged

Groovy 3 compatibility #1075

merged 23 commits into from Feb 10, 2020

Conversation

@szpak
Copy link
Member

szpak commented Jan 14, 2020

Summary

This pull request provides expermental Spock compatibility (compilation + tests) with Groovy 3.

In addition the Groovy 2.5 compatibility is kept. There are 2 variants available known from the previous Spock versions. There is a compatibility layer to cover breaking changes in Groovy 3.

The builds pass on Travis with Groovy 3.0 and 2.5 variants on supported range of Java versions.

Known issues/limitations (to be fixed - or acknowledged - before merge):

  • Unlike with the @NamedParam annotation, dummy classes LambdaExpression and MethodReferenceExpression have to be on runtime classpath with Groovy 2.5 (they are used as method arguments). It brings a runtime risk if groovy:3.x and spock-groovy2-compat:2.x-groovy-2.5 artifacts are accidentally mixed in one project (however, I don't know if we can do it another way).
  • Recently merged tests in AnonymousInnerClasses (#1027) fails with Groovy 3 - @Vampire, you are up-to-date with the changes, maybe you would like to take a look at it? - adjusted to Groovy 3 by @Vampire
  • CompileTimeErrorReporting."constructor declaration" test fails with Groovy 3 - the implementation of SpecParser.constructorMayHaveBeenAddedByCompiler() has to be improved. It's not a critical issue, so temporarily skipped it to finish making codebase cross-compatible. Contribution with that is welcome. - diagnosed by @Vampire, tracked in GROOVY-9360 - fixed by @Vampire.

Long term known issues (to be fixed before 2.0-final is released in separate PRs):

  • Groovy 3 allows to use += and -= in assertions (e.g. assert i += 2). This is not compatible with Groovy 2 and Spock's assumptions. I reported it GROOVY-9360 and most likely it would be reverted before 2.0-final
  • update to Groovy 3.0-final once released - upgraded
  • More lambada and method reference tests for Spock tests - contribution is welcome

Other issues/ideas:

  • I'm not satisfied with the way property calls are handled in mocks after changes in Groovy 3. I asked on the Groovy mailing list about some better ideas - tracked in #1076
  • JavaStubs tests could be rewritten to parameterized tests to test both interface and class-based stubs

Other remarks

It is recommended to merge this pull request WITHOUT squashing, to keep the original commit separation. They contain extra information about incompatibilities and, in addition, would be easier to selectively revert something, if needed.

Feel free to comment and propose possible improvements!


This change is Reviewable

@szpak szpak force-pushed the szpak/groovy3 branch from ec68edd to 33b01bb Jan 14, 2020
@codecov

This comment has been minimized.

Copy link

codecov bot commented Jan 14, 2020

Codecov Report

Merging #1075 into master will decrease coverage by 0.02%.
The diff coverage is 30.43%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1075      +/-   ##
============================================
- Coverage     74.02%   73.99%   -0.03%     
- Complexity     3437     3445       +8     
============================================
  Files           383      385       +2     
  Lines         10630    10657      +27     
  Branches       1297     1301       +4     
============================================
+ Hits           7869     7886      +17     
- Misses         2299     2309      +10     
  Partials        462      462
Impacted Files Coverage Δ Complexity Δ
...rk/compiler/ExpressionReplacingVisitorSupport.java 0% <ø> (ø) 0 <0> (ø) ⬇️
...aus/groovy/ast/expr/MethodReferenceExpression.java 0% <0%> (ø) 0 <0> (?)
...pockframework/runtime/ExpressionInfoConverter.java 89.15% <0%> (-1.72%) 34 <0> (ø)
...org/codehaus/groovy/ast/expr/LambdaExpression.java 0% <0%> (ø) 0 <0> (?)
...org/spockframework/compiler/ConditionRewriter.java 56.26% <0%> (-0.67%) 44 <0> (ø)
.../org/spockframework/runtime/GroovyRuntimeUtil.java 64.13% <50%> (+0.79%) 41 <2> (+3) ⬆️
...src/main/java/org/spockframework/util/MopUtil.java 24.13% <50%> (-0.43%) 10 <0> (ø)
...ockframework/mock/runtime/JavaMockInterceptor.java 92.3% <71.42%> (-4.57%) 16 <0> (+2)
...core/src/main/java/spock/util/environment/Jvm.java 79.16% <0%> (ø) 26% <0%> (ø) ⬇️
... and 6 more

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 ca2963a...68f204f. Read the comment docs.

@Vampire

This comment has been minimized.

Copy link
Member

Vampire commented Jan 14, 2020

The error has nothing to do with my changes.
If you change the test, so that the method name does not contain a dot and comment out my changes, you still get the same error about not finding a matching constructor.
Actually the constuctor is there in the class object, if you do getDeclaredConstructors().
Groovy though is not able to see it for whatever reason.
But as I said, this is not related to my changes, which just fix the enclosing method to the newly generated feature method if necessary.

Btw. I'm curious, is there a reason you used @FailsWith? Wouldn't @PendingFeature have been the better choice?

@szpak

This comment has been minimized.

Copy link
Member Author

szpak commented Jan 14, 2020

@Vampire I just said that you added those tests which started to fail after switch to Groovy 3 and therefore, I suspected, you might be more situation oriented than me. But if you are not, and you don't want to face the challenge, I can understand that ;-).

Btw, having your PR merged day or two later would cause it to fail before merge (or right after) ;-).

Btw. I'm curious, is there a reason you used @FailsWith? Wouldn't @PendingFeature have been the better choice?

The only reason is that I notoriously forget about that extension :). I changed it in some places, thanks for you suggestion.

@szpak szpak force-pushed the szpak/groovy3 branch from 2180b7c to d560077 Jan 14, 2020
build.gradle Outdated Show resolved Hide resolved
.editorconfig Outdated Show resolved Hide resolved
build.gradle Outdated Show resolved Hide resolved
//Groovy 3 started to call go.getProperty("x") method instead of go.getX() directly for go.x
Throwable throwable = new Throwable();
StackTraceElement mockCaller = throwable.getStackTrace()[3];
if (!("groovy.lang.GroovyObject$getProperty".equals(mockCaller.getClassName()) && "call".equals(mockCaller.getMethodName()))) {

This comment has been minimized.

Copy link
@leonard84

leonard84 Jan 14, 2020

Member

Please include a //HACK: regarding scanning the stacktrace, and maybe extend the if with a isGroovy3() check

This comment has been minimized.

Copy link
@szpak

szpak Jan 14, 2020

Author Member

There is a similar - already existing - "hack" with a stacktrace a few lines above for setProperty(). I should mark it as well in that case.

This comment has been minimized.

Copy link
@szpak

szpak Jan 15, 2020

Author Member

I overslept that case and - even it seems to work also for Groovy 2 - I agree it would be good to included isGroovy3() runtime check for that "hack". It will minimize a chance for regression for Groovy 2 and will allow us to release 2.0-M2 with more confidence.

This comment has been minimized.

Copy link
@szpak

szpak Jan 15, 2020

Author Member

Hmm, it turned out that the following fails with Groovy 2 and with 1.3-groovy-2.5:

    def "can stub property access"() {
      Person person = Stub(Person)
      person.getName() >> "fred"

      expect:
      person.name == "fred"
      person.getProperty("name") == "fred"  //fails with 1.3-groovy-2.5 and 2.0-M1-groovy-2.5 
    }

It doesn't seem to be often used if wasn't reported, so I may left it as it for Groovy 2.5.

However, for Groovy 3 my change is needed to have even:

      person.name == "fred"

passing. And along the way it fixes person.getProperty("name") == "fred".

Concluding, I keep the changes only for Groovy 3, waiting for some better idea to determine that in on the Groovy side (before 2.0-final is released).

@szpak szpak force-pushed the szpak/groovy3 branch from 72e6937 to b309e28 Jan 15, 2020
settings.gradle Outdated Show resolved Hide resolved
@szpak szpak force-pushed the szpak/groovy3 branch from b309e28 to 756cb6b Jan 15, 2020
@szpak szpak added this to the 2.0 milestone Feb 4, 2020
@szpak szpak force-pushed the szpak/groovy3 branch 2 times, most recently from 6278da3 to f4f93dc Feb 6, 2020
szpak added 14 commits Dec 31, 2019
To detect problems and know how problematic it will be.
Compiling, but with several broken tests.
Different exceptions in Groovy 2 and 3 are thrown.
Power assertions in Groovy 3 renders resolved values at
the beginning of the expression, not in the middle of it.
+= and -= are allowed in Groovy 3.0-rc2, but it should considered before
3.0-final. See https://issues.apache.org/jira/browse/GROOVY-9360 and
the linked discussion. Spock's tests should adjusted to it.
It fixed mock.getProperty("foo") stubbing while preserving previous
fix for property access to stubbed getters in Groovy 3.
The one related to constructor declaration. The implementation of
SpecParser.constructorMayHaveBeenAddedByCompiler no longer detect
specification constructor and should be enhanced to support Groovy 3.

As it is not a critical bug I postponed it to work on building the same
Spock code base with Groovy 2 and 3.
Those are two new tests, emerged after rebasing against master.
To be reviewed and fixed.
Added compatibility layer and conditional module inclusion to
keep codebase compilable (and buildable) with both Groovy 2.5 and 3.0.
szpak added 7 commits Jan 14, 2020
To detect compatibility problems faster.
Groovy script execution line in stacktrace format has changed since RC2:
script15791055931181262955476.groovy:3 ->
Script_eb303fa8835e8a0c4ec6558372940b3d.groovy:3
Fixed meanwhile in a separate branch.
@szpak szpak force-pushed the szpak/groovy3 branch from f4f93dc to ec92ba0 Feb 7, 2020
@szpak szpak force-pushed the szpak/groovy3 branch from ec92ba0 to 5def0a9 Feb 8, 2020
@leonard84 leonard84 merged commit bb8dd00 into master Feb 10, 2020
3 of 5 checks passed
3 of 5 checks passed
codecov/patch 30.43% of diff hit (target 74.02%)
Details
codecov/project 73.99% (-0.03%) compared to ca2963a
Details
ci/circleci: build Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.