Skip to content
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.

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

Chore: Add unit tests for architectural and AOP analysis #36

Merged
merged 24 commits into from
Dec 6, 2024
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
417bdab
Add tests
sarpsahinalp Oct 21, 2024
06c2869
Add archunit tests
sarpsahinalp Oct 21, 2024
bf59d91
Add archunit tests
sarpsahinalp Oct 21, 2024
f8ad126
tests for aop added
az108 Oct 23, 2024
a8caee0
Merge remote-tracking branch 'origin/chore/add-tests' into chore/add-…
az108 Oct 23, 2024
d95e6a0
fix 1 test
az108 Oct 23, 2024
4a6055b
Update tests
sarpsahinalp Oct 31, 2024
a2cc647
Update FileHandlerConstants.java
sarpsahinalp Oct 31, 2024
ccbeb53
Update src/test/java/de/tum/cit/ase/ares/integration/testuser/subject…
MarkusPaulsen Nov 3, 2024
1ed9070
Fixed error in one of the test cases.
Nov 3, 2024
bbd632c
Merge branch 'main' into chore/add-tests
Nov 4, 2024
9e19949
Merge branch 'main' into chore/add-tests
Dec 5, 2024
9bdd5c0
Fixed merge errors.
Dec 5, 2024
17ef7ce
Fixed test errors.
Dec 5, 2024
34c1b27
Update src/test/java/de/tum/cit/ase/ares/aop/JavaAOPModeTest.java
MarkusPaulsen Dec 5, 2024
fc41619
Adjusted JavaAOPModeTest.java
Dec 5, 2024
747bc16
Update src/test/java/de/tum/cit/ase/ares/aop/instrumentation/advice/J…
MarkusPaulsen Dec 5, 2024
b256b13
Update src/test/java/de/tum/cit/ase/ares/aop/instrumentation/advice/J…
MarkusPaulsen Dec 5, 2024
98b667c
Update src/test/java/de/tum/cit/ase/ares/aop/instrumentation/advice/J…
MarkusPaulsen Dec 5, 2024
5eae8b9
Adjusted api/aop
Dec 5, 2024
acb7e64
Add documentation
Dec 5, 2024
11cc0e9
Add documentation
Dec 5, 2024
0086010
Update src/test/java/de/tum/cit/ase/ares/api/architecture/JavaArchUni…
MarkusPaulsen Dec 5, 2024
8e21062
Improved Advice Tests
Dec 5, 2024
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
6 changes: 6 additions & 0 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,12 @@
<artifactId>junit-platform-launcher</artifactId>
<version>${junit-platform-version}</version>
</dependency>
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-inline</artifactId>
<version>5.2.0</version>
<scope>test</scope>
</dependency>
az108 marked this conversation as resolved.
Show resolved Hide resolved
<!-- Logging framework -->
<dependency>
<groupId>ch.qos.logback</groupId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ public class FileHandlerConstants {
public static final Path ARCHUNIT_COMMAND_EXECUTION_METHODS = FileTools.resolveOnResources("templates", "architecture" , "java", "archunit", "methods", "command-execution-methods.txt");
public static final Path ARCHUNIT_THREAD_MANIPULATION_METHODS = FileTools.resolveOnResources("templates", "architecture" , "java", "archunit", "methods", "thread-manipulation-methods.txt");
public static final Path ARCHUNIT_SERIALIZATION_METHODS = FileTools.resolveOnResources("templates", "architecture" , "java", "archunit", "methods", "serializable-methods.txt");
public static final Path ARCHUNIT_CLASSLOADER_METHODS = FileTools.resolveOnResources("templates", "architecture" , "java", "archunit", "methods", "classloader-methods.txt");
//</editor-fold>

//<editor-fold desc="WALA Methods">
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package de.tum.cit.ase.ares.api.architecture.java.archunit;

//<editor-fold desc="Imports">

import com.tngtech.archunit.core.domain.JavaClasses;
import de.tum.cit.ase.ares.api.architecture.java.JavaArchitecturalTestCaseSupported;
import de.tum.cit.ase.ares.api.policy.SecurityPolicy.PackagePermission;
Expand Down Expand Up @@ -51,7 +50,6 @@ private JavaArchUnitSecurityTestCase(@Nonnull Builder builder) {
this.javaArchitectureTestCaseSupported = builder.javaArchitectureTestCaseSupported;
this.allowedPackages = builder.allowedPackages;
}

//</editor-fold>

//<editor-fold desc="Tool methods">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -158,4 +158,11 @@ public boolean test(JavaClass javaClass) {
FileHandlerConstants.ARCHUNIT_SERIALIZATION_METHODS
);
//</editor-fold>

