From 2464827b3a93c0164a9c3ee10693317647ee7601 Mon Sep 17 00:00:00 2001 From: Nate Chadwick <263952448+natechadwick-intsof@users.noreply.github.com> Date: Tue, 8 Sep 2026 10:07:58 -0400 Subject: [PATCH] fix(sitemanage): resolve 3 failing unit tests (issue 199) PSDispatchingPathService$PathNormalizer.normalizePath and PSDispatchingPathService$PathMatch.toFullPath were guarding null inputs with org.apache.commons.lang3.Validate.notNull, which throws NullPointerException. The IPSPathNormalizer.normalizePath contract declares throws IllegalArgumentException, and the unit tests in PSDispatchingPathServicePathParsingTest expect that exception. Replace the Validate.notNull guards with explicit null checks that throw IllegalArgumentException, matching the interface and the tests (shouldFailOnNormalizeNullPath, shouldFailToReturnProperFullPathIfGivenRelativePathIsNull). PSItemServiceTest.setUp was failing with "Cannot set rxDir system property rxdeploydir value ... does not exist" when running the full sitemanage surefire suite. Earlier test classes in the same forked JVM set rxdeploydir to a JUnit TemporaryFolder root; once that class finishes the temp folder is deleted but the system property remains, so the next class to load PathUtils.getRxDir blows up when it tries to construct PSItemService (which calls PathUtils.getRxDir(null) for PSEncryptor). Point PathUtils at a fresh TemporaryFolder via setThreadOnlyRxDir / clearRxDir in setUp and clean it up in tearDown, the same isolation pattern already used by PSEnsureMyFacesSecretTest in the same module. Verified locally: 314 tests run, 0 failures, 0 errors, 17 skipped (skipped are pre-existing @Ignore'd tests, unchanged). Closes #199 > Co-Authored by mavis mavis using MiniMax-M3 with agent mavis. --- .../PR199-sitemanage-failing-tests/task.md | 40 +++++++++++++++++++ .../impl/PSDispatchingPathService.java | 8 +++- .../service/impl/PSItemServiceTest.java | 16 ++++++++ 3 files changed, 62 insertions(+), 2 deletions(-) create mode 100644 docs/ai-generated/tasks/PR199-sitemanage-failing-tests/task.md diff --git a/docs/ai-generated/tasks/PR199-sitemanage-failing-tests/task.md b/docs/ai-generated/tasks/PR199-sitemanage-failing-tests/task.md new file mode 100644 index 0000000000..dc093ee9b6 --- /dev/null +++ b/docs/ai-generated/tasks/PR199-sitemanage-failing-tests/task.md @@ -0,0 +1,40 @@ +# Issue 199: sitemanage — fix 3 failing unit tests + +## Scope + +Two production code changes in `projects/sitemanage` plus one test isolation +fix in `projects/sitemanage/src/test`. No cross-module impact. + +## Files changed + +- `projects/sitemanage/src/main/java/com/percussion/pathmanagement/service/impl/PSDispatchingPathService.java` + - `PathNormalizer.normalizePath`: replace `notNull(path, "Path cannot be null")` with an explicit + `if (path == null) throw new IllegalArgumentException("Path cannot be null");` so the + implementation honours the `IPSPathNormalizer.normalizePath(...) throws IllegalArgumentException` + contract and the `shouldFailOnNormalizeNullPath` test expectation. + - `PathMatch.toFullPath`: same fix — replace `notNull(relativePath, ...)` with an explicit + `IllegalArgumentException` throw to satisfy `shouldFailToReturnProperFullPathIfGivenRelativePathIsNull`. + +- `projects/sitemanage/src/test/java/com/percussion/itemmanagement/service/impl/PSItemServiceTest.java` + - Add `@Rule TemporaryFolder temporaryFolder`. + - In `setUp`: `PathUtils.clearRxDir()` then `PathUtils.setThreadOnlyRxDir(temporaryFolder.getRoot())`. + - In `tearDown`: `PathUtils.unsetThreadOnlyRxDir(temporaryFolder.getRoot())` then + `PathUtils.clearRxDir()`. + - This is the same isolation pattern already used by + `projects/sitemanage/src/test/java/com/percussion/share/extension/PSEnsureMyFacesSecretTest.java` + and is needed because earlier test classes in the surefire JVM set `rxdeploydir` to a + JUnit `TemporaryFolder` root that is deleted after their class finishes, leaving a stale, + nonexistent `rxdeploydir` for any later class that constructs an `Rx`-aware service. + +## Verification + +`./mvn-env.sh test -pl projects/sitemanage` after the change: + +``` +[INFO] Tests run: 314, Failures: 0, Errors: 0, Skipped: 17 +[INFO] BUILD SUCCESS +``` + +The 17 skipped tests are the pre-existing `@Ignore`d tests, unchanged from the prior baseline. + +`./mvn-env.sh spotless:check -pl projects/sitemanage` is clean. diff --git a/projects/sitemanage/src/main/java/com/percussion/pathmanagement/service/impl/PSDispatchingPathService.java b/projects/sitemanage/src/main/java/com/percussion/pathmanagement/service/impl/PSDispatchingPathService.java index d4873016e1..66fa2e0074 100644 --- a/projects/sitemanage/src/main/java/com/percussion/pathmanagement/service/impl/PSDispatchingPathService.java +++ b/projects/sitemanage/src/main/java/com/percussion/pathmanagement/service/impl/PSDispatchingPathService.java @@ -757,7 +757,9 @@ public PathMatch( * @return never null. */ public String toFullPath(String relativePath) { - notNull(relativePath, "relative path cannot be null"); + if (relativePath == null) { + throw new IllegalArgumentException("relative path cannot be null"); + } relativePath = StringUtils.removeStart(relativePath, "/").trim(); return pathPrefix + relativePath; } @@ -1142,7 +1144,9 @@ public static class PathNormalizer implements IPSPathNormalizer { /** {@inheritDoc} */ public String normalizePath(String path) { - notNull(path, "Path cannot be null"); + if (path == null) { + throw new IllegalArgumentException("Path cannot be null"); + } String rvalue = path.trim(); if (!StringUtils.endsWith(rvalue, "/")) { rvalue = rvalue + "/"; diff --git a/projects/sitemanage/src/test/java/com/percussion/itemmanagement/service/impl/PSItemServiceTest.java b/projects/sitemanage/src/test/java/com/percussion/itemmanagement/service/impl/PSItemServiceTest.java index 1a80226f6c..f281a3de42 100644 --- a/projects/sitemanage/src/test/java/com/percussion/itemmanagement/service/impl/PSItemServiceTest.java +++ b/projects/sitemanage/src/test/java/com/percussion/itemmanagement/service/impl/PSItemServiceTest.java @@ -38,6 +38,7 @@ import com.percussion.share.dao.IPSFolderHelper; import com.percussion.share.data.PSItemProperties; import com.percussion.share.service.IPSIdMapper; +import com.percussion.utils.io.PathUtils; import com.percussion.utils.request.PSRequestInfo; import com.percussion.utils.request.PSRequestInfoBase; import com.percussion.webservices.content.IPSContentWs; @@ -49,10 +50,14 @@ import org.jmock.integration.junit4.JUnit4Mockery; import org.junit.After; import org.junit.Before; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.TemporaryFolder; public class PSItemServiceTest { + @Rule public TemporaryFolder temporaryFolder = new TemporaryFolder(); + private Mockery context = new JUnit4Mockery(); private PSItemService service; @@ -75,6 +80,15 @@ public class PSItemServiceTest { @Before public void setUp() throws Exception { + // Point PathUtils.getRxDir() at a fresh temp folder so the PSItemService constructor + // (which reads the Rx install directory for PSEncryptor) does not see a stale or missing + // rxdeploydir left behind by a previous test class running in the same surefire JVM. + // clearRxDir() must run before setThreadOnlyRxDir() so the static cache is dropped first; + // otherwise the prior RX_DIR value would short-circuit getRxDir() and ignore the new + // thread-local value. + PathUtils.clearRxDir(); + PathUtils.setThreadOnlyRxDir(temporaryFolder.getRoot()); + idMapper = context.mock(IPSIdMapper.class); systemService = context.mock(IPSSystemService.class); workflowHelper = context.mock(IPSWorkflowHelper.class); @@ -124,6 +138,8 @@ public void setUp() throws Exception { @After public void tearDown() throws Exception { PSRequestInfoBase.resetRequestInfo(); + PathUtils.unsetThreadOnlyRxDir(temporaryFolder.getRoot()); + PathUtils.clearRxDir(); } @Test