[JShellAPI][Testing] Setting up First Integration Test - Simple code snippet evaluation scenario #50

firasrg · 2024-08-10T16:48:57Z

Pull-request

I have read the contributing guidelines.
I have read the code guidelines.
I have created a relating issue.

Changes

Some changes in gradle files to
New integration test

Description

This PR is about setting up a first working integration test for JShellAPI only, it contains the following :

New global property in root project'd gradle.build file to share JShellWrapper's image name across subprojects gradle.build files and Java files as well.
[JShellAPI] New custom tasks in gradle.build file for docker image : build + remove;
[JShellAPI] New Integration tests class with a 1st working test on WS POST/jshel/eval .

Note: this PR is about setting the roadmap for testing strategy for JShellAPI only. It doesn't close the relevant issue.

Note: this includes adding a 1st class for tests;

Note: this includes changes and improvements in gradle.build files;

… and test classes

Alathreon · 2024-09-10T21:28:37Z

I don't have time to do a review, so I'll just write here.
There are two problems :

Your test is too complicated to test something too simple, you need to simplify it, create methods or classes for this so your test can be as simple as for example assertEquals(sendEval("2+2"), new JShellResult(...))
Also include a test to test state. For example, you can send an eval "var list = List.of(1, 2, 3)" and then send a separate eval like "println(list)"

firasrg · 2024-09-12T21:15:15Z

Hello @Alathreon !

Your test is too complicated to test something too simple