//<editor-fold desc="ClassLoader related rule">
public static final ArchRule NO_CLASSES_SHOULD_USE_CLASSLOADERS = createNoClassShouldHaveMethodRule(
"uses ClassLoaders",
FileHandlerConstants.ARCHUNIT_CLASSLOADER_METHODS
);
//</editor-fold>
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
java.lang.ClassLoader
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ java.lang.Class.getDeclaringClass()
java.lang.ClassLoader.getParent()
java.lang.ClassLoader.getSystemClassLoader()
java.lang.ClassLoader.getPlatformClassLoader()
java.lang.ClassLoader
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Remove duplicate ClassLoader entry.

The java.lang.ClassLoader entry appears to be redundant since specific ClassLoader methods are already listed above (e.g., getParent(), getSystemClassLoader(), getPlatformClassLoader()).

-java.lang.ClassLoader
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
java.lang.ClassLoader

java.lang.Module.addReads(java.lang.Module)
java.lang.Module.getResourceAsStream(java.lang.String)
java.lang.Module.addExports(java.lang.String, java.lang.Module)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@ java.util.concurrent.DelayQueue.take()
java.util.concurrent.ExecutorService.close()
java.util.concurrent.ForkJoinPool.managedBlock(java.util.concurrent.ForkJoinPool$ManagedBlocker)
java.util.concurrent.ForkJoinPool.close()
java.util.concurrent.ForkJoinPool
java.util.concurrent.ForkJoinTask.inForkJoinPool()
java.util.concurrent.ForkJoinTask.fork()
java.util.concurrent.ForkJoinTask.getPool()
Expand Down
75 changes: 75 additions & 0 deletions src/test/java/de/tum/cit/ase/ares/aop/JavaAOPModeTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
package de.tum.cit.ase.ares.aop;

import de.tum.cit.ase.ares.api.aop.java.JavaAOPMode;
import de.tum.cit.ase.ares.api.util.FileTools;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.mockito.MockedStatic;

import java.lang.reflect.Method;
import java.nio.file.Path;
import java.util.List;

import static org.junit.jupiter.api.Assertions.*;
import static org.mockito.Mockito.*;

