Stop using PowerMock & JUnit 4

pom.xml

@@ -22,8 +22,7 @@
         <!-- todo: 'jackson-annotations' would normally have the same version string as 'jackson'. This looks like a
          typo from the maintainers - hopefully we can remove this next update. -->
         <jackson-annotations.version>2.20</jackson-annotations.version>
-        <powermock.version>2.0.9</powermock.version>
-        <junit-4.version>4.13.2</junit-4.version>
+        <mockito.version>5.11.0</mockito.version>
         <junit-5.version>5.14.1</junit-5.version>
         <swagger-ui-version>4.13.2</swagger-ui-version>
         <prometheus.version>0.16.0</prometheus.version>
@@ -202,12 +201,6 @@
             <artifactId>swagger-annotations-jakarta</artifactId>
             <version>${swagger-core.version}</version>
         </dependency>
-        <dependency>
-            <groupId>junit</groupId>
-            <artifactId>junit</artifactId>
-            <version>${junit-4.version}</version>
-            <scope>test</scope>
-        </dependency>
         <dependency>
             <groupId>org.junit.jupiter</groupId>
             <artifactId>junit-jupiter-engine</artifactId>
@@ -255,23 +248,22 @@
             <version>${testcontainers.version}</version>
             <scope>test</scope>
         </dependency>
-
         <dependency>
-            <groupId>org.powermock</groupId>
-            <artifactId>powermock-module-junit4</artifactId>
-            <version>${powermock.version}</version>
+            <groupId>org.easymock</groupId>
+            <artifactId>easymock</artifactId>
+            <version>5.6.0</version>
             <scope>test</scope>
         </dependency>
         <dependency>
-            <groupId>org.powermock</groupId>
-            <artifactId>powermock-api-easymock</artifactId>
-            <version>${powermock.version}</version>
+            <groupId>org.mockito</groupId>
+            <artifactId>mockito-core</artifactId>
+            <version>${mockito.version}</version>
             <scope>test</scope>
         </dependency>
         <dependency>
-            <groupId>org.easymock</groupId>
-            <artifactId>easymock</artifactId>
-            <version>4.3</version>
+            <groupId>org.mockito</groupId>
+            <artifactId>mockito-junit-jupiter</artifactId>
+            <version>${mockito.version}</version>
             <scope>test</scope>
         </dependency>
 
@@ -568,16 +560,6 @@
                 <groupId>org.apache.maven.plugins</groupId>
                 <artifactId>maven-surefire-plugin</artifactId>
                 <version>3.2.5</version>
-                <dependencies>
-                    <dependency>
-                        <!-- This forces surefire (i.e. all unit tests) to use JUnit 4, as PowerMock is used in unit
-                        tests and is not compatible with JUnit 5.
-                        See https://maven.apache.org/surefire/maven-surefire-plugin/examples/junit.html#manually-specifying-a-provider -->
-                        <groupId>org.apache.maven.surefire</groupId>
-                        <artifactId>surefire-junit47</artifactId>
-                        <version>3.5.4</version>
-                    </dependency>
-                </dependencies>
                 <configuration>
                     <forkCount>3</forkCount>
                     <reuseForks>true</reuseForks>

src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/GroupManager.java

@@ -46,11 +46,11 @@
 import uk.ac.cam.cl.dtg.segue.dao.SegueDatabaseException;
 import uk.ac.cam.cl.dtg.segue.dao.content.ContentManagerException;
 import uk.ac.cam.cl.dtg.segue.dao.users.IUserGroupPersistenceManager;
+import uk.ac.cam.cl.dtg.util.NameOrderer;
 import uk.ac.cam.cl.dtg.util.mappers.UserMapper;
 
 import java.util.ArrayList;
 import java.util.Collection;
-import java.util.Comparator;
 import java.util.Date;
 import java.util.HashMap;
 import java.util.LinkedList;
