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

AndreasTu · 2024-11-07T12:02:20Z

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.

codecov · 2024-11-07T12:08:47Z

Codecov Report

Attention: Patch coverage is 88.18898% with 15 lines in your changes missing coverage. Please review.

Project coverage is 81.84%. Comparing base (2c7db77) to head (c148647).
Report is 180 commits behind head on master.

Files with missing lines	Patch %	Lines
...untime/extension/builtin/FailsWithInterceptor.java	12.50%	4 Missing and 3 partials ⚠️
...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.84%   +1.39%     
- Complexity     4337     4686     +349     
============================================
  Files           441      452      +11     
  Lines         13534    14659    +1125     
  Branches       1707     1845     +138     
============================================
+ Hits          10888    11998    +1110     
+ Misses         2008     1979      -29     
- Partials        638      682      +44

Files with missing lines	Coverage Δ
...main/java/org/spockframework/compiler/AstUtil.java	`80.13% <ø> (+0.26%)`	⬆️
...java/org/spockframework/compiler/SpecRewriter.java	`93.91% <100.00%> (+0.21%)`	⬆️
...n/java/org/spockframework/compiler/SpockNames.java	`0.00% <ø> (ø)`
...in/java/org/spockframework/lang/SpecInternals.java	`86.86% <100.00%> (+5.85%)`	⬆️
...java/org/spockframework/mock/ISpockMockObject.java	`100.00% <100.00%> (ø)`
.../main/java/org/spockframework/mock/MockNature.java	`100.00% <100.00%> (ø)`
...rc/main/java/org/spockframework/mock/MockUtil.java	`100.00% <100.00%> (ø)`
...ork/mock/constraint/EqualMethodNameConstraint.java	`100.00% <100.00%> (ø)`
...k/mock/constraint/EqualPropertyNameConstraint.java	`100.00% <100.00%> (ø)`
...ockframework/mock/constraint/TargetConstraint.java	`100.00% <100.00%> (+6.66%)`	⬆️
... and 12 more

... and 20 files with indirect coverage changes

spock-core/src/main/java/org/spockframework/mock/runtime/BaseMockInterceptor.java

spock-core/src/main/java/org/spockframework/mock/runtime/GroovyMockMetaClass.java

spock-core/src/main/java/org/spockframework/mock/runtime/IMockInteractionValidation.java

spock-specs/src/test/groovy/org/spockframework/smoke/mock/JavaSpies.groovy

leonard84 · 2024-11-08T16:12:48Z

spock-specs/src/test/groovy/org/spockframework/smoke/mock/JavaStubs.groovy

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


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

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.

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.

It might help to use block labels in the test.

spock-core/src/main/java/org/spockframework/mock/runtime/IMockInteractionValidation.java

...k-core/src/main/java/org/spockframework/mock/runtime/ByteBuddyMockInteractionValidation.java

leonard84 · 2024-11-08T16:55:24Z

...k-core/src/main/java/org/spockframework/mock/runtime/ByteBuddyMockInteractionValidation.java

