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

Stub creates Spy when a method is final #2039

Open
vprudnikov opened this issue Nov 5, 2024 · 8 comments · May be fixed by #2040
Open

Stub creates Spy when a method is final #2039

vprudnikov opened this issue Nov 5, 2024 · 8 comments · May be fixed by #2040

Comments

@vprudnikov
Copy link

Describe the bug

Unexpectedly, a Stub turns into a Spy when creating it for a class with a final method.

To Reproduce

Try to create a Stub for org.springframework.security.oauth2.server.resource.authentication.JwtAuthenticationConverter like

def converter = Stub(JwtAuthenticationConverter) {
        convert(_ as Jwt) >> { println('I'm a stubbed method') }
}
  • And call the converter#convert method.

Expected behavior

Either the method gets stubbed or an exception while creating a Stub saying that the stub cannot be created.

Actual behavior

A new spied instance of JwtAuthenticationConverter is created and the real convert method is called.

Java version

21

Buildtool version

3.9.6

What operating system are you using

Windows

Dependencies

spock-spring:2.4-M1-groovy-4.0

Additional context

No response

@vprudnikov vprudnikov added the bug label Nov 5, 2024
@AndreasTu
Copy link
Member

AndreasTu commented Nov 5, 2024

The default mock maker can't mock final methods, you need to use the MockMakers.mockito:

def converter = Stub(JwtAuthenticationConverter, mockMaker: MockMakers.mockito) {
        convert(_ as Jwt) >> { println('I'm a stubbed method') }
}

Please see documentation for more details about the different mock makers and their capabilities. Note: the MockMakers.mockito was introduced with version 2.4-M2, so please upgrade your version to probably 2.4-M4.

You could also change the default used mock maker to mockito, if you do not want to specify it every time.

@AndreasTu AndreasTu added not a bug and removed bug labels Nov 5, 2024
@vprudnikov
Copy link
Author

@AndreasTu , thanks for the clarification.
I wonder if it's possible to throw an error if the Stubbing of final methods doesn't make sense using a chosen mock maker.
The rationale is to provide quick feedback to a developer rather than creating a Spy. In my case. I spent one hour on debugging. It was a big gotcha.

What do you think about it?
Thank you.

@AndreasTu
Copy link
Member

@vprudnikov Not really, because how do we not that we mock a class with transitive final methods, because every class has final methods e.g. Object.clone(), so the byte-buddy mock maker would be useless for every class to mock, if we would report an error that way.

For example final classes the system would report such an error, but for final methods it is hard to know that it was intended or not.
We only know it, when an interactions tries to override a final behavior, but then the mock was already created.

Also the mockito mock maker can't mock other stuff, which the byte-buddy could, so we can't just replace the one with the other. This would break other use cases and existing tests.

Mocking final stuff e.g. classes and/or methods is a sad story.

@Vampire
Copy link
Member

Vampire commented Nov 6, 2024

The docs even explicitly warn about it with an admonition reading

If you try to mock a final method without a Mock Maker supporting it, it will silently fail, without honoring your specified interactions.

@AndreasTu
Copy link
Member

Mmm maybe we could throw an exception, when the user specifies an interaction on a final method, which will never trigger, if the mock maker did not support final methods.

This would not prevent the general possible fault, but would ease the pain when someone does something like above.
I am not sure if that would be implementable, but just as a thought experiment.
The interresting part will be the fuzzy match for method overloads, _, type matcher etc.

@Vampire What do you think?

Nonetheless, the new exception would be a semantic break, but we could argue, that the old user code was already broken.

@Vampire
Copy link
Member

Vampire commented Nov 6, 2024

Feel free to give it a try.
I seem to remember having had a look at something similar in the past and giving it up, but I could mix up things.
Inexact matches are of course a problem.
Not only in arguments, also the method name can be a matcher.
And it probably means additional significant overhead for each and every interaction as you would most probably have to check at runtime whether the method is final.
I fear that the small confusion will not be worth the effort and especially overhead, but feel free to have a take on it if you like. :-)

AndreasTu added a commit to AndreasTu/spock that referenced this issue 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 IMockInteractionValidation interface allows IMockMakers
to implement different kinds of validations on mock interactions.

Fixes spockframework#2039
AndreasTu added a commit to AndreasTu/spock that referenced this issue 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 IMockInteractionValidation interface allows IMockMakers
to implement different kinds of validations on mock interactions.

Fixes spockframework#2039
AndreasTu added a commit to AndreasTu/spock that referenced this issue 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 IMockInteractionValidation interface allows IMockMakers
to implement different kinds of validations on mock interactions.

Fixes spockframework#2039
AndreasTu added a commit to AndreasTu/spock that referenced this issue 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 IMockInteractionValidation interface allows IMockMakers
to implement different kinds of validations on mock interactions.

Fixes spockframework#2039
AndreasTu added a commit to AndreasTu/spock that referenced this issue Nov 10, 2024
Add error reporting code to the byte-buddy` mock maker
to report interaction on final methods, which can't be handled.

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

Fixes spockframework#2039
AndreasTu added a commit to AndreasTu/spock that referenced this issue Nov 10, 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.

Fixes spockframework#2039
@vprudnikov
Copy link
Author

I have some simple thoughts from a developer's perspective. And I hope you agree :)

  • If I want to create a Stub, I expect to get a Stub. Otherwise, there should be an error saying why the framework didn't manage to provide the Stub for me. In other words, the contract is never broken.
  • The implementation details shouldn't leak into the contract. If the default mock maker can't provide a Stub for me, I expect to see an error with a suggestion to use another mock maker or remove the final keyword. If a Mockito mock maker can provide a Stub, it just does the job.
  • On the other hand, getting a Spy instead of Stub is quite unexpected behavior which leads to unpredictable results.

Hope it makes sense :)

@AndreasTu
Copy link
Member

@vprudnikov I fully agree as "developer's perspective", but IMHO this can't be implemented.

On the other hand, getting a Spy instead of Stub is quite unexpected behavior which leads to unpredictable results.
Hope it makes sense :)

From the implementation perspective there is no distinction, and we can't make that distinction.
The byte-buddy mockmacker subclasses an existing class, if something is final, we can't "incept" such method calls.

Every Mock/Spy/Stub is under the hood the same, because the inner workings do that, is the same.
What makes a Spy to a Stub, we intercept every method call with default responses.
So the creation of any class containing a single final method (e.g. every Object), can't be Stubed with your hard requirement. because we can't guarantee, that we could intercept the method to return a stub value instead of the real one.

The implementation details shouldn't leak into the contract.

Sure, but the detail is a Java JVM thing, what we can't change.
There are only two ways to do mocking of classes in the JVM world, subclass the class (byte-buddy way) or rewrite the class inline mockito mock maker with a Java Agent.
And the byte-buddy way suffers from the final method issue.

Otherwise, there should be an error saying why the framework didn't manage to provide the Stub for me. In other words, the contract is never broken.

So the answer of the byte-buddy mock maker would always be no, we can't provide that. This would also not help..

If you want your behavior, please use the mockito mock maker as a default.
With the byte-buddy one, we can't provide that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants