[test] Replace PowerMockito#mockStatic usages with mockito-inline #14098

nicoloboschi · 2022-02-02T23:27:11Z

Motivation

PowerMockito static mocks don't work well with the latest JDK class encapsulation mechanisms. The main problem is that PowerMockito, in order to mock a single static method of a class, try to setAccessible a lot of other apparently related classes. This means, starting from JDK17, you need to add --add-opens to the Java runtime options in order to avoid runtime errors.

PowerMockito development isn't much active and they don't seem to be ready for the new JDK releases.
The best replacement is to use mockito-inline (additional part of mockito-core), which uses a different algorithm in order to mock static methods - a lot less invasive.

Modifications

Replaced PowerMockito#mockStatic usages with MockedStatic
Added a Factory to NarClassLoader since it's not possible to mock ClassLoader classes in mockito-inline. The API for the NarClassLoader is internal so no problem changing it. See https://github.com/mockito/mockito/blob/main/src/main/java/org/mockito/internal/creation/bytebuddy/InlineDelegateByteBuddyMockMaker.java#L553

Documentation

no-need-doc

nicoloboschi · 2022-02-03T15:08:23Z

/pulsarbot rerun-failure-checks

nicoloboschi · 2022-02-04T23:05:34Z

/pulsarbot rerun-failure-checks

nicoloboschi · 2022-02-05T00:05:08Z

/pulsarbot rerun-failure-checks

nicoloboschi · 2022-02-07T08:37:31Z

/pulsarbot rerun-failure-checks

eolivelli

Great work.

I believe we have to rework that Factory class. I found it a bit unusual. I understand that it is for Mockito but we can probably organise it better

pulsar-common/src/main/java/org/apache/pulsar/common/nar/NarClassLoader.java

lhotari

did you try Mockito.mockStatic(NarClassLoader.class, Mockito.CALLS_REAL_METHODS) ? does that help address the limitation that you were describing about not being able to mock static methods on a classloader class?

nicoloboschi · 2022-02-08T22:50:21Z

@lhotari thanks for the suggestion. Unfortunately it doesn't help. I inspected the mockito-inline codebase and I haven't found a way to by-pass this check.

https://github.com/mockito/mockito/blob/c3d0847e6caee970e2db3ef77e1d31facf090550/src/main/java/org/mockito/internal/creation/bytebuddy/InlineDelegateByteBuddyMockMaker.java#L544-L558

lhotari

Really good work @nicoloboschi ! Awesome

eolivelli

Great work!

+1

nicoloboschi · 2022-02-09T17:58:00Z

/pulsarbot rerun-failure-checks

michaeljmarshall

LGTM

pulsar-common/src/main/java/org/apache/pulsar/common/protocol/Commands.java

nicoloboschi · 2022-02-09T21:29:21Z

/pulsarbot rerun-failure-checks

nicoloboschi · 2022-02-09T22:00:34Z

/pulsarbot rerun-failure-checks

nicoloboschi · 2022-02-09T22:43:48Z

/pulsarbot rerun-failure-checks

nicoloboschi · 2022-02-09T23:46:00Z

/pulsarbot rerun-failure-checks

nicoloboschi · 2022-02-10T01:52:14Z

/pulsarbot rerun-failure-checks

nicoloboschi · 2022-02-10T08:00:25Z

/pulsarbot rerun-failure-checks

nicoloboschi · 2022-02-10T18:24:53Z

/pulsarbot rerun-failure-checks

michaeljmarshall · 2022-02-10T22:04:08Z

@nicoloboschi - it looks like the broker test is failing with the below stack trace. I'm wondering if it is a legitimate failure and not just a flaky test.