@@ -181,7 +181,7 @@ public List<RegisteredUserDTO> getUsersInGroup(final UserGroupDTO group) throws
 
         List<RegisteredUserDTO> users = userManager.findUsers(groupMemberIds);
         // Sort the users by name
-        this.orderUsersByName(users);
+        NameOrderer.orderUsersByName(users);
         return users;
     }
 
@@ -210,24 +210,6 @@ public GroupMembershipStatus getGroupMembershipStatus(Long userId, Long groupId)
         return this.getUserMembershipMapForGroup(groupId).get(userId).getStatus();
     }
 
-    /**
-     * Helper method to consistently sort users by given name then family name in a case-insensitive order.
-     * @param users
-     *            - list of users.
-     */
-    private void orderUsersByName(final List<RegisteredUserDTO> users) {
-        // Remove apostrophes so that string containing them are ordered in the same way as in Excel.
-        // I.e. we want that "O'Aaa" < "Obbb" < "O'Ccc"
-        Comparator<String> excelStringOrder = Comparator.nullsLast((String a, String b) ->
-                String.CASE_INSENSITIVE_ORDER.compare(a.replaceAll("'", ""), b.replaceAll("'", "")));
-
-        // If names differ only by an apostrophe (i.e. "O'A" and "Oa"), break ties using name including any apostrophes:
-        users.sort(Comparator
-                .comparing(RegisteredUserDTO::getFamilyName, excelStringOrder)
-                .thenComparing(RegisteredUserDTO::getGivenName, excelStringOrder)
-                .thenComparing(RegisteredUserDTO::getFamilyName));
-    }
-
     /**
      * getAllGroupsOwnedAndManagedByUser.
      *

src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/UserAccountManager.java

@@ -780,7 +780,7 @@ public final boolean isRegisteredUserLoggedIn(final HttpServletRequest request)
      * @return Returns the current UserDTO if we can get it or null if user is not currently logged in
      * @throws NoUserLoggedInException - When the session has expired or there is no user currently logged in.
      */
