Visitar URL original
Fixes #2905 : ThreadLocal classes can be mocked. by robertotru · Pull Request #2908 · mockito/mockito · GitHub
Skip to content

Fixes #2905 : ThreadLocal classes can be mocked. - #2908

Merged
TimvdLippe merged 3 commits into
mockito:mainfrom
robertotru:bugifx/#2905-stackoverflow-when-mocking-ThreadLocal
Feb 11, 2023
Merged

TimvdLippe merged 3 commits into
mockito:mainfrom
robertotru:bugifx/#2905-stackoverflow-when-mocking-ThreadLocal

Conversation

@robertotru

@robertotru robertotru commented Feb 8, 2023 •

Copy link
Copy Markdown
Contributor

Fixes #2905

Changes

  • MockMethodAdvice way of checking if an object was a mock didn't work for ThreadLocal classes. The only change here is in method MockMethodAdvice#isMocked(Object) where the two conditions are flipped in order to avoid hitting the get() method from ThreadLocal used to check if the object was calling itself.

Note: this was working in Mockito 4.9.0, didn't check what happens in 5.0.0 and 5.1.0.

Checklist

  • Read the contributing guide
  • PR should be motivated, i.e. what does it fix, why, and if relevant how
  • If possible / relevant include an example in the description, that could help all readers
    including project members to get a better picture of the change
  • Avoid other runtime dependencies
  • Meaningful commit history ; intention is important please rebase your commit history so that each
    commit is meaningful and help the people that will explore a change in 2 years
  • The pull request follows coding style
  • Mention Fixes #<issue number> in the description if relevant
  • At least one commit should mention Fixes #<issue number> if relevant

@robertotru

Copy link
Copy Markdown
Contributor Author

Hello,

This PR is a possible fix for the issue reported in #2905
Hope it helps.
Best
R

@raphw

raphw commented Feb 11, 2023

Copy link
Copy Markdown
Member

While this will solve the issue for thread local classes, it will introduce it for map objects. Let me have a look if this can be solved differently.

@TimvdLippe

Copy link
Copy Markdown
Contributor

it will introduce it for map objects

Can we make sure we have a regression test for that as well? I would have merged this PR since all tests are green. Would be good to codify this requirement to ensure we don't regress.

@robertotru

Copy link
Copy Markdown
Contributor Author

@TimvdLippe and @raphw

Got it. I can add additional tests, just give me a list of use cases you want me to cover.

Best
R

@raphw

raphw commented Feb 11, 2023

Copy link
Copy Markdown
Member

Turns out the recursion check is one level deeper. For the case I meant, the self-call-check is pointless anyways. Turning this the way as suggested should not be an issue.

@raphw
raphw self-requested a review February 11, 2023 13:57
@TimvdLippe
TimvdLippe merged commit 5153526 into mockito:main Feb 11, 2023
@robertotru

Copy link
Copy Markdown
Contributor Author

Thanks a lot for the quick processing.

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.

StackOverflow while mocking a ThreadLocal on Mockito 5.1.1

3 participants