Error:  Tests run: 6, Failures: 3, Errors: 0, Skipped: 3, Time elapsed: 0.063 s <<< FAILURE! - in org.apache.pulsar.broker.service.persistent.PersistentSubscriptionTest
Error:  setup(org.apache.pulsar.broker.service.persistent.PersistentSubscriptionTest)  Time elapsed: 0.023 s  <<< FAILURE!
org.mockito.exceptions.base.MockitoException: Unable to create mock instance of type 'PulsarService'
	at org.apache.pulsar.broker.BrokerTestUtil.spyWithClassAndConstructorArgs(BrokerTestUtil.java:43)
	at org.apache.pulsar.broker.service.persistent.PersistentSubscriptionTest.setup(PersistentSubscriptionTest.java:116)
	at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:132)
	at org.testng.internal.MethodInvocationHelper.invokeMethodConsideringTimeout(MethodInvocationHelper.java:61)
	at org.testng.internal.ConfigInvoker.invokeConfigurationMethod(ConfigInvoker.java:366)
	at org.testng.internal.ConfigInvoker.invokeConfigurations(ConfigInvoker.java:320)
	at org.testng.internal.TestInvoker.runConfigMethods(TestInvoker.java:701)
	at org.testng.internal.TestInvoker.invokeMethod(TestInvoker.java:527)
	at org.testng.internal.TestInvoker.invokeTestMethod(TestInvoker.java:174)
	at org.testng.internal.MethodRunner.runInSequence(MethodRunner.java:46)
	at org.testng.internal.TestInvoker$MethodInvocationAgent.invoke(TestInvoker.java:822)
	at org.testng.internal.TestInvoker.invokeTestMethods(TestInvoker.java:147)
	at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:146)
	at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:128)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1541)
	at org.testng.TestRunner.privateRun(TestRunner.java:764)
	at org.testng.TestRunner.run(TestRunner.java:585)
	at org.testng.SuiteRunner.runTest(SuiteRunner.java:384)
	at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:378)
	at org.testng.SuiteRunner.privateRun(SuiteRunner.java:337)
	at org.testng.SuiteRunner.run(SuiteRunner.java:286)
	at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:53)
	at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:96)
	at org.testng.TestNG.runSuitesSequentially(TestNG.java:1218)
	at org.testng.TestNG.runSuitesLocally(TestNG.java:1140)
	at org.testng.TestNG.runSuites(TestNG.java:1069)
	at org.testng.TestNG.run(TestNG.java:1037)
	at org.apache.maven.surefire.testng.TestNGExecutor.run(TestNGExecutor.java:135)
	at org.apache.maven.surefire.testng.TestNGDirectoryTestSuite.executeSingleClass(TestNGDirectoryTestSuite.java:112)
	at org.apache.maven.surefire.testng.TestNGDirectoryTestSuite.executeLazy(TestNGDirectoryTestSuite.java:123)
	at org.apache.maven.surefire.testng.TestNGDirectoryTestSuite.execute(TestNGDirectoryTestSuite.java:90)
	at org.apache.maven.surefire.testng.TestNGProvider.invoke(TestNGProvider.java:146)
	at org.apache.maven.surefire.booter.ForkedBooter.invokeProviderInSameClassLoader(ForkedBooter.java:384)
	at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:345)
	at org.apache.maven.surefire.booter.ForkedBooter.execute(ForkedBooter.java:126)
	at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:418)
Caused by: org.mockito.creation.instance.InstantiationException: 
Unable to create instance of 'PulsarService'.
Please ensure the target class has a constructor that matches these argument types: [org.apache.pulsar.broker.ServiceConfiguration] and executes cleanly.
	... 36 more
