[Testing][JShellAPI] apply pr review fixes

Author: firasrg

Diff for: JShellAPI/README.MD

@@ -102,96 +102,3 @@ The maximum ram allocated per container, in megabytes.
 The cpu configuration of each container, see [--cpus option of docker](https://docs.docker.com/config/containers/resource_constraints/#cpu).
 ### jshellapi.schedulerSessionKillScanRate
 The rate at which the session killer will check and delete session, in seconds, see [Session timeout](#Session-timeout).
-
-## Testing
-
->  The work on testing was made in collaboration with [Alathreon](https://github.com/Alathreon) and [Wazei](https://github.com/tj-wazei). I'd like thank both of them for their trust. - FirasRG
-
-This section outlines the work done to set up the first integration test that evaluates Java code by running it in a [Docker](https://www.docker.com/get-started/) container. The test ensures that the [Eval endpoint](#eval) can execute code within the containerized environment of [**JShellWrapper**](../JShellWrapper).
-
-### Usage
-
-```java
-@ContextConfiguration(classes = Main.class)
-@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT)
-public class JShellApiTests {
-
-    @Autowired
-    private WebTestClient webTestClient;
-
-    @Test
-    @DisplayName("When posting code snippet, evaluate it then returns successfully result")
-    public void evaluateCodeSnippetTest() {
-
-        final String testEvalId = "test";
-        
-        final String firstCodeExpression = "int a = 2+2;";
-
-        final JShellSnippetResult firstCodeSnippet = new JShellSnippetResult(SnippetStatus.VALID, SnippetType.ADDITION, 1, firstCodeExpression, "4");
-        
-        final JShellResult firstCodeExpectedResult = getJShellResultDefaultInstance(firstCodeSnippet);
-
-        assertThat(testEval(testEvalId, firstCodeExpression)).isEqualTo(firstCodeExpectedResult);
-        
-        // performing a second code execution test ...
-    }
-    // some methods ...
-}
-```
-
-### 1. Java Test Setup
-
-The [@SpringBootTest](https://docs.spring.io/spring-boot/api/java/org/springframework/boot/test/context/SpringBootTest.html) and [@ContextConfiguration](https://docs.spring.io/spring-framework/reference/testing/annotations/integration-spring/annotation-contextconfiguration.html) annotations are needed to prepare the app to tests, like in a real scenario.
-
-NOTE: _Test classes must be located under `/src/test/java/{org.togetherjava.jshellapi}`._
-
-- The test uses [WebTestClient](https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/test/web/reactive/server/WebTestClient.html) to make HTTP calls to the target endpoint.
-- Multiple API calls are made within the test method, so a utility instance method was created for reuse.
-- The test ensures that code is correctly evaluated inside the **JShellWrapper** container.
-
-### 2. Gradle Configuration for Tests
-
-The `build.gradle` of this project has been updated to handle **JShellWrapper** Docker image lifecycle during tests.
-
-- **JShellWrapper Image Name**: the image name is injected from the root [build.gradle](../build.gradle) file, to this project's [build.gradle](build.gradle) file and also to [application.yaml](src/main/resources/application.yaml)!
-- **JShellWrapper Docker Image**: The image is built before the tests run.
-- **Container & Cleanup**: After the tests finish, the container and image are removed to ensure a clean environment.
-
-```groovy
-def jshellWrapperImageName = rootProject.ext.jShellWrapperImageName;
-
-processResources {
-  filesMatching('application.yaml') {
-    expand(jShellWrapperImageName: jshellWrapperImageName)
-  }
-}
-
-def taskBuildDockerImage = tasks.register('buildDockerImage') {
-    group = 'docker'
-    description = 'builds jshellwrapper as docker image'
-    dependsOn project(':JShellWrapper').tasks.named('jibDockerBuild')
-}
-
-def taskRemoveDockerImage = tasks.register('removeDockerImage', Exec) {
-    group = 'docker'
-    description = 'removes jshellwrapper image'
-    commandLine 'docker', 'rmi', '-f', jshellWrapperImageName
-}
-
-test {
-    dependsOn taskBuildDockerImage
-    finalizedBy taskRemoveDockerImage
-}
-```
-
-Below are the key dependencies that were added or modified in the `build.gradle` file of this project :
-
-```groovy
-testImplementation('org.springframework.boot:spring-boot-starter-test') {
-  exclude group: 'ch.qos.logback', module: 'logback-classic'
-}
-testImplementation 'org.springframework.boot:spring-boot-starter-webflux'
-```
-
-- The `logback-classic` has been excluded because of an issue encountered when running tests. The issue is typically about a conflict between some dependencies (This solution has been brought based on [a _good_ answer on Stackoverflow](https://stackoverflow.com/a/42641450/10000150))
-- The `spring-boot-starter-webflux` was needed in order to be able to use **WebTestClient**.

Diff for: JShellAPI/build.gradle

@@ -14,6 +14,9 @@ dependencies {
     implementation 'com.github.docker-java:docker-java-core:3.3.6'
 
     testImplementation('org.springframework.boot:spring-boot-starter-test') {
+        // `logback-classic` has been excluded because of an issue encountered when running tests.
+        // It's about a conflict between some dependencies.
+        // The solution has been brought based on a good answer on Stackoverflow: https://stackoverflow.com/a/42641450/10000150
         exclude group: 'ch.qos.logback', module: 'logback-classic'
     }
     testImplementation 'org.springframework.boot:spring-boot-starter-webflux'
@@ -43,11 +46,13 @@ shadowJar {
     archiveVersion.set('')
 }
 
+// -- Gradle testing configuration
+
 def jshellWrapperImageName = rootProject.ext.jShellWrapperImageName;
 
 processResources {
     filesMatching('application.yaml') {
-        expand(jShellWrapperImageName: jshellWrapperImageName)
+        expand("JSHELL_WRAPPER_IMAGE_NAME": jshellWrapperImageName)
     }
 }
 

Diff for: JShellAPI/src/main/java/org/togetherjava/jshellapi/Config.java

@@ -2,13 +2,28 @@
 
 import org.springframework.boot.context.properties.ConfigurationProperties;
 import org.springframework.lang.Nullable;
+import org.springframework.util.StringUtils;
 
 @ConfigurationProperties("jshellapi")
 public record Config(long regularSessionTimeoutSeconds, long oneTimeSessionTimeoutSeconds,
         long evalTimeoutSeconds, long evalTimeoutValidationLeeway, int sysOutCharLimit,
         long maxAliveSessions, int dockerMaxRamMegaBytes, double dockerCPUsUsage,
         @Nullable String dockerCPUSetCPUs, long schedulerSessionKillScanRateSeconds,
-        long dockerResponseTimeout, long dockerConnectionTimeout) {
+        long dockerResponseTimeout, long dockerConnectionTimeout, String jshellWrapperImageName) {
+
+    public static final String JSHELL_WRAPPER_IMAGE_NAME_TAG = ":master";
+
+    private static boolean checkJShellWrapperImageName(String imageName) {
+        if (!StringUtils.hasText(imageName)
+                || !imageName.endsWith(Config.JSHELL_WRAPPER_IMAGE_NAME_TAG)) {
+            return false;
+        }
+
+        final String imageNameFirstPart = imageName.split(Config.JSHELL_WRAPPER_IMAGE_NAME_TAG)[0];
+
+        return StringUtils.hasText(imageNameFirstPart);
+    }
+
     public Config {
         if (regularSessionTimeoutSeconds <= 0)
             throw new IllegalArgumentException("Invalid value " + regularSessionTimeoutSeconds);
@@ -35,5 +50,9 @@ public record Config(long regularSessionTimeoutSeconds, long oneTimeSessionTimeo
             throw new IllegalArgumentException("Invalid value " + dockerResponseTimeout);
         if (dockerConnectionTimeout <= 0)
             throw new IllegalArgumentException("Invalid value " + dockerConnectionTimeout);
+
+        if (!checkJShellWrapperImageName(jshellWrapperImageName)) {
+            throw new IllegalArgumentException("Invalid value " + jshellWrapperImageName);
+        }
     }
 }

Diff for: JShellAPI/src/main/java/org/togetherjava/jshellapi/rest/ApiEndpoints.java

@@ -1,10 +1,7 @@
 package org.togetherjava.jshellapi.rest;
 
 /**
- * This class holds endpoints mentioned in controllers. The main objective is to keep endpoints
- * synchronized with testing classes.
- *
- * @author Firas Regaieg
+ * Holds endpoints mentioned in controllers.
  */
 public final class ApiEndpoints {
     private ApiEndpoints() {}

Diff for: JShellAPI/src/main/java/org/togetherjava/jshellapi/service/DockerService.java

@@ -7,11 +7,9 @@
 import com.github.dockerjava.core.DefaultDockerClientConfig;
 import com.github.dockerjava.core.DockerClientImpl;
 import com.github.dockerjava.httpclient5.ApacheDockerHttpClient;
-import jakarta.el.PropertyNotFoundException;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.springframework.beans.factory.DisposableBean;
-import org.springframework.beans.factory.annotation.Value;
 import org.springframework.lang.Nullable;
 import org.springframework.stereotype.Service;
 
@@ -31,8 +29,7 @@ public class DockerService implements DisposableBean {
 
     private final DockerClient client;
 
-    @Value("${jshell-wrapper.image-name}")
-    private String jshellWrapperImageName;
+    private final String jshellWrapperBaseImageName;
 
     public DockerService(Config config) {
         DefaultDockerClientConfig clientConfig =
@@ -45,6 +42,9 @@ public DockerService(Config config) {
                     .build();
         this.client = DockerClientImpl.getInstance(clientConfig, httpClient);
 
+        this.jshellWrapperBaseImageName =
+                config.jshellWrapperImageName().split(Config.JSHELL_WRAPPER_IMAGE_NAME_TAG)[0];
+
         cleanupLeftovers(WORKER_UNIQUE_ID);
     }
 
@@ -64,33 +64,23 @@ private void cleanupLeftovers(UUID currentId) {
 
     public String spawnContainer(long maxMemoryMegs, long cpus, @Nullable String cpuSetCpus,
             String name, Duration evalTimeout, long sysoutLimit) throws InterruptedException {
-        String imageName = Optional.ofNullable(this.jshellWrapperImageName)
-            .orElseThrow(() -> new PropertyNotFoundException(
-                    "unable to find jshellWrapper image name property"));
-
-        String[] imageNameParts = imageName.split(":master");
-
-        if (imageNameParts.length != 1) {
-            throw new IllegalArgumentException("invalid jshellWrapper image name");
-        }
-
-        String baseImageName = imageNameParts[0];
 
         boolean presentLocally = client.listImagesCmd()
-            .withFilter("reference", List.of(baseImageName))
+            .withFilter("reference", List.of(jshellWrapperBaseImageName))
             .exec()
             .stream()
             .flatMap(it -> Arrays.stream(it.getRepoTags()))
-            .anyMatch(it -> it.endsWith(":master"));
+            .anyMatch(it -> it.endsWith(Config.JSHELL_WRAPPER_IMAGE_NAME_TAG));
 
         if (!presentLocally) {
-            client.pullImageCmd(baseImageName)
+            client.pullImageCmd(jshellWrapperBaseImageName)
                 .withTag("master")
                 .exec(new PullImageResultCallback())
                 .awaitCompletion(5, TimeUnit.MINUTES);
         }
 
-        return client.createContainerCmd(baseImageName + ":master")
+        return client
+            .createContainerCmd(jshellWrapperBaseImageName + Config.JSHELL_WRAPPER_IMAGE_NAME_TAG)
             .withHostConfig(HostConfig.newHostConfig()
                 .withAutoRemove(true)
                 .withInit(true)

Diff for: JShellAPI/src/main/resources/META-INF/additional-spring-configuration-metadata.json

@@ -1,7 +1,7 @@
 {
   "properties": [
     {
-      "name": "jshell-wrapper.image-name",
+      "name": "jshellapi.jshellwrapper-imageName",
       "type": "java.lang.String",
       "description": "JShellWrapper image name injected from the top-level gradle build file."
   }

Diff for: JShellAPI/src/main/resources/application.yaml

@@ -20,8 +20,8 @@ jshellapi:
   dockerResponseTimeout: 60
   dockerConnectionTimeout: 60
 
-jshell-wrapper:
-  image-name: ${jShellWrapperImageName}
+  # JShellWrapper related
+  jshellWrapperImageName: ${JSHELL_WRAPPER_IMAGE_NAME}
 
 server:
   error:

Diff for: JShellAPI/src/test/java/org/togetherjava/jshellapi/JShellApiTests.java

@@ -4,6 +4,7 @@
 import org.junit.jupiter.api.Test;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.boot.test.context.SpringBootTest;
+import org.springframework.test.context.ActiveProfiles;
 import org.springframework.test.context.ContextConfiguration;
 import org.springframework.test.web.reactive.server.WebTestClient;
 
@@ -19,54 +20,47 @@
 import static org.assertj.core.api.Assertions.assertThat;
 
 /**
- * This class holds integration tests for JShellAPI. It depends on gradle building image task, fore
- * more information check "test" section in gradle.build file.
- *
- * @author Firas Regaieg
+ * Integrates tests for JShellAPI.
  */
+@ActiveProfiles("testing")
 @ContextConfiguration(classes = Main.class)
 @SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT)
 public class JShellApiTests {
 
     @Autowired
     private WebTestClient webTestClient;
 
+    @Autowired
+    private Config testsConfig;
+
     @Test
-    @DisplayName("When posting code snippet, evaluate it then returns successfully result")
+    @DisplayName("When posting code snippet, evaluate it then return successfully result")
     public void evaluateCodeSnippetTest() {
 
         final String testEvalId = "test";
 
-        // -- performing a first code snippet execution
-
-        final String firstCodeExpression = "int a = 2+2;";
-
-        final JShellSnippetResult firstCodeSnippet = new JShellSnippetResult(SnippetStatus.VALID,
-                SnippetType.ADDITION, 1, firstCodeExpression, "4");
-        final JShellResult firstCodeExpectedResult =
-                getJShellResultDefaultInstance(firstCodeSnippet);
-
-        assertThat(testEval(testEvalId, firstCodeExpression)).isEqualTo(firstCodeExpectedResult);
-
-        // -- performing a second code snippet execution
+        // -- first code snippet eval
+        executeCodeEvalTest(testEvalId, "int a = 2+2;", 1, "4");
 
-        final String secondCodeExpression = "a * 2";
-
-        final JShellSnippetResult secondCodeSnippet = new JShellSnippetResult(SnippetStatus.VALID,
-                SnippetType.ADDITION, 2, secondCodeExpression, "8");
+        // -- second code snippet eval
+        executeCodeEvalTest(testEvalId, "a * 2", 2, "8");
+    }
 
-        final JShellResult secondCodeExpectedResult =
-                getJShellResultDefaultInstance(secondCodeSnippet);
+    private void executeCodeEvalTest(String evalId, String codeSnippet, int expectedId,
+            String expectedResult) {
+        final JShellSnippetResult jshellCodeSnippet = new JShellSnippetResult(SnippetStatus.VALID,
+                SnippetType.ADDITION, expectedId, codeSnippet, expectedResult);
 
-        assertThat(testEval(testEvalId, secondCodeExpression)).isEqualTo(secondCodeExpectedResult);
+        assertThat(testEval(evalId, codeSnippet))
+            .isEqualTo(new JShellResult(List.of(jshellCodeSnippet), null, false, ""));
     }
 
     private JShellResult testEval(String testEvalId, String codeInput) {
         final String endpoint =
                 String.join("/", ApiEndpoints.BASE, ApiEndpoints.EVALUATE, testEvalId);
 
-        JShellResult result = this.webTestClient.mutate()
-            .responseTimeout(Duration.ofSeconds(6))
+        return this.webTestClient.mutate()
+            .responseTimeout(Duration.ofSeconds(testsConfig.evalTimeoutSeconds()))
             .build()
             .post()
             .uri(endpoint)
@@ -78,13 +72,5 @@ private JShellResult testEval(String testEvalId, String codeInput) {
             .value((JShellResult evalResult) -> assertThat(evalResult).isNotNull())
             .returnResult()
             .getResponseBody();
-
-        assertThat(result).isNotNull();
-
-        return result;
-    }
-
-    private static JShellResult getJShellResultDefaultInstance(JShellSnippetResult snippetResult) {
-        return new JShellResult(List.of(snippetResult), null, false, "");
     }
 }

Diff for: JShellAPI/src/test/resources/application-testing.yaml

@@ -0,0 +1,11 @@
+jshellapi:
+
+  # Public API Config
+  regularSessionTimeoutSeconds: 10
+
+  # Internal config
+  schedulerSessionKillScanRateSeconds: 6
+
+  # Docker service config
+  dockerResponseTimeout: 6
+  dockerConnectionTimeout: 6