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

Rework spying on concrete instances #800

Merged
merged 1 commit into from Dec 31, 2017

Conversation

leonard84
Copy link
Member

@leonard84 leonard84 commented Dec 25, 2017

This fixes #771

@codecov
Copy link

codecov bot commented Dec 25, 2017

Codecov Report

Merging #800 into master will decrease coverage by 0.01%.
The diff coverage is 88.57%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #800      +/-   ##
============================================
- Coverage     74.43%   74.42%   -0.02%     
- Complexity     3321     3324       +3     
============================================
  Files           359      359              
  Lines         10344    10355      +11     
  Branches       1288     1291       +3     
============================================
+ Hits           7700     7707       +7     
- Misses         2176     2178       +2     
- Partials        468      470       +2
Impacted Files Coverage Δ Complexity Δ
...framework/mock/runtime/ByteBuddyMethodInvoker.java 44.44% <ø> (-26.99%) 2 <0> (-1)
...va/org/spockframework/mock/runtime/MockObject.java 91.42% <ø> (-0.88%) 23 <0> (-2)
...ckframework/spring/mock/DelegatingInterceptor.java 54.05% <ø> (+1.42%) 6 <0> (ø) ⬇️
...ockframework/mock/runtime/JavaMockInterceptor.java 96.87% <100%> (ø) 14 <0> (ø) ⬇️
...ckframework/mock/runtime/CompositeMockFactory.java 56.25% <100%> (ø) 5 <1> (ø) ⬇️
...framework/mock/runtime/CglibRealMethodInvoker.java 57.14% <100%> (-9.53%) 2 <1> (-1)
...g/spockframework/mock/runtime/JavaMockFactory.java 100% <100%> (ø) 11 <1> (+2) ⬆️
...spockframework/mock/runtime/GroovyMockFactory.java 82.35% <75%> (-4.75%) 12 <1> (+1)
...n/java/org/spockframework/util/ReflectionUtil.java 77.47% <88.23%> (+1.94%) 62 <4> (+4) ⬆️

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 c60373c...4943565. Read the comment docs.

@leonard84
Copy link
Member Author

@twicksell I'd like you to review it, since you did the original implementation with #695

@@ -113,6 +113,18 @@ class JavaSpies extends Specification {
result == "singing, singing"
}

def "sping on concrete instances can use partial mocking"() {
Copy link
Member

Choose a reason for hiding this comment

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

You could link to #771 here (@Issue)

@@ -69,11 +76,15 @@ public void run() {
}

IProxyBasedMockInterceptor mockInterceptor = new GroovyMockInterceptor(configuration, specification, newMetaClass);
ArrayList<Class<?>> additionalInterfaces = new ArrayList<>(configuration.getAdditionalInterfaces());
List<Class<?>> additionalInterfaces = new ArrayList<>(configuration.getAdditionalInterfaces());
Copy link
Member

Choose a reason for hiding this comment

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

Not related, but it could be inlined

import org.spockframework.mock.MockImplementation;
import org.spockframework.mock.MockNature;
import org.spockframework.runtime.GroovyRuntimeUtil;
import org.spockframework.util.ReflectionUtil;
Copy link
Member

Choose a reason for hiding this comment

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

+1 for not using *-import

Copy link
Member Author

Choose a reason for hiding this comment

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

actually that is something that I still need to fix, the new "official" style will prefer *-imports if it is more than 3 IIRC

Copy link
Member

Choose a reason for hiding this comment

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

I tended to like *-imports in the past, but over time I realized that it can hide complexity of a given class, regarding number of class level dependencies. There could be 5 star imports which under the hood import 40+ classes. While in some technical/utility/controller classes it can be fine/sane/required, usually it is good to be aware of that (especially that Idea hides imports by default, so it's doesn't reduce readability or effectiveness of working with that files). That's my two cents.

Copy link
Member

@szpak szpak left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@twicksell twicksell left a comment

Choose a reason for hiding this comment

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

LGTM!

@leonard84 leonard84 merged commit 635401d into spockframework:master Dec 31, 2017
@leonard84 leonard84 deleted the spy-rework branch December 31, 2017 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spying on externally created objects doesn't handle internal calls of stubbed methods
3 participants