Skip to content

KAFKA-16878: Remove powermock and easymock from code base #16236

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

Merged
chia7712 merged 1 commit into from
Jun 8, 2024
Merged

KAFKA-16878: Remove powermock and easymock from code base #16236

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 2 additions & 16 deletions build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -522,13 +522,6 @@ subprojects {
// The suites are for running sets of tests in IDEs.
// Gradle will run each test class, so we exclude the suites to avoid redundantly running the tests twice.
def testsToExclude = ['**/*Suite.class']
// Exclude PowerMock tests when running with Java 16 or newer until a version of PowerMock that supports the relevant versions is released

@mimaison mimaison Jun 7, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There's another comment about PowerMock at line 822, can we remove it as well?

@mimaison mimaison Jun 7, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There are also comments about EasyMock in 4 places:

  • 2 in DescribeTopicPartitionsRequestHandlerTest
  • 2 in KafkaApisTest
    Let's clean those up as well.

@FrankYang0529 FrankYang0529 Jun 7, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Hi @mimaison, thanks for the review. I remove all remaining comments.

// The relevant issues are https://github.com/powermock/powermock/issues/1094 and https://github.com/powermock/powermock/issues/1099
if (JavaVersion.current().isCompatibleWith(JavaVersion.VERSION_16)) {
testsToExclude.addAll([
// connect tests
])
}

test {
maxParallelForks = maxTestForks
Expand Down Expand Up @@ -825,10 +818,7 @@ subprojects {
jacoco {
toolVersion = versions.jacoco
}

// NOTE: Jacoco Gradle plugin does not support "offline instrumentation" this means that classes mocked by PowerMock

@mimaison mimaison Jun 8, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You can't just remove the first line, the following line does not mean anything now.

@FrankYang0529 FrankYang0529 Jun 8, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sorry for that. I remove related comments.

// may report 0 coverage, since the source was modified after initial instrumentation.
// See https://github.com/jacoco/jacoco/issues/51

jacocoTestReport {
dependsOn tasks.test
sourceSets sourceSets.main
Expand Down Expand Up @@ -2376,7 +2366,6 @@ project(':streams') {
testImplementation libs.log4j
testImplementation libs.junitJupiter
testImplementation libs.junitVintageEngine
testImplementation libs.easymock
testImplementation libs.bcpkix
testImplementation libs.hamcrest
testImplementation libs.mockitoCore
Expand Down Expand Up @@ -3257,12 +3246,9 @@ project(':connect:runtime') {
testImplementation project(':storage')
testImplementation project(':connect:test-plugins')
testImplementation project(':group-coordinator')

testImplementation libs.easymock

testImplementation libs.junitJupiterApi
testImplementation libs.junitVintageEngine
testImplementation libs.powermockJunit4
testImplementation libs.powermockEasymock
testImplementation libs.mockitoCore
testImplementation libs.httpclient

Expand Down
5 changes: 0 additions & 5 deletions checkstyle/import-control.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,6 @@
<allow pkg="org.opentest4j" />
<allow pkg="org.hamcrest" />
<allow pkg="org.mockito" />
<allow pkg="org.easymock" />
<allow pkg="org.powermock" />
<allow pkg="java.security" />
<allow pkg="javax.net.ssl" />
<allow pkg="javax.security" />
Expand Down Expand Up @@ -622,9 +620,6 @@
<subpackage name="file">
<allow pkg="org.apache.kafka.connect" />
<allow pkg="org.apache.kafka.clients.consumer" />
<!-- for tests -->
<allow pkg="org.easymock" />
<allow pkg="org.powermock" />
</subpackage>

<subpackage name="tools">
Expand Down
2 changes: 0 additions & 2 deletions core/src/test/java/kafka/server/handlers/DescribeTopicPartitionsRequestHandlerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,6 @@ void testDescribeTopicPartitionsRequest() {
Action expectedActions2 = new Action(AclOperation.DESCRIBE, new ResourcePattern(ResourceType.TOPIC, authorizedTopic, PatternType.LITERAL), 1, true, true);
Action expectedActions3 = new Action(AclOperation.DESCRIBE, new ResourcePattern(ResourceType.TOPIC, authorizedNonExistTopic, PatternType.LITERAL), 1, true, true);

// Here we need to use AuthHelperTest.matchSameElements instead of EasyMock.eq since the order of the request is unknown
when(authorizer.authorize(any(RequestContext.class), argThat(t ->
t.contains(expectedActions1) || t.contains(expectedActions2) || t.contains(expectedActions3))))
.thenAnswer(invocation -> {
Expand Down Expand Up @@ -328,7 +327,6 @@ void testDescribeTopicPartitionsRequestWithEdgeCases() {
Action expectedActions1 = new Action(AclOperation.DESCRIBE, new ResourcePattern(ResourceType.TOPIC, authorizedTopic, PatternType.LITERAL), 1, true, true);
Action expectedActions2 = new Action(AclOperation.DESCRIBE, new ResourcePattern(ResourceType.TOPIC, authorizedTopic2, PatternType.LITERAL), 1, true, true);

// Here we need to use AuthHelperTest.matchSameElements instead of EasyMock.eq since the order of the request is unknown
when(authorizer.authorize(any(RequestContext.class), argThat(t ->
t.contains(expectedActions1) || t.contains(expectedActions2))))
.thenAnswer(invocation -> {
Expand Down
2 changes: 0 additions & 2 deletions core/src/test/scala/unit/kafka/server/KafkaApisTest.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2825,7 +2825,6 @@ class KafkaApisTest extends Logging {

@Test
def requiredAclsNotPresentWriteTxnMarkersThrowsAuthorizationException(): Unit = {
// Here we need to use AuthHelperTest.matchSameElements instead of EasyMock.eq since the order of the request is unknown
val topicPartition = new TopicPartition("t", 0)
val (_, request) = createWriteTxnMarkersRequest(asList(topicPartition))

Expand Down Expand Up @@ -4098,7 +4097,6 @@ class KafkaApisTest extends Logging {
new Action(AclOperation.DESCRIBE, new ResourcePattern(ResourceType.TOPIC, authorizedTopic, PatternType.LITERAL), 1, true, true)
)

// Here we need to use AuthHelperTest.matchSameElements instead of EasyMock.eq since the order of the request is unknown
when(authorizer.authorize(any[RequestContext], argThat((t: java.util.List[Action]) => t.containsAll(expectedActions.asJava))))
.thenAnswer { invocation =>
val actions = invocation.getArgument(1).asInstanceOf[util.List[Action]].asScala
Expand Down
12 changes: 0 additions & 12 deletions gradle/dependencies.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,14 +76,6 @@ if (JavaVersion.current().isCompatibleWith(JavaVersion.VERSION_11))
else
mockitoArtifactName = "mockito-inline"

// easymock 5.2 is required for Java 21 support, but it breaks tests using powermock
// powermock doesn't work with Java 16 or newer and hence it's safe to use the newer version in this case only
String easymockVersion
if (JavaVersion.current().isCompatibleWith(JavaVersion.VERSION_16))
easymockVersion = "5.2.0"
else
easymockVersion = "4.3"

// When adding, removing or updating dependencies, please also update the LICENSE-binary file accordingly.
// See https://issues.apache.org/jira/browse/KAFKA-12622 for steps to verify the LICENSE-binary file is correct.
versions += [
Expand Down Expand Up @@ -149,7 +141,6 @@ versions += [
netty: "4.1.110.Final",
opentelemetryProto: "1.0.0-alpha",
pcollections: "4.0.1",
powermock: "2.0.9",
reflections: "0.10.2",
reload4j: "1.2.25",
rocksDB: "7.9.2",
Expand Down Expand Up @@ -184,7 +175,6 @@ libs += [
caffeine: "com.github.ben-manes.caffeine:caffeine:$versions.caffeine",
commonsCli: "commons-cli:commons-cli:$versions.commonsCli",
commonsValidator: "commons-validator:commons-validator:$versions.commonsValidator",
easymock: "org.easymock:easymock:$easymockVersion",
jacksonAnnotations: "com.fasterxml.jackson.core:jackson-annotations:$versions.jackson",
jacksonDatabind: "com.fasterxml.jackson.core:jackson-databind:$versions.jackson",
jacksonDataformatCsv: "com.fasterxml.jackson.dataformat:jackson-dataformat-csv:$versions.jackson",
Expand Down Expand Up @@ -246,8 +236,6 @@ libs += [
nettyTransportNativeEpoll: "io.netty:netty-transport-native-epoll:$versions.netty",
pcollections: "org.pcollections:pcollections:$versions.pcollections",
opentelemetryProto: "io.opentelemetry.proto:opentelemetry-proto:$versions.opentelemetryProto",
powermockJunit4: "org.powermock:powermock-module-junit4:$versions.powermock",
powermockEasymock: "org.powermock:powermock-api-easymock:$versions.powermock",
reflections: "org.reflections:reflections:$versions.reflections",
rocksDBJni: "org.rocksdb:rocksdbjni:$versions.rocksDB",
scalaCollectionCompat: "org.scala-lang.modules:scala-collection-compat_$versions.baseScala:$versions.scalaCollectionCompat",
Expand Down