Remove PowerMock and replace with EasyMock Remove PowerMock 2.0.9 (powermock-api-easymock, powermock-module-junit4 and their five transitive siblings) and the javassist version pin that only existed to override PowerMock's older transitive. PowerMock 2.0.9 is the last release from 2020 and its classloader rewriting is incompatible with EasyMock 5's MethodHandles.Lookup-based ByteBuddy class injection on Java 17+: the generated proxy lands in org.easymock.mocks.* but the Lookup is anchored at org.easymock.internal.ClassProxyFactory and the JVM refuses the cross-package define. PowerMock's only feature this codebase used was mockStatic on JgitWrapper. Convert JgitWrapper from static utility to a stateless instance class so its method becomes invokevirtual on a mocked instance, which plain EasyMock instance mocking handles. PathOwners gains a package-private 11-arg constructor that accepts the wrapper for tests; the public 10-arg overloads default to a real new JgitWrapper(), so all production callers in plugins/owners are unchanged. Keeping a single mocking framework on the test classpath was preferred over adopting Mockito.mockStatic on a still-static JgitWrapper. Bump org.easymock:easymock to 5.6.0 for a byte-buddy that knows recent JDK class-file versions. Co-Authored-By: Daniele Sassoli <danielesassoli@gmail.com> Change-Id: I5079628f6f3cde4786800e4810939e40b8a4ac48
diff --git a/BUILD b/BUILD index 97a3ee9..81777d7 100644 --- a/BUILD +++ b/BUILD
@@ -36,12 +36,5 @@ deps = PLUGIN_DEPS + PLUGIN_TEST_DEPS + [ ":owners-common-api__plugin", "@easymock//jar", - "@javassist//jar", - "@powermock-api-easymock//jar", - "@powermock-api-support//jar", - "@powermock-core//jar", - "@powermock-module-junit4-common//jar", - "@powermock-module-junit4//jar", - "@powermock-reflect//jar", ], )
diff --git a/external_plugin_deps.bzl b/external_plugin_deps.bzl index 5ad1930..830c0ad 100644 --- a/external_plugin_deps.bzl +++ b/external_plugin_deps.bzl
@@ -35,8 +35,8 @@ maven_jar( name = "easymock", - artifact = "org.easymock:easymock:3.1", - sha1 = "3e127311a86fc2e8f550ef8ee4abe094bbcf7e7e", + artifact = "org.easymock:easymock:5.6.0", + sha1 = "f8e15a47aac9838ee36be6c3eddc50bb78a06191", deps = [ "@cglib//jar", "@objenesis//jar", @@ -54,46 +54,3 @@ artifact = "org.objenesis:objenesis:2.6", sha1 = "639033469776fd37c08358c6b92a4761feb2af4b", ) - - POWERM_VERS = "1.6.1" - maven_jar( - name = "powermock-module-junit4", - artifact = "org.powermock:powermock-module-junit4:" + POWERM_VERS, - sha1 = "ea8530b2848542624f110a393513af397b37b9cf", - ) - - maven_jar( - name = "powermock-module-junit4-common", - artifact = "org.powermock:powermock-module-junit4-common:" + POWERM_VERS, - sha1 = "7222ced54dabc310895d02e45c5428ca05193cda", - ) - - maven_jar( - name = "powermock-reflect", - artifact = "org.powermock:powermock-reflect:" + POWERM_VERS, - sha1 = "97d25eda8275c11161bcddda6ef8beabd534c878", - ) - - maven_jar( - name = "powermock-api-easymock", - artifact = "org.powermock:powermock-api-easymock:" + POWERM_VERS, - sha1 = "aa740ecf89a2f64d410b3d93ef8cd6833009ef00", - ) - - maven_jar( - name = "powermock-api-support", - artifact = "org.powermock:powermock-api-support:" + POWERM_VERS, - sha1 = "592ee6d929c324109d3469501222e0c76ccf0869", - ) - - maven_jar( - name = "powermock-core", - artifact = "org.powermock:powermock-core:" + POWERM_VERS, - sha1 = "5afc1efce8d44ed76b30af939657bd598e45d962", - ) - - maven_jar( - name = "javassist", - artifact = "org.javassist:javassist:3.22.0-GA", - sha1 = "3e83394258ae2089be7219b971ec21a8288528ad", - )
diff --git a/src/main/java/com/googlesource/gerrit/owners/common/JgitWrapper.java b/src/main/java/com/googlesource/gerrit/owners/common/JgitWrapper.java index 17b8af9..8f3cd42 100644 --- a/src/main/java/com/googlesource/gerrit/owners/common/JgitWrapper.java +++ b/src/main/java/com/googlesource/gerrit/owners/common/JgitWrapper.java
@@ -34,7 +34,7 @@ public class JgitWrapper { private static final Logger log = LoggerFactory.getLogger(JgitWrapper.class); - public static Optional<byte[]> getBlobAsBytes(Repository repository, String revision, String path) + public Optional<byte[]> getBlobAsBytes(Repository repository, String revision, String path) throws IOException { Ref ref = repository.getRefDatabase().exactRef(RefNames.fullName(revision)); if (ref == null) { @@ -52,14 +52,14 @@ } } - private static RevCommit parseCommit(Repository repository, ObjectId commit) throws IOException { + private RevCommit parseCommit(Repository repository, ObjectId commit) throws IOException { try (final RevWalk walk = new RevWalk(repository)) { walk.setRetainBody(true); return walk.parseCommit(commit); } } - private static Optional<byte[]> readBlob(Repository repository, ObjectId id) { + private Optional<byte[]> readBlob(Repository repository, ObjectId id) { try { return Optional.of(repository.open(id, OBJ_BLOB).getCachedBytes(Integer.MAX_VALUE)); } catch (Exception e) {
diff --git a/src/main/java/com/googlesource/gerrit/owners/common/PathOwners.java b/src/main/java/com/googlesource/gerrit/owners/common/PathOwners.java index 71ce117..7b81a1a 100644 --- a/src/main/java/com/googlesource/gerrit/owners/common/PathOwners.java +++ b/src/main/java/com/googlesource/gerrit/owners/common/PathOwners.java
@@ -18,7 +18,6 @@ import static com.google.gerrit.entities.Patch.COMMIT_MSG; import static com.google.gerrit.entities.Patch.MERGE_LIST; -import static com.googlesource.gerrit.owners.common.JgitWrapper.getBlobAsBytes; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; @@ -97,6 +96,8 @@ private final Optional<LabelDefinition> label; + private final JgitWrapper jgitWrapper; + public PathOwners( Accounts accounts, GitRepositoryManager repositoryManager, @@ -159,6 +160,36 @@ PathOwnersEntriesCache cache, Optional<LabelDefinition> globalLabel) throws InvalidOwnersFileException { + this( + accounts, + repositoryManager, + repository, + parentProjectsNames, + branchWhenEnabled, + modifiedPaths, + expandGroups, + project, + cache, + globalLabel, + new JgitWrapper()); + } + + // Package-private constructor used by tests to inject a mocked JgitWrapper. + // Production callers go through the 10-arg overloads above, which default to + // a real `new JgitWrapper()`. + PathOwners( + Accounts accounts, + GitRepositoryManager repositoryManager, + Repository repository, + List<Project.NameKey> parentProjectsNames, + Optional<String> branchWhenEnabled, + Set<String> modifiedPaths, + boolean expandGroups, + String project, + PathOwnersEntriesCache cache, + Optional<LabelDefinition> globalLabel, + JgitWrapper jgitWrapper) + throws InvalidOwnersFileException { this.repositoryManager = repositoryManager; this.repository = repository; this.parentProjectsNames = parentProjectsNames; @@ -166,6 +197,7 @@ this.parser = new ConfigurationParser(accounts); this.accounts = accounts; this.expandGroups = expandGroups; + this.jgitWrapper = jgitWrapper; OwnersMap map; if (branchWhenEnabled.isPresent()) { @@ -539,7 +571,7 @@ String project, Repository repo, String ownersPath, String branch) throws InvalidOwnersFileException { try { - Optional<byte[]> configBytes = getBlobAsBytes(repo, branch, ownersPath); + Optional<byte[]> configBytes = jgitWrapper.getBlobAsBytes(repo, branch, ownersPath); if (configBytes.isEmpty()) { return Optional.empty(); }
diff --git a/src/test/java/com/googlesource/gerrit/owners/common/Config.java b/src/test/java/com/googlesource/gerrit/owners/common/Config.java index e531a3f..155a8ec 100644 --- a/src/test/java/com/googlesource/gerrit/owners/common/Config.java +++ b/src/test/java/com/googlesource/gerrit/owners/common/Config.java
@@ -24,60 +24,60 @@ import com.google.gerrit.server.patch.PatchListEntry; import java.io.IOException; import java.util.Optional; +import org.easymock.EasyMockSupport; import org.eclipse.jgit.lib.Repository; import org.junit.Ignore; -import org.powermock.api.easymock.PowerMock; @Ignore -public abstract class Config { +public abstract class Config extends EasyMockSupport { protected GitRepositoryManager repositoryManager; protected Repository repository; protected Repository parentRepository1; protected Repository parentRepository2; protected ConfigurationParser parser; + protected JgitWrapper jgitWrapper; protected TestAccounts accounts = new TestAccounts(); protected Optional<String> branch = Optional.of("master"); public void setup() throws Exception { - PowerMock.mockStatic(JgitWrapper.class); - - repositoryManager = PowerMock.createMock(GitRepositoryManager.class); - repository = PowerMock.createMock(Repository.class); - parentRepository1 = PowerMock.createMock(Repository.class); - parentRepository2 = PowerMock.createMock(Repository.class); + jgitWrapper = createMock(JgitWrapper.class); + repositoryManager = createMock(GitRepositoryManager.class); + repository = createMock(Repository.class); + parentRepository1 = createMock(Repository.class); + parentRepository2 = createMock(Repository.class); parser = new ConfigurationParser(accounts); } void expectConfig(String path, String config) throws IOException { expect( - JgitWrapper.getBlobAsBytes( + jgitWrapper.getBlobAsBytes( anyObject(Repository.class), anyObject(String.class), eq(path))) .andReturn(Optional.of(config.getBytes())) .anyTimes(); } void expectConfig(String path, String branch, String config) throws IOException { - expect(JgitWrapper.getBlobAsBytes(anyObject(Repository.class), eq(branch), eq(path))) + expect(jgitWrapper.getBlobAsBytes(anyObject(Repository.class), eq(branch), eq(path))) .andReturn(Optional.of(config.getBytes())) .anyTimes(); } void expectConfig(String path, String branch, Repository repo, String config) throws IOException { - expect(JgitWrapper.getBlobAsBytes(eq(repo), eq(branch), eq(path))) + expect(jgitWrapper.getBlobAsBytes(eq(repo), eq(branch), eq(path))) .andReturn(Optional.of(config.getBytes())) .anyTimes(); } void expectNoConfig(String path) throws IOException { expect( - JgitWrapper.getBlobAsBytes( + jgitWrapper.getBlobAsBytes( anyObject(Repository.class), anyObject(String.class), eq(path))) .andReturn(Optional.empty()) .anyTimes(); } PatchListEntry expectEntry(String name) { - PatchListEntry entry = PowerMock.createMock(PatchListEntry.class); + PatchListEntry entry = createMock(PatchListEntry.class); expect(entry.getNewName()).andReturn(name).anyTimes(); expect(entry.getChangeType()).andReturn(Patch.ChangeType.MODIFIED).anyTimes(); expect(entry.getDeletions()).andReturn(1);
diff --git a/src/test/java/com/googlesource/gerrit/owners/common/PathOwnersTest.java b/src/test/java/com/googlesource/gerrit/owners/common/PathOwnersTest.java index 134eeda..8160373 100644 --- a/src/test/java/com/googlesource/gerrit/owners/common/PathOwnersTest.java +++ b/src/test/java/com/googlesource/gerrit/owners/common/PathOwnersTest.java
@@ -23,7 +23,6 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; -import static org.powermock.api.easymock.PowerMock.replayAll; import com.google.common.truth.Truth8; import com.google.gerrit.entities.Account; @@ -37,14 +36,7 @@ import org.eclipse.jgit.lib.Repository; import org.junit.Before; import org.junit.Test; -import org.junit.runner.RunWith; -import org.powermock.core.classloader.annotations.PowerMockIgnore; -import org.powermock.core.classloader.annotations.PrepareForTest; -import org.powermock.modules.junit4.PowerMockRunner; -@RunWith(PowerMockRunner.class) -@PowerMockIgnore("jdk.internal.reflect.*") -@PrepareForTest(JgitWrapper.class) public class PathOwnersTest extends ClassicConfig { private static final String CLASSIC_OWNERS = "classic/OWNERS"; @@ -85,7 +77,8 @@ EXPAND_GROUPS, "foo", CACHE_MOCK, - Optional.empty()); + Optional.empty(), + jgitWrapper); Set<Account.Id> ownersSet = owners.get().get(CLASSIC_OWNERS); assertEquals(2, ownersSet.size()); assertTrue(ownersSet.contains(USER_A_ID)); @@ -109,7 +102,8 @@ EXPAND_GROUPS, "foo", CACHE_MOCK, - Optional.of(EXPECTED_LABEL_DEFINITION)); + Optional.of(EXPECTED_LABEL_DEFINITION), + jgitWrapper); Truth8.assertThat(owners.getLabel()).hasValue(EXPECTED_LABEL_DEFINITION); } @@ -128,7 +122,8 @@ DO_NOT_EXPAND_GROUPS, "foo", CACHE_MOCK, - Optional.empty()); + Optional.empty(), + jgitWrapper); Set<String> ownersSet = owners.getFileGroupOwners().get(CLASSIC_FILE_TXT); assertEquals(2, ownersSet.size()); assertTrue(ownersSet.contains(USER_A)); @@ -151,7 +146,8 @@ EXPAND_GROUPS, "foo", CACHE_MOCK, - Optional.empty()); + Optional.empty(), + jgitWrapper); Set<Account.Id> ownersSet = owners.get().get(CLASSIC_OWNERS); assertEquals(0, ownersSet.size()); } @@ -177,7 +173,8 @@ EXPAND_GROUPS, "foo", CACHE_MOCK, - Optional.empty()); + Optional.empty(), + jgitWrapper); Set<Account.Id> ownersSet2 = owners2.get().get(CLASSIC_OWNERS); // in this case we are inheriting the acct3 from /OWNERS @@ -209,7 +206,8 @@ EXPAND_GROUPS, "foo", CACHE_MOCK, - Optional.of(EXPECTED_LABEL_DEFINITION)); + Optional.of(EXPECTED_LABEL_DEFINITION), + jgitWrapper); Truth8.assertThat(owners2.getLabel()).hasValue(EXPECTED_LABEL_DEFINITION); } @@ -231,7 +229,8 @@ EXPAND_GROUPS, "foo", CACHE_MOCK, - Optional.of(EXPECTED_LABEL_DEFINITION)); + Optional.of(EXPECTED_LABEL_DEFINITION), + jgitWrapper); Truth8.assertThat(owners2.getLabel()).hasValue(EXPECTED_LABEL_DEFINITION); } @@ -261,7 +260,8 @@ EXPAND_GROUPS, "foo", CACHE_MOCK, - Optional.empty()); + Optional.empty(), + jgitWrapper); Map<String, Set<Account.Id>> fileOwners = owners.getFileOwners(); assertEquals(1, fileOwners.size()); @@ -307,7 +307,8 @@ EXPAND_GROUPS, "foo", CACHE_MOCK, - Optional.empty()); + Optional.empty(), + jgitWrapper); Map<String, Set<Account.Id>> fileOwners = owners.getFileOwners(); assertEquals(fileOwners.size(), 1); @@ -357,7 +358,8 @@ EXPAND_GROUPS, "foo", CACHE_MOCK, - Optional.empty()); + Optional.empty(), + jgitWrapper); Map<String, Set<Account.Id>> fileOwners = owners.getFileOwners(); assertEquals(fileOwners.size(), 2); @@ -402,7 +404,8 @@ EXPAND_GROUPS, "foo", CACHE_MOCK, - Optional.empty()); + Optional.empty(), + jgitWrapper); Set<Account.Id> ownersSet = owners.get().get("dir/subdir/OWNERS"); assertEquals(3, ownersSet.size()); @@ -483,7 +486,8 @@ EXPAND_GROUPS, "foo", cacheMock, - Optional.empty()); + Optional.empty(), + jgitWrapper); assertThat(owners.getFileOwners()).isNotEmpty(); int expectedCacheCalls = @@ -510,7 +514,8 @@ EXPAND_GROUPS, "foo", CACHE_MOCK, - Optional.empty()); + Optional.empty(), + jgitWrapper); assertThat(owners.getFileOwnersAllowedAutoApproval()).isEmpty(); assertThat(owners.getFileOwners()).isEmpty(); @@ -536,7 +541,8 @@ EXPAND_GROUPS, "foo", CACHE_MOCK, - Optional.empty()); + Optional.empty(), + jgitWrapper); assertThat(owners.getFileOwnersAllowedAutoApproval()).contains("dir/file.txt"); } @@ -561,7 +567,8 @@ EXPAND_GROUPS, "foo", CACHE_MOCK, - Optional.empty()); + Optional.empty(), + jgitWrapper); assertThat(owners.getFileOwnersAllowedAutoApproval()).isEmpty(); } @@ -590,7 +597,8 @@ EXPAND_GROUPS, "foo", CACHE_MOCK, - Optional.empty()); + Optional.empty(), + jgitWrapper); assertThat(owners.getFileOwnersAllowedAutoApproval()).contains("file.txt"); } @@ -620,7 +628,8 @@ EXPAND_GROUPS, "foo", CACHE_MOCK, - Optional.empty()); + Optional.empty(), + jgitWrapper); assertThat(owners.getFileOwnersAllowedAutoApproval()).isEmpty(); }
diff --git a/src/test/java/com/googlesource/gerrit/owners/common/RegexTest.java b/src/test/java/com/googlesource/gerrit/owners/common/RegexTest.java index a6e10a6..a28b7fc 100644 --- a/src/test/java/com/googlesource/gerrit/owners/common/RegexTest.java +++ b/src/test/java/com/googlesource/gerrit/owners/common/RegexTest.java
@@ -24,7 +24,6 @@ import static java.util.Collections.emptyList; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; -import static org.powermock.api.easymock.PowerMock.replayAll; import com.google.gerrit.entities.Account; import com.google.gerrit.extensions.client.InheritableBoolean; @@ -36,14 +35,7 @@ import java.util.stream.Collectors; import org.junit.Before; import org.junit.Test; -import org.junit.runner.RunWith; -import org.powermock.core.classloader.annotations.PowerMockIgnore; -import org.powermock.core.classloader.annotations.PrepareForTest; -import org.powermock.modules.junit4.PowerMockRunner; -@RunWith(PowerMockRunner.class) -@PowerMockIgnore("jdk.internal.reflect.*") -@PrepareForTest(JgitWrapper.class) public class RegexTest extends Config { private static final String ACCOUNT_A = "a"; @@ -158,7 +150,8 @@ EXPAND_GROUPS, "foo", new PathOwnersEntriesCacheMock(), - Optional.empty()); + Optional.empty(), + jgitWrapper); // assertions on classic owners Set<Account.Id> ownersSet = owners.get().get("project/OWNERS"); @@ -266,7 +259,8 @@ EXPAND_GROUPS, "foo", new PathOwnersEntriesCacheMock(), - Optional.empty()); + Optional.empty(), + jgitWrapper); Set<String> ownedFiles = owners.getFileOwners().keySet(); assertThat(ownedFiles).containsExactly("project/file.sql");