class JavaAOPModeTest {

private JavaAOPMode instrumentationMode;
private JavaAOPMode aspectjMode;

@BeforeEach
void setUp() {
instrumentationMode = JavaAOPMode.INSTRUMENTATION;
aspectjMode = JavaAOPMode.ASPECTJ;
}

@Test
void testEnumValues() {
assertNotNull(instrumentationMode);
assertNotNull(aspectjMode);
}
MarkusPaulsen marked this conversation as resolved.
Show resolved Hide resolved

@Test
void testFilesToCopy_InstrumentationMode() {
try (MockedStatic<FileTools> mockedFileTools = mockStatic(FileTools.class)) {
mockedFileTools.when(() -> FileTools.resolveOnResources(any(String[].class)))
.thenReturn(mock(Path.class));
instrumentationMode.filesToCopy();
mockedFileTools.verify(() -> FileTools.resolveOnResources(any(String[].class)), times(13));
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Extract magic numbers and add documentation for file operations.

The verification counts (13 and 2) appear to be magic numbers. Consider:

  1. Extracting these as named constants
  2. Adding documentation explaining why these specific numbers are expected
  3. Adding test cases for the actual file paths being requested
+    /**
+     * Expected number of files to copy for instrumentation mode.
+     * This includes:
+     * - [List the specific files or types of files expected]
+     */
+    private static final int INSTRUMENTATION_FILES_COUNT = 13;
+
+    /**
+     * Expected number of files to copy for AspectJ mode.
+     * This includes:
+     * - [List the specific files or types of files expected]
+     */
+    private static final int ASPECTJ_FILES_COUNT = 2;

     @Test
     void testFilesToCopy_InstrumentationMode() {
         try (MockedStatic<FileTools> mockedFileTools = mockStatic(FileTools.class)) {
+            List<String[]> capturedPaths = new ArrayList<>();
             mockedFileTools.when(() -> FileTools.resolveOnResources(any(String[].class)))
-                    .thenReturn(mock(Path.class));
+                    .then(inv -> {
+                        capturedPaths.add(inv.getArgument(0));
+                        return mock(Path.class);
+                    });
             instrumentationMode.filesToCopy();
-            mockedFileTools.verify(() -> FileTools.resolveOnResources(any(String[].class)), times(13));
+            mockedFileTools.verify(() -> FileTools.resolveOnResources(any(String[].class)), 
+                times(INSTRUMENTATION_FILES_COUNT));
+            // Verify specific paths
+            // TODO: Add assertions for expected paths in capturedPaths
         }
     }

Also applies to: 43-51


@Test
void testFilesToCopy_AspectJMode() {
try (MockedStatic<FileTools> mockedFileTools = mockStatic(FileTools.class)) {
mockedFileTools.when(() -> FileTools.resolveOnResources(any(String[].class)))
.thenReturn(mock(Path.class));
aspectjMode.filesToCopy();
mockedFileTools.verify(() -> FileTools.resolveOnResources(any(String[].class)), times(2));
}
}

@Test
void testFileValues_InstrumentationMode() {
try (MockedStatic<FileTools> mockedFileTools = mockStatic(FileTools.class)) {
mockedFileTools.when(() -> FileTools.generatePackageNameArray(anyString(), anyInt()))
.thenReturn(new String[]{"mocked", "array"});
instrumentationMode.fileValues("com.example", "MainClass");
mockedFileTools.verify(() -> FileTools.generatePackageNameArray(anyString(), anyInt()), times(12));
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve testFileValues with meaningful test data.

The test uses a hardcoded package name and doesn't verify the actual content of the generated arrays.

     @Test
     void testFileValues_InstrumentationMode() {
         try (MockedStatic<FileTools> mockedFileTools = mockStatic(FileTools.class)) {
+            String testPackage = "de.tum.cit.ase.test";
+            String testMainClass = "TestMain";
+            String[] expectedArray = {"de", "tum", "cit", "ase", "test"};
             mockedFileTools.when(() -> FileTools.generatePackageNameArray(anyString(), anyInt()))
-                    .thenReturn(new String[]{"mocked", "array"});
+                    .thenReturn(expectedArray);
-            instrumentationMode.fileValues("com.example", "MainClass");
+            instrumentationMode.fileValues(testPackage, testMainClass);
             mockedFileTools.verify(() -> FileTools.generatePackageNameArray(anyString(), anyInt()), times(12));
+            
+            // Verify the actual parameters passed
+            mockedFileTools.verify(() -> FileTools.generatePackageNameArray(eq(testPackage), anyInt()));
         }
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
@Test
void testFileValues_InstrumentationMode() {
try (MockedStatic<FileTools> mockedFileTools = mockStatic(FileTools.class)) {
mockedFileTools.when(() -> FileTools.generatePackageNameArray(anyString(), anyInt()))
.thenReturn(new String[]{"mocked", "array"});
instrumentationMode.fileValues("com.example", "MainClass");
mockedFileTools.verify(() -> FileTools.generatePackageNameArray(anyString(), anyInt()), times(12));
}
}
@Test
void testFileValues_InstrumentationMode() {
try (MockedStatic<FileTools> mockedFileTools = mockStatic(FileTools.class)) {
String testPackage = "de.tum.cit.ase.test";
String testMainClass = "TestMain";
String[] expectedArray = {"de", "tum", "cit", "ase", "test"};
mockedFileTools.when(() -> FileTools.generatePackageNameArray(anyString(), anyInt()))
.thenReturn(expectedArray);
instrumentationMode.fileValues(testPackage, testMainClass);
mockedFileTools.verify(() -> FileTools.generatePackageNameArray(anyString(), anyInt()), times(12));
// Verify the actual parameters passed
mockedFileTools.verify(() -> FileTools.generatePackageNameArray(eq(testPackage), anyInt()));
}
}


@Test
void testReset() {
try {
ClassLoader classLoader = Thread.currentThread().getContextClassLoader();
Class<?> settingsClass = Class.forName("de.tum.cit.ase.ares.api.aop.java.JavaSecurityTestCaseSettings", true, classLoader);
Method resetMethod = settingsClass.getDeclaredMethod("reset");
resetMethod.setAccessible(true);
resetMethod.invoke(null);
} catch (Exception e) {
fail("Exception should not have been thrown: " + e.getMessage());
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
package de.tum.cit.ase.ares.aop;

import de.tum.cit.ase.ares.api.aop.java.JavaSecurityTestCaseSettings;
import org.junit.jupiter.api.Test;

import java.lang.reflect.Constructor;
import java.lang.reflect.Field;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;

import static org.junit.jupiter.api.Assertions.*;

class JavaSecurityTestCaseSettingsTest {

@Test
void testConstructorThrowsException() {
try {
Constructor<JavaSecurityTestCaseSettings> constructor = JavaSecurityTestCaseSettings.class.getDeclaredConstructor();
constructor.setAccessible(true);
constructor.newInstance();
fail("Expected SecurityException to be thrown");
} catch (InvocationTargetException e) {
assertInstanceOf(SecurityException.class, e.getCause());
assertEquals("Ares Security Error (Reason: Ares-Code; Stage: Creation): JavaSecurityTestCaseSettings is a utility class and should not be instantiated.", e.getCause().getMessage());
} catch (Exception e) {
fail("Unexpected exception: " + e);
}
}

@Test
void testResetMethod() {
try {
Field aopModeField = JavaSecurityTestCaseSettings.class.getDeclaredField("aopMode");
Field allowedListedClassesField = JavaSecurityTestCaseSettings.class.getDeclaredField("allowedListedClasses");
Field portsAllowedToBeConnectedToField = JavaSecurityTestCaseSettings.class.getDeclaredField("portsAllowedToBeConnectedTo");

aopModeField.setAccessible(true);
allowedListedClassesField.setAccessible(true);
portsAllowedToBeConnectedToField.setAccessible(true);

aopModeField.set(null, "test");
allowedListedClassesField.set(null, new String[]{"testClass"});
portsAllowedToBeConnectedToField.set(null, new int[]{8080});

Method resetMethod = JavaSecurityTestCaseSettings.class.getDeclaredMethod("reset");
resetMethod.setAccessible(true);
resetMethod.invoke(null);

assertNull(aopModeField.get(null));
assertNull(allowedListedClassesField.get(null));
assertNull(portsAllowedToBeConnectedToField.get(null));

Field pathsAllowedToBeReadField = JavaSecurityTestCaseSettings.class.getDeclaredField("pathsAllowedToBeRead");
pathsAllowedToBeReadField.setAccessible(true);
assertNull(pathsAllowedToBeReadField.get(null));

Field pathsAllowedToBeOverwrittenField = JavaSecurityTestCaseSettings.class.getDeclaredField("pathsAllowedToBeOverwritten");
pathsAllowedToBeOverwrittenField.setAccessible(true);
assertNull(pathsAllowedToBeOverwrittenField.get(null));

} catch (Exception e) {
fail("Unexpected exception: " + e);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
package de.tum.cit.ase.ares.aop;

import de.tum.cit.ase.ares.api.aop.java.JavaSecurityTestCase;
import de.tum.cit.ase.ares.api.aop.java.JavaSecurityTestCaseSupported;
import de.tum.cit.ase.ares.api.policy.SecurityPolicy.ResourceAccesses;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.mockito.MockedStatic;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.util.List;

import static org.junit.jupiter.api.Assertions.*;
import static org.mockito.Mockito.*;

class JavaSecurityTestCaseTest {

private JavaSecurityTestCase javaSecurityTestCase;
private ResourceAccesses resourceAccesses;

@BeforeEach
void setUp() {
JavaSecurityTestCaseSupported supported = JavaSecurityTestCaseSupported.FILESYSTEM_INTERACTION;
resourceAccesses = mock(ResourceAccesses.class);
javaSecurityTestCase = new JavaSecurityTestCase(supported, resourceAccesses);
}

@Test
void testWriteAOPSecurityTestCase() {
String result = javaSecurityTestCase.writeAOPSecurityTestCase("INSTRUMENTATION");
assertEquals("", result);
}

@Test
void testWriteAOPSecurityTestCaseFile() {
List<String> allowedListedClasses = List.of("TestClass");
List<JavaSecurityTestCase> javaSecurityTestCases = List.of(javaSecurityTestCase);

String result = JavaSecurityTestCase.writeAOPSecurityTestCaseFile(
"INSTRUMENTATION",
"de.tum.cit",
allowedListedClasses,
javaSecurityTestCases
);

assertTrue(result.contains("private static String aopMode"));
assertTrue(result.contains("private static String restrictedPackage"));
assertTrue(result.contains("private static String[] allowedListedClasses"));
}

@Test
void testExecuteAOPSecurityTestCase() {
try (MockedStatic<JavaSecurityTestCase> mockedStatic = mockStatic(JavaSecurityTestCase.class)) {
javaSecurityTestCase.executeAOPSecurityTestCase("INSTRUMENTATION");
mockedStatic.verify(() -> JavaSecurityTestCase.setJavaAdviceSettingValue(anyString(), any(), eq("INSTRUMENTATION")), atLeastOnce());
}
}

@Test
void testGetPermittedFilePaths() throws Exception {
Method method = JavaSecurityTestCase.class.getDeclaredMethod("getPermittedFilePaths", String.class);
method.setAccessible(true);
List<String> filePaths = (List<String>) method.invoke(javaSecurityTestCase, "read");
assertEquals(filePaths.size(), 0);
}

@Test
void testGenerateAdviceSettingValue() throws Exception {
Method method = JavaSecurityTestCase.class.getDeclaredMethod("generateAdviceSettingValue", String.class, String.class, Object.class);
method.setAccessible(true);
String result = (String) method.invoke(null, "String", "testAdvice", "testValue");
assertEquals("private static String testAdvice = \"testValue\";\n", result);
result = (String) method.invoke(null, "String[]", "testAdviceArray", List.of("value1", "value2"));
assertEquals("private static String[] testAdviceArray = new String[] {\"value1\", \"value2\"};\n", result);
InvocationTargetException thrown = assertThrows(InvocationTargetException.class, () -> {
method.invoke(null, "UnknownType", "testAdvice", "value");
});
assertEquals(SecurityException.class, thrown.getCause().getClass());
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
package de.tum.cit.ase.ares.aop.instrumentation.advice;

import de.tum.cit.ase.ares.api.aop.java.instrumentation.advice.JavaInstrumentationAdviceToolbox;
import org.junit.jupiter.api.Test;
import org.mockito.MockedStatic;

import java.lang.reflect.Method;

import static org.junit.jupiter.api.Assertions.*;
import static org.mockito.Mockito.*;

class JavaInstrumentationAdviceToolboxTest {

@Test
void testCheckFileSystemInteraction_AllowedInteraction() {
try (MockedStatic<JavaInstrumentationAdviceToolbox> mockedToolbox = mockStatic(JavaInstrumentationAdviceToolbox.class)) {
Method getValueFromSettings = JavaInstrumentationAdviceToolbox.class.getDeclaredMethod("getValueFromSettings", String.class);
getValueFromSettings.setAccessible(true);

mockedToolbox.when(() -> getValueFromSettings.invoke(null, "aopMode")).thenReturn("INSTRUMENTATION");
mockedToolbox.when(() -> getValueFromSettings.invoke(null, "restrictedPackage")).thenReturn("de.tum.cit.ase");
mockedToolbox.when(() -> getValueFromSettings.invoke(null, "allowedListedClasses")).thenReturn(new String[]{"de.tum.cit.ase.safe"});
mockedToolbox.when(() -> getValueFromSettings.invoke(null, "pathsAllowedToBeRead")).thenReturn(new String[]{"/allowed/path"});

assertDoesNotThrow(() -> JavaInstrumentationAdviceToolbox.checkFileSystemInteraction(
"read",
"de.tum.cit.ase.safe.FileReader",
"readFile",
"(Ljava/lang/String;)V",
null,
new Object[]{"/allowed/path"}
));
} catch (Exception e) {
fail("Exception should not have been thrown: " + e.getMessage());
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Security test coverage is insufficient

The test suite only contains a single test method testCheckFileSystemInteraction_AllowedInteraction for file system security checks, which only verifies the allowed path scenario. Given this is security-critical functionality, the current coverage is inadequate.

Required test scenarios:

  • Denied file system interactions (e.g., accessing restricted paths)
  • Edge cases with malformed paths
  • Different file operations (write, delete)
  • Attempts to access paths outside allowed directories
  • Null or empty path arguments

Additionally:

  • Using reflection to access private methods reduces test maintainability
  • Exception handling could be more specific than catching generic Exception
🔗 Analysis chain

Test coverage needs improvement for security-critical functionality.

While the test covers the allowed interaction path, there are several improvements needed:

  1. The test only verifies the "happy path". For security-critical functionality, we should also test:

    • Denied file system interactions
    • Edge cases with malformed paths
    • Different file operations (write, delete, etc.)
  2. Using reflection to access private methods is a code smell. Consider:

    • Making the method package-private for testing
    • Or creating a test-specific interface

Let's verify if there are other tests covering the denied cases:


Let's search for actual test methods in the test files to verify the coverage:


Let's try a broader search to find all test methods in these files:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other test methods that might cover denied cases
rg -l "checkFileSystemInteraction.*denied|restricted|forbidden" --type java

Length of output: 1613


Script:

#!/bin/bash
# Search for test methods in JavaInstrumentationAdviceToolboxTest.java
rg "void test.*checkFileSystemInteraction" src/test/java/de/tum/cit/ase/ares/aop/instrumentation/advice/JavaInstrumentationAdviceToolboxTest.java -B1 -A1

# Also check JavaSecurityTestCaseTest.java for related tests
rg "void test.*FileSystem" src/test/java/de/tum/cit/ase/ares/aop/JavaSecurityTestCaseTest.java -B1 -A1

Length of output: 260


Script:

#!/bin/bash
# Show all test methods in JavaInstrumentationAdviceToolboxTest.java
echo "=== Tests in JavaInstrumentationAdviceToolboxTest.java ==="
rg "@Test" -A 2 src/test/java/de/tum/cit/ase/ares/aop/instrumentation/advice/JavaInstrumentationAdviceToolboxTest.java

# Show implementation of checkFileSystemInteraction to understand what we should test
echo -e "\n=== Implementation of checkFileSystemInteraction ==="
ast-grep --pattern 'checkFileSystemInteraction($$$) { $$$ }'

Length of output: 734


@Test
void testLocalizeFallback() {
String key = "security.advice.test.key";
String result = JavaInstrumentationAdviceToolbox.localize(key, "arg1", "arg2");
key = "!security.advice.test.key!";
assertEquals(key, result);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance localization test coverage.

The current test only verifies the fallback behavior, but several improvements are needed:

  1. Remove unnecessary key reassignment on line 42
  2. Add test cases for:
    • Different numbers of arguments
    • Null arguments
    • Empty strings
     @Test
     void testLocalizeFallback() {
         String key = "security.advice.test.key";
         String result = JavaInstrumentationAdviceToolbox.localize(key, "arg1", "arg2");
-        key = "!security.advice.test.key!";
-        assertEquals(key, result);
+        assertEquals("!security.advice.test.key!", result);
     }
+
+    @Test
+    void testLocalizeWithDifferentArguments() {
+        assertEquals("!test.key!", JavaInstrumentationAdviceToolbox.localize("test.key"));
+        assertEquals("!test.key!", JavaInstrumentationAdviceToolbox.localize("test.key", (Object[]) null));
+        assertEquals("!test.key!", JavaInstrumentationAdviceToolbox.localize("test.key", ""));
+    }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
@Test
void testLocalizeFallback() {
String key = "security.advice.test.key";
String result = JavaInstrumentationAdviceToolbox.localize(key, "arg1", "arg2");
key = "!security.advice.test.key!";
assertEquals(key, result);
}
@Test
void testLocalizeFallback() {
String key = "security.advice.test.key";
String result = JavaInstrumentationAdviceToolbox.localize(key, "arg1", "arg2");
assertEquals("!security.advice.test.key!", result);
}
@Test
void testLocalizeWithDifferentArguments() {
assertEquals("!test.key!", JavaInstrumentationAdviceToolbox.localize("test.key"));
assertEquals("!test.key!", JavaInstrumentationAdviceToolbox.localize("test.key", (Object[]) null));
assertEquals("!test.key!", JavaInstrumentationAdviceToolbox.localize("test.key", ""));
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
package de.tum.cit.ase.ares.aop.instrumentation.advice;

import de.tum.cit.ase.ares.api.aop.java.instrumentation.advice.JavaInstrumentationAdviceToolbox;
import de.tum.cit.ase.ares.api.aop.java.instrumentation.advice.JavaInstrumentationDeletePathConstructorAdvice;
import org.junit.jupiter.api.Test;
import org.mockito.MockedStatic;

import static org.mockito.Mockito.*;

class JavaInstrumentationDeletePathConstructorAdviceTest {

@Test
void testOnEnter() {
try (MockedStatic<JavaInstrumentationAdviceToolbox> mockedToolbox = mockStatic(JavaInstrumentationAdviceToolbox.class)) {
mockedToolbox.when(() -> JavaInstrumentationAdviceToolbox.checkFileSystemInteraction(
"delete",
"de.tum.cit.ase.ares.api.aop.java.instrumentation.advice.JavaInstrumentationDeletePathConstructorAdvice",
"<init>",
"",
new Object[0],
new Object[]{"param1", "param2"}
)).thenAnswer(invocation -> null);

JavaInstrumentationDeletePathConstructorAdvice.onEnter(
"de.tum.cit.ase.ares.api.aop.java.instrumentation.advice.JavaInstrumentationDeletePathConstructorAdvice",
"param1", "param2"
);

mockedToolbox.verify(() -> JavaInstrumentationAdviceToolbox.checkFileSystemInteraction(
"delete",
"de.tum.cit.ase.ares.api.aop.java.instrumentation.advice.JavaInstrumentationDeletePathConstructorAdvice",
"<init>",
"",
new Object[0],
new Object[]{"param1", "param2"}
));
}
}
MarkusPaulsen marked this conversation as resolved.
Show resolved Hide resolved
}
Loading
Loading