Caused by: java.lang.reflect.InvocationTargetException
	at org.mockito.internal.util.reflection.InstrumentationMemberAccessor.newInstance(InstrumentationMemberAccessor.java:198)
	at org.mockito.internal.util.reflection.InstrumentationMemberAccessor.newInstance(InstrumentationMemberAccessor.java:161)
	at org.mockito.internal.util.reflection.ModuleMemberAccessor.newInstance(ModuleMemberAccessor.java:42)
	at org.mockito.internal.creation.instance.ConstructorInstantiator.invokeConstructor(ConstructorInstantiator.java:70)
	at org.mockito.internal.creation.instance.ConstructorInstantiator.withParams(ConstructorInstantiator.java:53)
	at org.mockito.internal.creation.instance.ConstructorInstantiator.newInstance(ConstructorInstantiator.java:39)
	at org.mockito.internal.creation.bytebuddy.InlineDelegateByteBuddyMockMaker.doCreateMock(InlineDelegateByteBuddyMockMaker.java:360)
	at org.mockito.internal.creation.bytebuddy.InlineDelegateByteBuddyMockMaker.createMock(InlineDelegateByteBuddyMockMaker.java:330)
	at org.mockito.internal.creation.bytebuddy.InlineByteBuddyMockMaker.createMock(InlineByteBuddyMockMaker.java:58)
	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.persistent.PersistentSubscriptionTest.setup(PersistentSubscriptionTest.java:116)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	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.IllegalArgumentException: Required clusterName is null
	at org.apache.pulsar.common.configuration.PulsarConfigurationLoader.isComplete(PulsarConfigurationLoader.java:153)
	at org.apache.pulsar.broker.PulsarService.<init>(PulsarService.java:295)
	at org.apache.pulsar.broker.PulsarService.<init>(PulsarService.java:285)
	at org.apache.pulsar.broker.PulsarService.<init>(PulsarService.java:277)
	at java.base/java.lang.invoke.MethodHandle.invokeWithArguments(MethodHandle.java:710)
	at org.mockito.internal.util.reflection.InstrumentationMemberAccessor$Dispatcher$ByteBuddy$6w1xGzlj.invokeWithArguments(Unknown Source)
	at org.mockito.internal.util.reflection.InstrumentationMemberAccessor.lambda$newInstance$0(InstrumentationMemberAccessor.java:191)
	at org.mockito.internal.util.reflection.InstrumentationMemberAccessor.newInstance(InstrumentationMemberAccessor.java:188)
	... 51 more

nicoloboschi · 2022-02-10T22:46:56Z

