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

Best-effort error reporting for interactions on final methods #2040

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

AndreasTu
Copy link
Member

@AndreasTu AndreasTu commented Nov 7, 2024

Add error reporting code to the byte-buddy mock maker to report interaction on final methods, which can't be handled.

The IMockInteractionValidator interface allows IMockMakers to implement different kinds of validations on mock interactions.

Copy link

codecov bot commented Nov 7, 2024

Codecov Report

Attention: Patch coverage is 92.79279% with 8 lines in your changes missing coverage. Please review.

Project coverage is 81.89%. Comparing base (2c7db77) to head (5999e3b).
Report is 174 commits behind head on master.

Files with missing lines Patch % Lines
...va/org/spockframework/mock/runtime/MockObject.java 71.42% 1 Missing and 3 partials ⚠️
...ock/runtime/ByteBuddyMockInteractionValidator.java 95.74% 1 Missing and 1 partial ⚠️
...ckframework/spring/mock/DelegatingInterceptor.java 33.33% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #2040      +/-   ##
============================================
+ Coverage     80.44%   81.89%   +1.44%     
- Complexity     4337     4651     +314     
============================================
  Files           441      450       +9     
  Lines         13534    14531     +997     
  Branches       1707     1835     +128     
============================================
+ Hits          10888    11900    +1012     
+ Misses         2008     1954      -54     
- Partials        638      677      +39     

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

@AndreasTu AndreasTu force-pushed the fix_2039 branch 2 times, most recently from 2b54442 to a50eb7f Compare November 7, 2024 13:04
}

@Issue("https://github.com/spockframework/spock/issues/2039")
def "cannot stub final methods with byteBuddy without error message when one overload is non final"() {
Copy link
Member

Choose a reason for hiding this comment

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

The method name suggests that we should get an error here, but the test does not reflect that.

Copy link
Member Author

Choose a reason for hiding this comment

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

It fails to mock the method,. because one of the overloads is final, so there is not error message, because the parameters could have matched both the final or the non-final method, so we can't detect that right now => No error message.

This is the best-effort part of this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but this doesn't become obvious. The confusing part is what without error message refers to. It can be read in two ways: one, that you can't do this without getting an error message, or it is a scenario where you don't expect to get an error message.

Copy link
Member

Choose a reason for hiding this comment

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

It might help to use block labels in the test.

* @author Andreas Turban
*/
@ThreadSafe
final class ByteBuddyMockInteractionValidation implements IMockInteractionValidation {
Copy link
Member

Choose a reason for hiding this comment

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

Is this really specific to ByteBuddy, or does it apply to CgLib as well?

Copy link
Member Author

@AndreasTu AndreasTu Nov 9, 2024

Choose a reason for hiding this comment

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

It would also work for the cglib mock maker, but I did not want to implement the static field cache in cglib, because it is already deprecated.

If you want that supported, let me know, then I will have a look.

@Vampire
Copy link
Member

Vampire commented Nov 8, 2024

I didn't have time for a proper review, but is the validation done at compile-time or runtime?
If it was done at runtime, how much is the performance impact?

@AndreasTu
Copy link
Member Author

@Vampire It is done at runtime.
During compile-time, it would not be possible to know the used mock maker, therefore it would lead to false positives, if the user would use e.g. mockito or an external mock maker.

During runtime, the expensive operation of calculating the final methods is done, once per class and only if an interaction is specified. Note: Currently I am only checking for the public finals, => class.getMethods() not for anything else, as I said best-effort. But this could be extended in the future.

The final methods Set cache is attached to the generated mock class as static field, so the cache is GCed without any weakrefs etc., when the mock class is GCed.

For each interaction specification the cost is to loop over the contained IInvocationConstraints in the IMockInteraction and make a Set lookup for the method name, and two Set lookups for a property name, if the byteBuddy mock maker is used.

For mockito and java-proxy there is not overhead, except of the getter of the spock mock obj and a null check.

Add error reporting code to the byte-buddy` mock maker
to report interaction on final methods, which can't be handled.

The IMockInteractionValidator interface allows IMockMakers
to implement different kinds of validations on mock interactions.

Fixes spockframework#2039
}

@Issue("https://github.com/spockframework/spock/issues/2039")
def "cannot stub final methods with byteBuddy without error message when one overload is non final"() {
Copy link
Member

Choose a reason for hiding this comment

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

Yes, but this doesn't become obvious. The confusing part is what without error message refers to. It can be read in two ways: one, that you can't do this without getting an error message, or it is a scenario where you don't expect to get an error message.

@AndreasTu AndreasTu requested a review from leonard84 December 19, 2024 17:44
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.

3 participants