+ * @author Andreas Turban
+ */
+@ThreadSafe
+final class ByteBuddyMockInteractionValidation implements IMockInteractionValidation {


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

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.

spock-specs/src/test/groovy/org/spockframework/smoke/mock/JavaStubs.groovy

Vampire · 2024-11-08T17:21:58Z

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 · 2024-11-09T09:13:32Z

@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

spock-core/src/main/java/org/spockframework/mock/runtime/BaseMockInterceptor.java

leonard84 · 2024-12-19T17:18:47Z

spock-specs/src/test/groovy/org/spockframework/smoke/mock/JavaStubs.groovy

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


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.

spock-specs/src/test/groovy/org/spockframework/smoke/mock/JavaMocks.groovy

…ockInterceptor.java Co-authored-by: Leonard Brünings <leonard.bruenings@gradle.com>

…Mocks.groovy Co-authored-by: Leonard Brünings <leonard.bruenings@gradle.com>

mock creation message

AndreasTu · 2024-12-28T14:22:19Z

@leonard84 Thanks for the review and the changes!

leonard84 · 2024-12-28T14:26:08Z

You were too fast, I wanted to split out the @FailsWith into its own PR 😅 .

AndreasTu requested a review from Vampire November 7, 2024 12:02

AndreasTu self-assigned this Nov 7, 2024

AndreasTu force-pushed the fix_2039 branch from c1e69be to bd6f600 Compare November 7, 2024 12:04

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

leonard84 reviewed Nov 8, 2024

View reviewed changes

AndreasTu force-pushed the fix_2039 branch from a50eb7f to ca25614 Compare November 10, 2024 11:06

AndreasTu requested a review from leonard84 November 10, 2024 11:08

AndreasTu force-pushed the fix_2039 branch from ca25614 to 5d5a3e2 Compare November 10, 2024 11:14

leonard84 requested changes Dec 19, 2024

View reviewed changes

AndreasTu and others added 6 commits December 19, 2024 18:30

Update spock-core/src/main/java/org/spockframework/mock/runtime/BaseM…

1d796fe

…ockInterceptor.java Co-authored-by: Leonard Brünings <leonard.bruenings@gradle.com>

Update spock-specs/src/test/groovy/org/spockframework/smoke/mock/Java…

3ea9e4b

…Mocks.groovy Co-authored-by: Leonard Brünings <leonard.bruenings@gradle.com>

Update spock-specs/src/test/groovy/org/spockframework/smoke/mock/Java…

c5c08d0

…Mocks.groovy Co-authored-by: Leonard Brünings <leonard.bruenings@gradle.com>

Update spock-specs/src/test/groovy/org/spockframework/smoke/mock/Java…

92396d4

…Mocks.groovy Co-authored-by: Leonard Brünings <leonard.bruenings@gradle.com>

Merge remote-tracking branch 'upstream/master' into fix_2039

3d6bde5

Fix review finding.

8ad5c52

AndreasTu requested a review from leonard84 December 19, 2024 17:44

AndreasTu and others added 8 commits December 20, 2024 12:15

Merge remote-tracking branch 'upstream/master' into fix_2039

cc4160c

Merge remote-tracking branch 'upstream/master' into fix_2039

5999e3b

Merge remote-tracking branch 'upstream/master' into fix_2039

eb6da2b

Align indentation

2995d22

Fix release notes order

b252907

Add support for @FailsWith to assert errorMessage

c42e602

Align specification to what it does and improve...

7d2f94f

mock creation message

Fix minor things

c148647

leonard84 force-pushed the fix_2039 branch from 564b39a to c148647 Compare December 28, 2024 14:06

leonard84 approved these changes Dec 28, 2024

View reviewed changes

AndreasTu merged commit e081141 into spockframework:master Dec 28, 2024
29 checks passed

AndreasTu deleted the fix_2039 branch December 28, 2024 14:21

AndreasTu added this to the 2.4-M5 milestone Dec 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

AndreasTu commented Nov 7, 2024 •

edited

Loading

codecov bot commented Nov 7, 2024 •

edited

Loading

leonard84 Nov 8, 2024

AndreasTu Nov 10, 2024

leonard84 Dec 19, 2024

leonard84 Dec 19, 2024

leonard84 Nov 8, 2024

AndreasTu Nov 9, 2024 •

edited

Loading

Vampire commented Nov 8, 2024

AndreasTu commented Nov 9, 2024

leonard84 Dec 19, 2024

AndreasTu commented Dec 28, 2024

leonard84 commented Dec 28, 2024

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

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

Conversation

AndreasTu commented Nov 7, 2024 • edited Loading

codecov bot commented Nov 7, 2024 • edited Loading

Codecov Report

leonard84 Nov 8, 2024

Choose a reason for hiding this comment

AndreasTu Nov 10, 2024

Choose a reason for hiding this comment

leonard84 Dec 19, 2024

Choose a reason for hiding this comment

leonard84 Dec 19, 2024

Choose a reason for hiding this comment

leonard84 Nov 8, 2024

Choose a reason for hiding this comment

AndreasTu Nov 9, 2024 • edited Loading

Choose a reason for hiding this comment

Vampire commented Nov 8, 2024

AndreasTu commented Nov 9, 2024

leonard84 Dec 19, 2024

Choose a reason for hiding this comment

AndreasTu commented Dec 28, 2024

leonard84 commented Dec 28, 2024

AndreasTu commented Nov 7, 2024 •

edited

Loading

codecov bot commented Nov 7, 2024 •

edited

Loading

AndreasTu Nov 9, 2024 •

edited

Loading