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

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

Merged

Conversation

firasrg
Copy link
Member

@firasrg firasrg commented Aug 10, 2024

Pull-request

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;
@firasrg firasrg requested review from a team as code owners August 10, 2024 16:48
Note: this includes changes and improvements in gradle.build files;
@firasrg firasrg force-pushed the testing/30-add-integration-tests branch from 3502cb7 to c8747e1 Compare August 10, 2024 16:59
@firasrg 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
@Alathreon
Copy link
Contributor

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
Copy link
Member Author

firasrg commented Sep 12, 2024

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 ?

@firasrg firasrg force-pushed the testing/30-add-integration-tests branch from e7c2450 to cccd5f1 Compare September 13, 2024 20:29
JShellAPI/README.MD Outdated Show resolved Hide resolved
JShellAPI/README.MD Outdated Show resolved Hide resolved
JShellAPI/README.MD Outdated Show resolved Hide resolved
JShellAPI/README.MD Outdated Show resolved Hide resolved
JShellAPI/README.MD Outdated Show resolved Hide resolved
@firasrg firasrg force-pushed the testing/30-add-integration-tests branch from ae5fb7c to 4f4ec50 Compare September 20, 2024 19:51
JShellAPI/src/main/resources/application.yaml Outdated Show resolved Hide resolved
JShellAPI/README.MD Outdated Show resolved Hide resolved
JShellAPI/README.MD Outdated Show resolved Hide resolved
@firasrg firasrg force-pushed the testing/30-add-integration-tests branch 3 times, most recently from 362653c to 89a7c65 Compare September 24, 2024 19:23
JShellAPI/README.MD Outdated Show resolved Hide resolved
JShellAPI/src/main/resources/application.yaml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@firasrg firasrg force-pushed the testing/30-add-integration-tests branch from 89a7c65 to 1d77dfa Compare September 28, 2024 16:43
@firasrg firasrg force-pushed the testing/30-add-integration-tests branch from 1d77dfa to 8cc1485 Compare September 28, 2024 19:39
@Alathreon Alathreon merged commit 3969809 into Together-Java:develop Oct 4, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add functionnal tests to JShellAPI ?
3 participants