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 7 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 @@ -57,6 +57,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 @@ -17,6 +17,7 @@ public class FileHandlerConstants {
public static final Path JAVA_REFLECTION_METHODS = FileTools.resolveOnResources("templates", "architecture" , "java", "archunit", "methods", "reflection-methods.txt");
public static final Path JAVA_COMMAND_EXECUTION_METHODS = FileTools.resolveOnResources("templates", "architecture" , "java", "archunit", "methods", "command-execution-methods.txt");
public static final Path JAVA_THREAD_CREATION_METHODS = FileTools.resolveOnResources("templates", "architecture" , "java", "archunit", "methods", "thread-creation-methods.txt");
public static final Path JAVA_CLASSLOADER_METHODS = FileTools.resolveOnResources("templates", "architecture" , "java", "archunit", "methods", "classloader-methods.txt");;
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 the extra semicolon.

There's a double semicolon at the end of the line which should be removed.

-    public static final Path JAVA_CLASSLOADER_METHODS = FileTools.resolveOnResources("templates", "architecture" , "java", "archunit", "methods", "classloader-methods.txt");;
+    public static final Path JAVA_CLASSLOADER_METHODS = FileTools.resolveOnResources("templates", "architecture" , "java", "archunit", "methods", "classloader-methods.txt");
📝 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
public static final Path JAVA_CLASSLOADER_METHODS = FileTools.resolveOnResources("templates", "architecture" , "java", "archunit", "methods", "classloader-methods.txt");;
public static final Path JAVA_CLASSLOADER_METHODS = FileTools.resolveOnResources("templates", "architecture" , "java", "archunit", "methods", "classloader-methods.txt");


private FileHandlerConstants() {
throw new UnsupportedOperationException(localized("security.general.utility.initialization"));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import de.tum.cit.ase.ares.api.architecture.java.archunit.postcompile.JavaArchitectureTestCaseCollection;
import de.tum.cit.ase.ares.api.policy.SecurityPolicy.PackagePermission;

import javax.annotation.Nonnull;
import java.util.HashSet;
import java.util.Set;
import java.util.stream.Collectors;
Expand Down Expand Up @@ -42,7 +43,7 @@ public class JavaArchUnitSecurityTestCase implements ArchitectureSecurityTestCas
*
* @param javaArchitectureTestCaseSupported Selects the supported architecture test case in the Java programming language
*/
public JavaArchUnitSecurityTestCase(JavaArchUnitTestCaseSupported javaArchitectureTestCaseSupported) {
public JavaArchUnitSecurityTestCase(@Nonnull JavaArchUnitTestCaseSupported javaArchitectureTestCaseSupported) {
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

Enhance null-safety consistency and validation.

While adding @nonnull is good, there are several improvements needed:

  1. The second constructor should also have @nonnull for consistency
  2. Runtime null validation should be added
  3. JavaDoc should document the non-null requirement

Here's the suggested implementation:

     /**
      * Constructor for JavaArchUnitSecurityTestCase.
      *
      * @param javaArchitectureTestCaseSupported Selects the supported architecture test case in the Java programming language
+     * @throws NullPointerException if javaArchitectureTestCaseSupported is null
      */
     public JavaArchUnitSecurityTestCase(@Nonnull JavaArchUnitTestCaseSupported javaArchitectureTestCaseSupported) {
         super();
+        this.javaArchitectureTestCaseSupported = java.util.Objects.requireNonNull(javaArchitectureTestCaseSupported, 
+            "javaArchitectureTestCaseSupported must not be null");
-        this.javaArchitectureTestCaseSupported = javaArchitectureTestCaseSupported;
         this.allowedPackages = new HashSet<>();
     }

-    public JavaArchUnitSecurityTestCase(JavaArchUnitTestCaseSupported javaArchitectureTestCaseSupported, Set<PackagePermission> packages) {
+    public JavaArchUnitSecurityTestCase(@Nonnull JavaArchUnitTestCaseSupported javaArchitectureTestCaseSupported,
+                                       @Nonnull Set<PackagePermission> packages) {
         super();
-        this.javaArchitectureTestCaseSupported = javaArchitectureTestCaseSupported;
+        this.javaArchitectureTestCaseSupported = java.util.Objects.requireNonNull(javaArchitectureTestCaseSupported,
+            "javaArchitectureTestCaseSupported must not be null");
-        this.allowedPackages = packages.stream().map(PackagePermission::importTheFollowingPackage).collect(Collectors.toSet());
+        this.allowedPackages = java.util.Objects.requireNonNull(packages, "packages must not be null")
+            .stream()
+            .map(PackagePermission::importTheFollowingPackage)
+            .collect(Collectors.toSet());
     }

Committable suggestion was skipped due to low confidence.

super();
this.javaArchitectureTestCaseSupported = javaArchitectureTestCaseSupported;
this.allowedPackages = new HashSet<>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -153,4 +153,11 @@ public boolean test(JavaAccess<?> javaAccess) {
FileHandlerConstants.JAVA_THREAD_CREATION_METHODS
);
//</editor-fold>

//<editor-fold desc="ClassLoader related rule">
public static final ArchRule NO_CLASSES_SHOULD_USE_CLASSLOADERS = createNoClassShouldHaveMethodRule(
"uses ClassLoaders",
FileHandlerConstants.JAVA_CLASSLOADER_METHODS
);
//</editor-fold>
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

Based on the findings, I can now generate the final response:

ClassLoader rule needs test coverage and method list enhancement

The implementation has two issues that need attention:

  • The classloader-methods.txt file only contains the class name java.lang.ClassLoader but lacks the actual method signatures that should be restricted
  • There are no specific test cases for the NO_CLASSES_SHOULD_USE_CLASSLOADERS rule in JavaArchitectureTestCaseCollectionTest.java

Required actions:

  • Add comprehensive method signatures to classloader-methods.txt (e.g., loadClass, defineClass, etc.)
  • Add test cases in JavaArchitectureTestCaseCollectionTest.java to verify the rule's behavior
🔗 Analysis chain

Implementation looks good, but needs test coverage.

The new ClassLoader rule follows the established pattern and is well-structured. However, since this PR is about adding tests, we should ensure proper test coverage for this new rule.

Let's verify the constant and its potential test coverage:


Let me try a different search strategy to verify the constant and test coverage:

This should help us:

  1. Find the actual definition of the JAVA_CLASSLOADER_METHODS constant
  2. List all test files in the project
  3. Find any test methods related to ClassLoader functionality

Let me check the content of the classloader-methods.txt file and look for related test cases:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if the constant exists and if there are any tests for this rule
echo "Checking for JAVA_CLASSLOADER_METHODS constant:"
rg "JAVA_CLASSLOADER_METHODS" -l

echo -e "\nChecking for potential test files:"
fd -e java -e kt . | rg -i "test.*classloader"

Length of output: 311


Script:

#!/bin/bash
# Search for the constant definition and its usages
echo "Searching for JAVA_CLASSLOADER_METHODS constant definition:"
rg "JAVA_CLASSLOADER_METHODS\s*=" -A 5

echo -e "\nSearching for test files in the project:"
fd "Test.*\.java$"

echo -e "\nSearching for ClassLoader-related test methods:"
ast-grep --pattern 'void test$_ClassLoader$_() { $$$ }'

Length of output: 7925


Script:

#!/bin/bash
# Check the content of classloader-methods.txt
echo "Checking classloader-methods.txt content:"
cat src/main/resources/de/tum/cit/ase/ares/api/templates/architecture/java/archunit/methods/classloader-methods.txt || echo "File not found"

echo -e "\nChecking for ClassLoader-related tests in JavaArchitectureTestCaseCollectionTest:"
rg -A 5 "NO_CLASSES_SHOULD_USE_CLASSLOADERS" "src/test/java/de/tum/cit/ase/ares/api/architecture/JavaArchitectureTestCaseCollectionTest.java"

echo -e "\nChecking for other potential ClassLoader test files:"
rg -l "ClassLoader" src/test/java

Length of output: 985

}
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
import java.util.Optional;
import java.util.Set;
import java.util.HashSet;
import java.util.Collections;

import static com.tngtech.archunit.lang.ConditionEvent.createMessage;
import static com.tngtech.archunit.thirdparty.com.google.common.base.Preconditions.checkNotNull;
Expand Down Expand Up @@ -54,17 +53,12 @@ public TransitivelyAccessesMethodsCondition(DescribedPredicate<? super JavaAcces
*/
@Override
public void check(JavaClass item, ConditionEvents events) {
boolean hastTransitiveAccess = false;
for (JavaAccess<?> target : item.getAccessesFromSelf()) {
List<JavaAccess<?>> dependencyPath = transitiveAccessPath.findPathTo(target);
if (!dependencyPath.isEmpty()) {
events.add(newTransitiveAccessPathFoundEvent(target, dependencyPath));
hastTransitiveAccess = true;
}
}
if (!hastTransitiveAccess) {
events.add(newNoTransitiveDependencyPathFoundEvent(item));
}
}

/**
Expand All @@ -85,18 +79,12 @@ private static ConditionEvent newTransitiveAccessPathFoundEvent(JavaAccess<?> ja
return SimpleConditionEvent.satisfied(javaClass, createMessage(javaClass, message));
}

/**
* @return a violated event if no transitive dependency path was found
*/
private static ConditionEvent newNoTransitiveDependencyPathFoundEvent(JavaClass javaClass) {
return SimpleConditionEvent.violated(javaClass, createMessage(javaClass, "does not transitively depend on any matching class"));
}

private class TransitiveAccessPath {
public class TransitiveAccessPath {
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

Keep TransitiveAccessPath as a private inner class

The class TransitiveAccessPath is only used internally within TransitivelyAccessesMethodsCondition and has a single test usage. Making it public exposes unnecessary implementation details. The test can still access the inner class using package-private visibility since the test is in the same package hierarchy (de.tum.cit.ase.ares.api.architecture).

  • Change public class TransitiveAccessPath back to private class TransitiveAccessPath
  • If needed, consider making it package-private (remove access modifier) to maintain test access while preserving encapsulation
🔗 Analysis chain

Consider keeping TransitiveAccessPath as an internal implementation detail.

Making this inner class public exposes implementation details and could make future refactoring more difficult. If this change is solely for testing purposes, consider using alternative testing approaches that don't require breaking encapsulation.

Let's check if this class is only used in tests:

Alternative approaches to consider:

  1. Use package-private visibility if the tests are in the same package
  2. Extract the class if it represents a reusable concept
  3. Use testing utilities like reflection if absolutely necessary

Let me verify if this class is used in test files as well to understand the complete usage pattern.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for usage of TransitiveAccessPath outside of test files
rg "TransitiveAccessPath" --type java --glob '!**/*Test.java'

Length of output: 1797


Script:

#!/bin/bash
# Search for usage of TransitiveAccessPath in test files
rg "TransitiveAccessPath" --type java --glob '**/*Test.java'

Length of output: 385

/**
* @return some outgoing transitive dependency path to the supplied class or empty if there is none
*/
List<JavaAccess<?>> findPathTo(JavaAccess<?> method) {
@SuppressWarnings("java:S1452")
public List<JavaAccess<?>> findPathTo(JavaAccess<?> method) {
ImmutableList.Builder<JavaAccess<?>> transitivePath = ImmutableList.builder();
addAccessesToPathFrom(method, transitivePath, new HashSet<>());
return transitivePath.build().reverse();
Expand Down Expand Up @@ -138,18 +126,10 @@ private Set<JavaAccess<?>> getDirectAccessTargetsOutsideOfAnalyzedClasses(JavaAc
JavaClass resolvedTarget = resolveTargetOwner(item.getTargetOwner());

// Match the accesses to the target
// TODO Sarp: This is a workaround use Wala to get the actual call graph!!!!
Set<JavaClass> subclasses = resolvedTarget.getSubclasses().stream().map(this::resolveTargetOwner).collect(toSet());
subclasses.add(resolvedTarget);

/**
* If the number of subclasses is more than 20, return an empty set.
* These classes are always generic interfaces or abstract classes
* TODO: Check if this is also the case for foreign packages
*/
if (subclasses.size() > 20 || isExceptionOrError(resolvedTarget)) {
return Collections.emptySet();
}

return subclasses.stream()
.map(javaClass ->
getAccessesFromClass(javaClass, item.getTarget().getFullName().substring(item.getTargetOwner().getFullName().length())))
Expand All @@ -172,9 +152,5 @@ private JavaClass resolveTargetOwner(JavaClass targetOwner) {
Optional<JavaClass> resolvedTarget = CustomClassResolver.tryResolve(targetOwner.getFullName());
return resolvedTarget.orElse(targetOwner);
}

private boolean isExceptionOrError(JavaClass javaClass) {
return javaClass.isAssignableTo(Exception.class) || javaClass.isAssignableTo(Error.class);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ private void createSecurityTestCases() {
(Supplier<List<?>>) resourceAccesses::regardingFileSystemInteractions,
(Supplier<List<?>>) resourceAccesses::regardingNetworkConnections,
(Supplier<List<?>>) resourceAccesses::regardingCommandExecutions,
// (Supplier<List<?>>) resourceAccesses::regardingThreadCreations,
(Supplier<List<?>>) resourceAccesses::regardingThreadCreations,
};
IntStream
.range(0, methods.length)
Expand Down Expand Up @@ -323,6 +323,7 @@ public void executeSecurityTestCases() {
//<editor-fold desc="Enforce fixed rules code">
JavaArchitectureTestCaseCollection.NO_CLASSES_SHOULD_USE_REFLECTION.check(classes);
JavaArchitectureTestCaseCollection.NO_CLASSES_SHOULD_TERMINATE_JVM.check(classes);
JavaArchitectureTestCaseCollection.NO_CLASSES_SHOULD_USE_CLASSLOADERS.check(classes);
//</editor-fold>

//<editor-fold desc="Enforce variable rules code">
Expand Down
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 @@ -153,6 +153,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());
}
}
Loading