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.

Sign up for GitHub

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

Jump to bottom

ByteBuddyMockFactory adds @Internal annotation to Groovy MOP methods #1729

Merged
merged 11 commits into from
Sep 15, 2023
Merged

ByteBuddyMockFactory adds @Internal annotation to Groovy MOP methods #1729

merged 11 commits into from
Sep 15, 2023

Conversation

AndreasTu
Copy link
Member

@AndreasTu AndreasTu commented Jul 31, 2023 •
edited
Loading

The Groovy 3 & 4 runtime expects to have the @Internal annotation on the MOP methods from GroovyObject. That AbstractCallSite.createGroovyObjectGetPropertySite() processes it as GroovyObject. So the ByteBuddyMockFactory now adds the @Internal annotation to the intercepted MOP methods.

After that the "Unable to access protected constant when spying instances" problems are gone, because we take the normal Groovy route.

Also some strange inconsistencies for Groovy 2 <=> 3,4 are now gone, but a new inconsistency appeared Groovy 4 prefers is over get for boolean. But this is not a Spock issue, because in Groovy only code the same thing happens. See tests in JavaMocksForGroovyClasses.

This fixes #1501, #1452, #1608 and #1145.

@codecov
Copy link

codecov bot commented Jul 31, 2023 •
edited
Loading

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.02% 🎉

Comparison is base (7b8863a) 79.83% compared to head (358509d) 79.85%.

Additional details and impacted files 
@@             Coverage Diff              @@
##             master    #1729      +/-   ##
============================================
+ Coverage     79.83%   79.85%   +0.02%     
- Complexity     4084     4096      +12     
============================================
  Files           425      425              
  Lines         12953    12964      +11     
  Branches       1640     1642       +2     
============================================
+ Hits          10341    10353      +12     
+ Misses         2000     1999       -1     
  Partials        612      612
Files Changed Coverage Δ
...ockframework/mock/runtime/BaseMockInterceptor.java 85.71% <100.00%> (-1.79%) ⬇️
...ckframework/mock/runtime/ByteBuddyMockFactory.java 91.54% <100.00%> (+1.38%) ⬆️
...ockframework/mock/runtime/GroovyMockMetaClass.java 87.23% <100.00%> (+0.87%) ⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@AndreasTu AndreasTu force-pushed the AccessProtectedConst branch 2 times, most recently from 70bc712 to ff8b78c Compare July 31, 2023 09:52
@AndreasTu AndreasTu mentioned this pull request Jul 31, 2023
Closed
@AndreasTu AndreasTu changed the title ByteBuddyMockFactory adds @Internal annotation to Groovy MOP methods ByteBuddyMockFactory adds @Internal annotation to Groovy MOP methods Jul 31, 2023
This was referenced Jul 31, 2023
Closed
Closed
Closed
@AndreasTu AndreasTu force-pushed the AccessProtectedConst branch 3 times, most recently from 12de4f8 to 99306f3 Compare August 4, 2023 09:21
leonard84
leonard84 requested changes Aug 4, 2023
View reviewed changes
Copy link
Member

@leonard84 leonard84 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, just some small changes

spock-core/src/main/java/org/spockframework/mock/runtime/ByteBuddyMockFactory.java Outdated Show resolved Hide resolved
spock-specs/src/test/groovy/org/spockframework/mock/AccessProtectedPropsSpec.groovy Outdated Show resolved Hide resolved
spock-specs/src/test/groovy/org/spockframework/smoke/mock/JavaMocksForGroovyClasses.groovy Outdated Show resolved Hide resolved
spock-specs/src/test/groovy/org/spockframework/smoke/mock/JavaMocksForGroovyClasses.groovy Outdated Show resolved Hide resolved
@AndreasTu AndreasTu self-assigned this Aug 14, 2023
Vampire
Vampire requested changes Aug 25, 2023
View reviewed changes
Copy link
Member

@Vampire Vampire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mentioned this will fix #1608, but no @Issue mentions it.
If an already existing test covers it, please add a second @Issue annotation, if not please add a test for that case.