@michaeljmarshall yes you're right. I've found a couple of other tests that can't simply pass for the same reason (Required clusterName is null). I can't explain to myself how they can be green on current master. Maybe enabling `mockito-inline´ Mockito itself is more stable.

…est - don't know how they passed before

nicoloboschi · 2022-02-11T00:20:05Z

/pulsarbot rerun-failure-checks

nicoloboschi · 2022-02-11T01:34:03Z

/pulsarbot rerun-failure-checks

nicoloboschi · 2022-02-11T12:22:27Z

/pulsarbot rerun-failure-checks

…ache#14098) * introduce NarClassLoaderBuilder since Mockito Inline cannot mock classloader classes * fix spyWithClassAndConstructorArgs(PulsarService.class, svcConfig) tests

github-actions bot assigned nicoloboschi Feb 2, 2022

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Feb 2, 2022

nicoloboschi added 4 commits February 7, 2022 17:35

first round

7801894

spotbugs

c5d85ce

checkstyle

8a728ba

fix CI

e5cd5c2

nicoloboschi force-pushed the test/replace-powermock-static branch from f64c3bd to e5cd5c2 Compare February 7, 2022 16:35

nicoloboschi added 3 commits February 7, 2022 22:12

fix other tests

7ecfb47

fix all the tests using Powermock#mockStatic

dd6d480

checkstyle

492939e

eolivelli requested changes Feb 8, 2022

View reviewed changes

pulsar-common/src/main/java/org/apache/pulsar/common/nar/NarClassLoader.java Outdated Show resolved Hide resolved

lhotari reviewed Feb 8, 2022

View reviewed changes

nicoloboschi added 2 commits February 8, 2022 23:45

NarClassLoaderBuilder

5e4e465

fix npe

3e2a13e

nicoloboschi added 2 commits February 8, 2022 23:51

spotbugs..

a512e77

spotbugs.. part2

b1a92ac

lhotari requested a review from eolivelli February 9, 2022 11:20

lhotari approved these changes Feb 9, 2022

View reviewed changes

lhotari requested review from hezhangjian, jerrypeng, codelipenghui, michaeljmarshall and merlimat February 9, 2022 11:21

eolivelli approved these changes Feb 9, 2022

View reviewed changes

michaeljmarshall approved these changes Feb 9, 2022

View reviewed changes

pulsar-common/src/main/java/org/apache/pulsar/common/protocol/Commands.java Outdated Show resolved Hide resolved

pulsar-common/src/main/java/org/apache/pulsar/common/protocol/Commands.java Outdated Show resolved Hide resolved

revert mortal changes and reduce code dup

b6b3cc9

FunctionRuntimeManagerTest

ef52cce

nicoloboschi added 3 commits February 10, 2022 16:53

fix api tests

d51323f

fix /PersistentTopicTest

bed4f15

Merge branch 'master' into test/replace-powermock-static

0e8e1da

fix spyWithClassAndConstructorArgs(PulsarService.class, svcConfig); t…

a923219

…est - don't know how they passed before

lhotari merged commit ca64c67 into apache:master Feb 11, 2022

BewareMyPower mentioned this pull request Aug 1, 2022

[branch-2.8][improve][broker] Recycle OpReadEntry in some corner cases (#16399) #16869

Merged

4 tasks

lhotari mentioned this pull request Feb 7, 2024

Replace the usage of PowerMock with Mockito in the Bookkeeper tests apache/bookkeeper#4201

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[test] Replace PowerMockito#mockStatic usages with mockito-inline #14098

[test] Replace PowerMockito#mockStatic usages with mockito-inline #14098

nicoloboschi commented Feb 2, 2022 •

edited

Loading

nicoloboschi commented Feb 3, 2022

nicoloboschi commented Feb 4, 2022

nicoloboschi commented Feb 5, 2022

nicoloboschi commented Feb 7, 2022

eolivelli left a comment

lhotari left a comment

nicoloboschi commented Feb 8, 2022

lhotari left a comment

eolivelli left a comment

nicoloboschi commented Feb 9, 2022

michaeljmarshall left a comment

nicoloboschi commented Feb 9, 2022

nicoloboschi commented Feb 9, 2022

nicoloboschi commented Feb 9, 2022

nicoloboschi commented Feb 9, 2022

nicoloboschi commented Feb 10, 2022

nicoloboschi commented Feb 10, 2022

nicoloboschi commented Feb 10, 2022

michaeljmarshall commented Feb 10, 2022

nicoloboschi commented Feb 10, 2022 •

edited

Loading

nicoloboschi commented Feb 11, 2022

nicoloboschi commented Feb 11, 2022

nicoloboschi commented Feb 11, 2022

[test] Replace PowerMockito#mockStatic usages with mockito-inline #14098

[test] Replace PowerMockito#mockStatic usages with mockito-inline #14098

Conversation

nicoloboschi commented Feb 2, 2022 • edited Loading

Motivation

Modifications

Documentation

nicoloboschi commented Feb 3, 2022

nicoloboschi commented Feb 4, 2022

nicoloboschi commented Feb 5, 2022

nicoloboschi commented Feb 7, 2022

eolivelli left a comment

Choose a reason for hiding this comment

lhotari left a comment

Choose a reason for hiding this comment

nicoloboschi commented Feb 8, 2022

lhotari left a comment

Choose a reason for hiding this comment

eolivelli left a comment

Choose a reason for hiding this comment

nicoloboschi commented Feb 9, 2022

michaeljmarshall left a comment

Choose a reason for hiding this comment

nicoloboschi commented Feb 9, 2022

nicoloboschi commented Feb 9, 2022

nicoloboschi commented Feb 9, 2022

nicoloboschi commented Feb 9, 2022

nicoloboschi commented Feb 10, 2022

nicoloboschi commented Feb 10, 2022

nicoloboschi commented Feb 10, 2022

michaeljmarshall commented Feb 10, 2022

nicoloboschi commented Feb 10, 2022 • edited Loading

nicoloboschi commented Feb 11, 2022

nicoloboschi commented Feb 11, 2022

nicoloboschi commented Feb 11, 2022

nicoloboschi commented Feb 2, 2022 •

edited

Loading

nicoloboschi commented Feb 10, 2022 •

edited

Loading