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.

Sign up for GitHub

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

Jump to bottom

Add null check to workaround NPE in unit tests with Mockito/PowerMock #14006

Merged
merged 1 commit into from
Jan 28, 2022
Merged

Add null check to workaround NPE in unit tests with Mockito/PowerMock #14006

merged 1 commit into from
Jan 28, 2022

Conversation

lhotari
Copy link
Member

@lhotari lhotari commented Jan 28, 2022

Motivation

Workaround for a NPE issue that happens with Mockito/PowerMock:

org.mockito.exceptions.base.MockitoException: Unable to create mock instance of type 'ServerCnx'
	at org.apache.pulsar.broker.BrokerTestUtil.spyWithClassAndConstructorArgs(BrokerTestUtil.java:43)
	at 
	... 
	... 
	... 
Caused by: java.lang.reflect.InvocationTargetException
	at org.mockito.internal.creation.bytebuddy.ByteBuddyMockMaker.createMock(ByteBuddyMockMaker.java:43)
	at org.powermock.api.mockito.mockmaker.PowerMockMaker.createMock(PowerMockMaker.java:41)
	at org.mockito.internal.util.MockUtil.createMock(MockUtil.java:53)
	at org.mockito.internal.MockitoCore.mock(MockitoCore.java:84)
	at org.mockito.Mockito.mock(Mockito.java:1964)
	at org.apache.pulsar.broker.BrokerTestUtil.spyWithClassAndConstructorArgs(BrokerTestUtil.java:43)
	at org.apache.pulsar.broker.service.PersistentTopicTest.setup(PersistentTopicTest.java:211)
	at org.apache.pulsar.broker.service.persistent.PersistentTopicStreamingDispatcherTest.setup(PersistentTopicStreamingDispatcherTest.java:34)
	at jdk.internal.reflect.GeneratedMethodAccessor332.invoke(Unknown Source)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	... 34 more
Caused by: java.lang.NullPointerException
	at org.apache.pulsar.broker.service.ServerCnx.<init>(ServerCnx.java:234)
	at org.apache.pulsar.broker.service.ServerCnx.<init>(ServerCnx.java:230)
	at org.apache.pulsar.broker.service.ServerCnx$MockitoMock$985617261.<init>(Unknown Source)
	... 55 more

Modification

Add a null check to prevent the NPE.

@lhotari lhotari self-assigned this Jan 28, 2022
@lhotari lhotari mentioned this pull request Jan 28, 2022
Closed
@github-actions
Copy link

@lhotari:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

@lhotari lhotari added the doc-not-needed Your PR changes do not impact docs label Jan 28, 2022
@github-actions
Copy link

@lhotari:Thanks for providing doc info!

@michaeljmarshall
Copy link
Member

I believe this would also fix #13808.

@michaeljmarshall michaeljmarshall linked an issue Jan 28, 2022 that may be closed by this pull request
[branch-2.9] Flaky-test: PersistentTopicStreamingDispatcherTest #13808
Closed
@michaeljmarshall michaeljmarshall mentioned this pull request Jan 28, 2022
Closed
michaeljmarshall
michaeljmarshall approved these changes Jan 28, 2022
View reviewed changes
Copy link
Member

@michaeljmarshall michaeljmarshall left a comment

Choose a reason for hiding this comment

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

LGTM. If/when we remove the mocking that is responsible for these flaky tests, it'd be good to revert this change.

@lhotari lhotari merged commit e45b4f2 into apache:master Jan 28, 2022
codelipenghui pushed a commit that referenced this pull request Jan 30, 2022
@lhotari @codelipenghui
Add null check to workaround NPE in unit tests with Mockito/PowerMock (…
4691d3e 
…#14006)

- Fixes #13620

(cherry picked from commit e45b4f2)
Nicklee007 pushed a commit to Nicklee007/pulsar that referenced this pull request Apr 20, 2022
@lhotari
Add null check to workaround NPE in unit tests with Mockito/PowerMock (…
7b9ac49 
…apache#14006)

- Fixes apache#13620
@nicoloboschi nicoloboschi mentioned this pull request May 17, 2022
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eolivelli eolivelli eolivelli left review comments

@michaeljmarshall michaeljmarshall michaeljmarshall approved these changes

@codelipenghui codelipenghui Awaiting requested review from codelipenghui

Assignees

@lhotari lhotari

Labels
area/broker cherry-picked/branch-2.9 Archived: 2.9 is end of life doc-not-needed Your PR changes do not impact docs release/2.9.2 type/flaky-tests
Projects
None yet
Milestone
No milestone
5 participants
@lhotari @michaeljmarshall @eolivelli @codelipenghui @gaoran10