spock-core/src/main/java/org/spockframework/mock/runtime/ByteBuddyMockFactory.java Outdated Show resolved Hide resolved
spock-core/src/main/java/org/spockframework/mock/runtime/ByteBuddyMockFactory.java Outdated Show resolved Hide resolved
spock-core/src/main/java/org/spockframework/mock/runtime/ByteBuddyMockFactory.java Outdated Show resolved Hide resolved
spock-core/src/main/java/org/spockframework/mock/runtime/ByteBuddyMockFactory.java Show resolved Hide resolved
spock-core/src/main/java/org/spockframework/mock/runtime/ByteBuddyMockFactory.java Outdated Show resolved Hide resolved
spock-core/src/main/java/org/spockframework/mock/runtime/ByteBuddyMockFactory.java Outdated Show resolved Hide resolved
spock-specs/src/test/groovy/org/spockframework/mock/MockMapDelegateSpec.groovy Show resolved Hide resolved
spock-core/src/main/java/org/spockframework/mock/runtime/ByteBuddyMockFactory.java Outdated Show resolved Hide resolved
@AndreasTu @Vampire
ByteBuddyMockFactory adds @internal annotation to Groovy MOP methods
95c6ee5 
The Groovy 3 & 4 runtime expects to have the @internal annotation on the MOP methods from GroovyObject.
That AbstractCallSite.createGroovyObjectGetPropertySite() processes it as GroovyObject.
So the ByteBuddyMockFactory now adds the @internal annotation to the intercepted MOP methods.

After that the "Unable to access protected constant when spying instances" problems are gone,
because we take the normal Groovy route.

Also some strange inconsistencies for Groovy  2 <=> 3,4 are now gone,
but a new inconsistency appeared Groovy 4 prefers is over get for boolean.
But this is not a Spock issue, because in Groovy only code the same thing happens.
See tests in JavaMocksForGroovyClasses.

The test "Access protected fields Issue spockframework#1452" covers the
"MissingMethodException when accessing fields without explicit @" issue.

This fixes spockframework#1145, spockframework#1452, spockframework#1501, spockframework#1608

Co-authored-by: Björn Kautler <[email protected]>
@AndreasTu AndreasTu requested a review from Vampire August 25, 2023 13:37
Vampire
Vampire requested changes Aug 25, 2023
View reviewed changes
Copy link
Member

@Vampire Vampire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The more often I look, the more I find. 🙈

spock-specs/src/test/groovy/org/spockframework/mock/AccessProtectedPropsSpec.groovy Outdated Show resolved Hide resolved
spock-specs/src/test/groovy/org/spockframework/mock/AccessProtectedPropsSpec.groovy Outdated Show resolved Hide resolved
spock-specs/src/test/groovy/org/spockframework/mock/AccessProtectedPropsSpec.groovy Outdated Show resolved Hide resolved
spock-specs/src/test/groovy/org/spockframework/mock/AccessProtectedPropsSpec.groovy Outdated Show resolved Hide resolved
spock-specs/src/test/groovy/org/spockframework/mock/AccessProtectedPropsSpec.groovy Outdated Show resolved Hide resolved
spock-specs/src/test/groovy/org/spockframework/mock/AccessProtectedPropsSpec.groovy Outdated Show resolved Hide resolved
spock-specs/src/test/groovy/org/spockframework/mock/MockMapDelegateSpec.groovy Outdated Show resolved Hide resolved
spock-specs/src/test/groovy/org/spockframework/mock/AccessProtectedJavaBaseClass.java Outdated Show resolved Hide resolved
@AndreasTu AndreasTu requested a review from Vampire August 25, 2023 15:03
Vampire
Vampire previously approved these changes Aug 25, 2023
View reviewed changes
Copy link
Member

@Vampire Vampire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thx

@leonard84
Merge remote-tracking branch 'spockframework/master' into AccessProte…
a7f1c1b 
…ctedConst

* spockframework/master:
  Use multiple locks in ByteBuddyMockFactory to reduce lock contention (spockframework#1778)
  Fixup LICENSE file after change to geantyref (spockframework#1773)

# Conflicts:
#	spock-core/src/main/java/org/spockframework/mock/runtime/ByteBuddyMockFactory.java
@leonard84 leonard84 merged commit 12682bb into spockframework:master Sep 15, 2023
21 checks passed
@leonard84
Copy link
Member

Thanks @AndreasTu

@AndreasTu AndreasTu deleted the AccessProtectedConst branch September 15, 2023 17:34
@AndreasTu AndreasTu added this to the 2.4 milestone Feb 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leonard84 leonard84 leonard84 approved these changes

@Vampire Vampire Vampire left review comments

Assignees

@AndreasTu AndreasTu

Labels
None yet
Projects
None yet
Milestone
2.4-M2
Development

Successfully merging this pull request may close these issues.

Unable to access protected constant when spying instances
3 participants
@AndreasTu @leonard84 @Vampire