I think that my test code is pretty simple, it uses WebTestClient API to establish test in a smooth way (ig it's a good practice as mentioned in spring docs) :

create methods or classes for this so your test can be as simple as for example assertEquals(sendEval("2+2"), new JShellResult(...))

~~I'm not certain i understand this sorry, can u elaborate ?~~

…dditional checks

JShellAPI/README.MD

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

JShellWrapper/src/test/java/JShellWrapperTest.java

JShellAPI/src/main/resources/application.yaml

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

JShellWrapper/src/test/java/JShellWrapperTest.java

JShellAPI/src/main/java/org/togetherjava/jshellapi/service/DockerService.java

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

JShellAPI/README.MD

Alathreon · 2024-09-25T16:14:38Z

JShellAPI/README.MD

-The rate at which the session killer will check and delete session, in seconds, see [Session timeout](#Session-timeout).
+The rate at which the session killer will check and delete session, in seconds, see [Session timeout](#Session-timeout).


huh i dunno what's this, i'll revert

Alathreon · 2024-09-25T16:16:02Z

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

+public final class Utils {
+    private Utils() {}
+
+    public static boolean validateJShellWrapperImageName(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);
+    }
+}


Move this to a static private method in Config, since it's only used once and should only be handled by Config.

Alathreon · 2024-09-25T16:22:19Z

JShellAPI/src/main/java/org/togetherjava/jshellapi/service/DockerService.java

+        String[] imageNameParts =
+                this.jshellWrapperImageName.split(Config.JSHELL_WRAPPER_IMAGE_NAME_TAG);
+
+        String baseImageName = imageNameParts[0];


Do this in the constructor.

JShellAPI/src/main/resources/application.yaml

Alathreon · 2024-09-25T16:25:59Z

README.md

+## Testing
+**JShellWrapper** Docker image lifecycle is handled during tests.
+
+The image name is injected from the root [build.gradle](./build.gradle) file, to JShellAPI project's [build.gradle](./JShellAPI/build.gradle) file and also to its [application.yaml](./JShellAPI/src/main/resources/application.yaml).
+
+- 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.


How many times I have to repeat this, readme isn't documentation nor user manual, you are supposed to tell the user how to run the tests, not how the project works.

Alathreon · 2024-09-25T16:28:53Z

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

+    private WebTestClient webTestClient;
+
+    @Autowired
+    private Config appConfig;


How do you handle custom config ? Like if for one test, you need to use custom config ?

I thought about that, but I don't think that would be a good idea as it's an integration-test, and it must verify what exactly happens in real scenario (with the real config).

The problem is that the real world config has timers from the minute to the dozen of minutes, you do not want a test to last for 30 minute because you wanted to check if a session correctly disapear after 30 minutes...

~~in that case we may create a common config and separate config~~

firasrg · 2024-09-28T16:45:17Z

JShellAPI/README.MD

FYI, this change is due to spotlessApply task

[Testing][JShellAPI] Settings up gradle tasks for testing; ;

9dc586e

Note: this includes adding a 1st class for tests;

firasrg requested review from a team as code owners August 10, 2024 16:48

[Testing][JShellAPI] Setting first integration test for /eval endpoint;

c8747e1

Note: this includes changes and improvements in gradle.build files;

firasrg force-pushed the testing/30-add-integration-tests branch from 3502cb7 to c8747e1 Compare August 10, 2024 16:59

[Testing][JShellAPI] Synchronizing endpoint paths between controllers…

6ab35c4

… and test classes

firasrg changed the title ~~[JShellAPI][Testing] Setting up First Integration Test - Simple code snippet evaluation~~ [JShellAPI][Testing] Setting up First Integration Test - Simple code snippet evaluation scenario Aug 11, 2024

[Testing][JShellAPI] adding a second subtest for same endpoint with a…

cccd5f1

…dditional checks

firasrg force-pushed the testing/30-add-integration-tests branch from e7c2450 to cccd5f1 Compare September 13, 2024 20:29

[Testing][JShellAPI] write documentation about testing approach

3de55c6

Alathreon requested changes Sep 15, 2024

View reviewed changes

firasrg force-pushed the testing/30-add-integration-tests branch from ae5fb7c to 4f4ec50 Compare September 20, 2024 19:51

Alathreon requested changes Sep 20, 2024

View reviewed changes

firasrg force-pushed the testing/30-add-integration-tests branch 3 times, most recently from 362653c to 89a7c65 Compare September 24, 2024 19:23

firasrg mentioned this pull request Sep 24, 2024

Add functionnal tests to JShellAPI ? #30

Open

Alathreon requested changes Sep 25, 2024

View reviewed changes

firasrg force-pushed the testing/30-add-integration-tests branch from 89a7c65 to 1d77dfa Compare September 28, 2024 16:43

firasrg commented Sep 28, 2024

View reviewed changes

[Testing][JShellAPI] apply pr review fixes

8cc1485

firasrg force-pushed the testing/30-add-integration-tests branch from 1d77dfa to 8cc1485 Compare September 28, 2024 19:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JShellAPI][Testing] Setting up First Integration Test - Simple code snippet evaluation scenario #50

[JShellAPI][Testing] Setting up First Integration Test - Simple code snippet evaluation scenario #50

firasrg commented Aug 10, 2024

Alathreon commented Sep 10, 2024

firasrg commented Sep 12, 2024 •

edited

Loading

Alathreon Sep 25, 2024

firasrg Sep 25, 2024

Alathreon Sep 25, 2024

Alathreon Sep 25, 2024

Alathreon Sep 25, 2024

Alathreon Sep 25, 2024

firasrg Sep 25, 2024

Alathreon Sep 25, 2024

firasrg Sep 25, 2024 •

edited

Loading

firasrg Sep 28, 2024

		The rate at which the session killer will check and delete session, in seconds, see [Session timeout](#Session-timeout). No newline at end of file
		The rate at which the session killer will check and delete session, in seconds, see [Session timeout](#Session-timeout).

[JShellAPI][Testing] Setting up First Integration Test - Simple code snippet evaluation scenario #50

Are you sure you want to change the base?

[JShellAPI][Testing] Setting up First Integration Test - Simple code snippet evaluation scenario #50

Conversation

firasrg commented Aug 10, 2024

Pull-request

Changes

Description

Alathreon commented Sep 10, 2024

firasrg commented Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

firasrg Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

firasrg commented Sep 12, 2024 •

edited

Loading

firasrg Sep 25, 2024 •

edited

Loading