-    public final RegisteredUserDTO getCurrentRegisteredUser(final HttpServletRequest request)
+    public RegisteredUserDTO getCurrentRegisteredUser(final HttpServletRequest request)
             throws NoUserLoggedInException {
         Objects.requireNonNull(request);
 
@@ -875,7 +875,7 @@ public List<RegisteredUserDTO> findUsers(final Collection<Long> userIds) throws
      * @throws SegueDatabaseException - If there is another database error
      */
     @Override
-    public final RegisteredUserDTO getUserDTOById(final Long id) throws NoUserException, SegueDatabaseException {
+    public RegisteredUserDTO getUserDTOById(final Long id) throws NoUserException, SegueDatabaseException {
         return this.getUserDTOById(id, false);
     }
 

src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/UserAuthenticationManager.java

@@ -1058,7 +1058,7 @@ private void createSession(final HttpServletRequest request, final HttpServletRe
      *            - the real user we are to validate this cookie against.
      * @return true if it is still valid, false if not.
      */
-    private boolean isValidUsersSession(final Map<String, String> sessionInformation, final RegisteredUser userFromDatabase) {
+    public boolean isValidUsersSession(final Map<String, String> sessionInformation, final RegisteredUser userFromDatabase) {
         Objects.requireNonNull(sessionInformation);
         Objects.requireNonNull(userFromDatabase);
 
@@ -1120,7 +1120,7 @@ private boolean isValidUsersSession(final Map<String, String> sessionInformation
      *            - Boolean data to encode in the cookie - true if a partial login
      * @return HMAC signature.
      */
-    private String calculateSessionHMAC(final String key, final String userId, final String currentDate, final String sessionToken,
+    public static String calculateSessionHMAC(final String key, final String userId, final String currentDate, final String sessionToken,
                                         @Nullable final Set<String> caveatFlags) {
         StringBuilder sb = new StringBuilder();
         sb.append(userId);

src/main/java/uk/ac/cam/cl/dtg/segue/dao/content/GitContentManager.java

@@ -197,7 +197,7 @@ public final ContentDTO getContentById(final String id) throws ContentManagerExc
      * @return the content DTO object.
      * @throws ContentManagerException on failure to return the object or null.
      */
-    public final ContentDTO getContentById(final String id, final boolean failQuietly) throws ContentManagerException {
+    public ContentDTO getContentById(final String id, final boolean failQuietly) throws ContentManagerException {
         return this.contentSubclassMapper.getDTOByDO(this.getContentDOById(id, failQuietly));
     }
 
@@ -242,7 +242,7 @@ public final Content getContentDOById(final String id) throws ContentManagerExce
      * @return the content DTO object.
      * @throws ContentManagerException on failure to return the object or null.
      */
-    public final Content getContentDOById(final String id, final boolean failQuietly) throws ContentManagerException {
+    public Content getContentDOById(final String id, final boolean failQuietly) throws ContentManagerException {
         if (null == id || id.isEmpty()) {
             return null;
         }
@@ -596,7 +596,7 @@ public final ResultsWrapper<ContentDTO> findByFieldNamesRandomOrder(
     }
 
     @Deprecated
-    public final ResultsWrapper<ContentDTO> findByFieldNamesRandomOrder(
+    public ResultsWrapper<ContentDTO> findByFieldNamesRandomOrder(
             final List<BooleanSearchClause> fieldsToMatch, final Integer startIndex,
             final Integer limit, @Nullable final Long randomSeed
     ) throws ContentManagerException {

src/main/java/uk/ac/cam/cl/dtg/segue/etl/ContentIndexer.java

@@ -562,7 +562,7 @@ private String fixMediaSrc(final String canonicalSourceFile, final String origin
      *            content object to flatten
      * @return Set of content objects comprised of all children and the parent.
      */
-    private Set<Content> flattenContentObjects(final Content content) {
+    public static Set<Content> flattenContentObjects(final Content content) {
         Set<Content> setOfContentObjects = new HashSet<>();
         if (!content.getChildren().isEmpty()) {
 
@@ -682,7 +682,7 @@ private synchronized void registerUnits(final IsaacNumericQuestion q, Map<String
      * @param gitCache
      *            a map that represents indexed content for a given sha.
      */
-    private synchronized void buildElasticSearchIndex(final String sha,
+    public synchronized void buildElasticSearchIndex(final String sha,
                                                       final Map<String, Content> gitCache,
                                                       final Set<String> tagsList,
                                                       final Map<String, String> allUnits,
@@ -931,7 +931,7 @@ private String collateExpandableChildren(Content content) {
      * @param sha       version to validate integrity of.
      * @param content   a single item of content
      */
-    private void recordContentTypeSpecificError(final String sha, final Content content, final Map<Content, List<String>> indexProblemCache) {
+    public void recordContentTypeSpecificError(final String sha, final Content content, final Map<Content, List<String>> indexProblemCache) {
         // ensure content does not have children and a value
         if (content.getValue() != null && !content.getChildren().isEmpty()) {
             String id = content.getId();

src/main/java/uk/ac/cam/cl/dtg/util/NameOrderer.java

@@ -0,0 +1,36 @@
+package uk.ac.cam.cl.dtg.util;
+
+import uk.ac.cam.cl.dtg.isaac.dto.users.RegisteredUserDTO;
+
+import java.util.Comparator;
+import java.util.List;
+
+/**
+ *  A utility class to order names lexicographically.
+ */
+public class NameOrderer {
+
+    /**
+     *  It does not make sense to create one of these!
+     */
+    private NameOrderer() {
+    }
+
+    /**
+     * Helper method to consistently sort users by given name then family name in a case-insensitive order.
+     * @param users
+     *            - list of users.
+     */
+    public static void orderUsersByName(final List<RegisteredUserDTO> users) {
+        // Remove apostrophes so that string containing them are ordered in the same way as in Excel.
+        // I.e. we want that "O'Aaa" < "Obbb" < "O'Ccc"
+        Comparator<String> excelStringOrder = Comparator.nullsLast((String a, String b) ->
+                String.CASE_INSENSITIVE_ORDER.compare(a.replaceAll("'", ""), b.replaceAll("'", "")));
+
+        // If names differ only by an apostrophe (i.e. "O'A" and "Oa"), break ties using name including any apostrophes:
+        users.sort(Comparator
+                .comparing(RegisteredUserDTO::getFamilyName, excelStringOrder)
+                .thenComparing(RegisteredUserDTO::getGivenName, excelStringOrder)
+                .thenComparing(RegisteredUserDTO::getFamilyName));
+    }
+}

src/main/java/uk/ac/cam/cl/dtg/util/YamlLoader.java

@@ -66,9 +66,23 @@ public Set<String> getKeys() {
         return this.loadedConfig.keySet();
     }
 
+    /**
+     * Factory method that can be overridden for testing
+     */
+    protected Yaml createYaml() {
+        return new Yaml();
+    }
+
+    /**
+     * Factory method that can be overridden for testing
+     */
+    protected FileInputStream createFileInputStream(String path) throws IOException {
+        return new FileInputStream(path);
+    }
+
     @Override
     protected synchronized void loadConfig() throws IOException {
-        Yaml yaml = new Yaml();
+        Yaml yaml = createYaml();
 
         String[] configPaths = this.configPath.split(",");
 
@@ -77,7 +91,7 @@ protected synchronized void loadConfig() throws IOException {
 
             // check to see if this a resource or a file somewhere else
             if (getClass().getClassLoader().getResourceAsStream(path) == null) {
-                try (FileInputStream ioStream = new FileInputStream(path)) {
+                try (FileInputStream ioStream = createFileInputStream(path)) {
                     // then we have to look further afield
                     subConfig = yaml.load(ioStream);
                 }

src/test/java/uk/ac/cam/cl/dtg/isaac/IsaacTest.java

@@ -17,7 +17,7 @@
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
-import org.junit.Before;
+import org.junit.jupiter.api.BeforeEach;
 import uk.ac.cam.cl.dtg.isaac.api.Constants;
 import uk.ac.cam.cl.dtg.isaac.api.managers.GameManager;
 import uk.ac.cam.cl.dtg.isaac.api.managers.QuizManager;
@@ -56,13 +56,13 @@
 
 import static org.easymock.EasyMock.anyLong;
 import static org.easymock.EasyMock.anyObject;
+import static org.easymock.EasyMock.createMock;
 import static org.easymock.EasyMock.expect;
 import static org.easymock.EasyMock.getCurrentArguments;
 import static org.easymock.EasyMock.partialMockBuilder;
-import static org.powermock.api.easymock.PowerMock.createMock;
-import static org.powermock.api.easymock.PowerMock.replay;
-import static org.powermock.api.easymock.PowerMock.reset;
-import static org.powermock.api.easymock.PowerMock.verify;
+import static org.easymock.EasyMock.replay;
+import static org.easymock.EasyMock.reset;
+import static org.easymock.EasyMock.verify;
 
 public class IsaacTest {
     protected static Date somePastDate = new Date(System.currentTimeMillis() - 7*24*60*60*1000);
@@ -130,7 +130,7 @@ public class IsaacTest {
 
     protected Map<Object, MockConfigurer> defaultsMap = new HashMap<>();
 
-    @Before
+    @BeforeEach
     public final void initializeIsaacTest() throws SegueDatabaseException, ContentManagerException {
         initializeIsaacObjects();
         initializeMocks();

src/test/java/uk/ac/cam/cl/dtg/isaac/api/AbstractFacadeTest.java

@@ -17,11 +17,7 @@
 
 import com.google.api.client.util.Maps;
 import com.google.common.base.Joiner;
-import org.junit.Before;
-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;
+import org.junit.jupiter.api.BeforeEach;
 import uk.ac.cam.cl.dtg.isaac.IsaacTest;
 import uk.ac.cam.cl.dtg.isaac.dto.SegueErrorResponse;
 import uk.ac.cam.cl.dtg.isaac.dto.users.RegisteredUserDTO;
@@ -47,14 +43,14 @@
 import java.util.stream.Collectors;
 
 import static org.easymock.EasyMock.anyObject;
+import static org.easymock.EasyMock.createMock;
 import static org.easymock.EasyMock.createNiceMock;
 import static org.easymock.EasyMock.expect;
 import static org.easymock.EasyMock.getCurrentArguments;
-import static org.junit.Assert.assertEquals;
-import static org.powermock.api.easymock.PowerMock.createMock;
-import static org.powermock.api.easymock.PowerMock.createPartialMock;
-import static org.powermock.api.easymock.PowerMock.replay;
-import static org.powermock.api.easymock.PowerMock.verifyAll;
+import static org.easymock.EasyMock.partialMockBuilder;
+import static org.easymock.EasyMock.replay;
+import static org.easymock.EasyMock.verify;
+import static org.junit.jupiter.api.Assertions.assertEquals;
 
 /**
  * A test base for testing Facades, specifically targeted around testing facades as different users.
@@ -83,9 +79,6 @@
  *
  * @deprecated in favour of IsaacIntegrationTest
  */
-@RunWith(PowerMockRunner.class)
-@PrepareForTest({UserAccountManager.class})
-@PowerMockIgnore({ "jakarta.ws.*", "jakarta.management.*", "jakarta.script.*" })
 @Deprecated
 abstract public class AbstractFacadeTest extends IsaacTest {
     protected Request request;
@@ -98,14 +91,17 @@ abstract public class AbstractFacadeTest extends IsaacTest {
     private Map<RegisteredUserDTO, UserSummaryDTO> userSummaries = Maps.newHashMap();
 
 
-    @Before
+    @BeforeEach
     public void abstractFacadeTestSetup() {
         httpServletRequest = createMock(HttpServletRequest.class);
         replay(httpServletRequest);
         request = createNiceMock(Request.class);  // We don't particularly care about what gets called on this.
         replay(request);
 
-        userManager = createPartialMock(UserAccountManager.class, "getCurrentUser", "getCurrentRegisteredUser", "convertToUserSummaryObject", "getUserDTOById");
+        userManager = partialMockBuilder(UserAccountManager.class)
+                .addMockedMethods("getCurrentUser", "getCurrentRegisteredUser", "convertToUserSummaryObject")
+                .addMockedMethod("getUserDTOById", Long.class) // This is overloaded so we have to specify the signature
+                .createMock();
 
         registerDefaultsFor(userManager, m -> {
             expect(m.convertToUserSummaryObject(anyObject(RegisteredUserDTO.class))).andStubAnswer(() -> {
@@ -271,7 +267,8 @@ protected RegisteredUserDTO currentUser() {
     ///////////////////////////////////////////////////////////////////////////
 
     private void assertErrorResponse(Status expectedStatus, Response actual) {
-        assertEquals("Expected status " + expectedStatus.name() + ", got " + actual.getStatusInfo().toEnum().name() + " with details " + extractErrorInfo(actual), expectedStatus.getStatusCode(), actual.getStatus());
+        assertEquals(expectedStatus.getStatusCode(), actual.getStatus(),
+                "Expected status " + expectedStatus.name() + ", got " + actual.getStatusInfo().toEnum().name() + " with details " + extractErrorInfo(actual));
     }
 
     private String extractErrorInfo(Response response) {
@@ -444,7 +441,7 @@ private void runStepsAs(@Nullable RegisteredUserDTO user, Endpoint endpoint) {
         private void verifyEndpoint(String when, Endpoint endpoint) {
             try {
                 runSteps(endpoint);
-                verifyAll();
+                verify(userManager, httpServletRequest, request);
             } catch (AssertionError e) {
                 String message = when + ", test failed, expected:\r\n";
                 message += steps.stream().map(s -> "  " + s.toString() + "\r\n").collect(Collectors.joining());