[Testing][JShellAPI] apply pr review fixes

Author: firasrg

JShellAPI/README.MD

@@ -87,111 +87,28 @@ The following startup scripts id can be used :
 - EMPTY : no startup script
 - CUSTOM_DEFAULT : contains basic imports, print methods and range method
 ## Configuration
+
+### Properties
 Properties can be defined in resources/application.properties
-### jshellapi.regularSessionTimeoutSeconds
+#### jshellapi.regularSessionTimeoutSeconds
 The timeout of a regular session, in seconds, see [Session timeout](#Session-timeout).
-### jshellapi.oneTimeSessionTimeoutSeconds
+#### jshellapi.oneTimeSessionTimeoutSeconds
 The timeout of a one-time session, in seconds, see [One-time session timeout](#One-time-session-timeout).
-### jshellapi.evalTimeoutSeconds
+#### jshellapi.evalTimeoutSeconds
 The timeout of an evaluation, in seconds, see [Eval timeout](#Eval-timeout)
-### jshellapi.maxAliveSessions
+#### jshellapi.maxAliveSessions
 The maximum number of alive sessions, see [Errors](#Errors)
-### jshellapi.dockerMaxRamMegaBytes
+#### jshellapi.dockerMaxRamMegaBytes
 The maximum ram allocated per container, in megabytes.
-### jshellapi.dockerCPUsUsage
+#### jshellapi.dockerCPUsUsage
 The cpu configuration of each container, see [--cpus option of docker](https://docs.docker.com/config/containers/resource_constraints/#cpu).
-### jshellapi.schedulerSessionKillScanRate
+#### 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.
+### Testing
+**JShellWrapper** Docker image lifecycle is handled during tests.
 
-### 2. Gradle Configuration for Tests
+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).
 
-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.
+- JShellWrapper Docker 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**.

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(jshellWrapperImageName: jshellWrapperImageName)
     }
 }
 

JShellAPI/src/main/java/org/togetherjava/jshellapi/Config.java

@@ -2,13 +2,14 @@
 
 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 Config {
         if (regularSessionTimeoutSeconds <= 0)
             throw new IllegalArgumentException("Invalid value " + regularSessionTimeoutSeconds);
@@ -35,5 +36,7 @@ public record Config(long regularSessionTimeoutSeconds, long oneTimeSessionTimeo
             throw new IllegalArgumentException("Invalid value " + dockerResponseTimeout);
         if (dockerConnectionTimeout <= 0)
             throw new IllegalArgumentException("Invalid value " + dockerConnectionTimeout);
+        if (!StringUtils.hasText(jshellWrapperImageName))
+            throw new IllegalArgumentException("Invalid value " + jshellWrapperImageName);
     }
 }

JShellAPI/src/main/java/org/togetherjava/jshellapi/dto/JShellResult.java

@@ -9,4 +9,8 @@ public record JShellResult(List<JShellSnippetResult> snippetsResults,
     public JShellResult {
         snippetsResults = List.copyOf(snippetsResults);
     }
+
+    public JShellResult(JShellSnippetResult snippetsResultList) {
+        this(List.of(snippetsResultList), null, false, "");
+    }
 }

JShellAPI/src/main/java/org/togetherjava/jshellapi/rest/ApiEndpoints.java

@@ -4,7 +4,6 @@
  * This class holds endpoints mentioned in controllers. The main objective is to keep endpoints
  * synchronized with testing classes.
  *
- * @author Firas Regaieg
  */
 public final class ApiEndpoints {
     private ApiEndpoints() {}

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 jshellWrapperImageName;
 
     public DockerService(Config config) {
         DefaultDockerClientConfig clientConfig =
@@ -44,6 +41,7 @@ public DockerService(Config config) {
                     .connectionTimeout(Duration.ofSeconds(config.dockerConnectionTimeout()))
                     .build();
         this.client = DockerClientImpl.getInstance(clientConfig, httpClient);
+        this.jshellWrapperImageName = config.jshellWrapperImageName();
 
         cleanupLeftovers(WORKER_UNIQUE_ID);
     }
@@ -64,11 +62,8 @@ 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");
+        String[] imageNameParts = this.jshellWrapperImageName.split(":master");
 
         if (imageNameParts.length != 1) {
             throw new IllegalArgumentException("invalid jshellWrapper image name");

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."
   }

JShellAPI/src/main/resources/application.yaml

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

JShellAPI/src/test/java/org/togetherjava/jshellapi/JShellApiTests.java

@@ -14,15 +14,11 @@
 import org.togetherjava.jshellapi.rest.ApiEndpoints;
 
 import java.time.Duration;
-import java.util.List;
 
 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.
  */
 @ContextConfiguration(classes = Main.class)
 @SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT)
@@ -31,42 +27,38 @@ public class JShellApiTests {
     @Autowired
     private WebTestClient webTestClient;
 
+    @Autowired
+    private Config appConfig;
+
     @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
+        // -- first code snippet eval
 
         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(new JShellResult(firstCodeSnippet));
 
-        assertThat(testEval(testEvalId, firstCodeExpression)).isEqualTo(firstCodeExpectedResult);
-
-        // -- performing a second code snippet execution
+        // -- second code snippet eval
 
         final String secondCodeExpression = "a * 2";
-
         final JShellSnippetResult secondCodeSnippet = new JShellSnippetResult(SnippetStatus.VALID,
                 SnippetType.ADDITION, 2, secondCodeExpression, "8");
-
-        final JShellResult secondCodeExpectedResult =
-                getJShellResultDefaultInstance(secondCodeSnippet);
-
-        assertThat(testEval(testEvalId, secondCodeExpression)).isEqualTo(secondCodeExpectedResult);
+        assertThat(testEval(testEvalId, secondCodeExpression))
+            .isEqualTo(new JShellResult(secondCodeSnippet));
     }
 
     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(appConfig.evalTimeoutSeconds()))
             .build()
             .post()
             .uri(endpoint)
@@ -78,13 +70,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, "");
     }
 }