split integration and unit tests #31

niemannd · 2020-10-03T18:56:04Z

Well, the title says all. This splits the tests into unit tests (run on every build) and integration tests.
This should allow us to develop and run basic unit tests without the need for a meilisearch instance.

i'm not yet satisfied with that solution, maybe remove the test Classpaths.

/discuss

eskombro · 2020-10-05T09:22:03Z

Hello @niemannd ! Thanks for opening the discussion!

I'm not sure I understand something, when you say

This should allow us to develop and run basic unit tests without the need for a meilisearch instance

do you plan to use a mock server response for testing?

In the rest of SDKs development, we often use a MeiliSearch running instance for testing, and there's no separation between unit tests and integration tests, they are all the same. I think the process is fast and quite simple. I personally prefer to use a docker image for testing and the process is quite simple. When I'm doing a PR, I'm quite sure tests will succeed, as I did run the same tests locally just before.

So I'd be really happy to read what this approach will add for development / contribution from your point of view :)

niemannd · 2020-10-05T12:25:20Z

do you plan to use a mock server response for testing?

I plan to mock class dependencies in unit tests.
This way we dont write classes that only work with a specific implementation of a dependency and rather just need a specific behaviour.
Example: We want to test the Documents class. This class uses the MeiliSearchHttpRequest class. In our unit test we dont want to test the MeiliSearchHttpRequest class, just the behaviour. This behaviour is "method get(...) returns the response body as string".
This way the mock would just need to return a string we define, nothing more. Makes testing specific cases much easier.

Unit tests basically describe what behaviour we expect from our class dependencies. I dont need to rely on 100% bug-free implementations of class dependencies. I dont need to rely on a live instance of meilisearch. Integration tests exist to test that our defined behaviour matches the real behaviour.

A side effect of mocking most class dependencies is that your code becomes more modular because you have to design your classes to allow mocking. Look at the Documents class for example. To unit test this class, i would have to mock the MeiliSearchHttpRequest class. The way the class is written this is currently not possible.

eskombro · 2020-10-06T02:33:52Z

Thanks for the explanation @niemannd !

This would be really interesting, as you point out, for making tests more atomic and independent, and can make development easier, even if at the same time I think that it also introduces more work as the mocked behaviors sometimes need to be maintained/updated. But it seems reasonable indeed!

It is really important, though, that we have a reliable integration set to check that everything is running smoothly for the whole SDK on the CI at integration time.

Great! :)

eskombro · 2020-10-06T08:09:00Z

I'm working on an integration tests PR #32, running docker on the CI. I'll try to merge it very soon, is it ok if you rebase your branch on it and do the test split?

niemannd · 2020-10-06T08:29:58Z

sure

niemannd · 2020-10-11T09:00:02Z

Regarding my last commit: I reworked the integration test setup a little bit, now using Tag Annotations provided by JUnit to seperate integration tests from the unit tests. This way we dont need a second SourceSet.

I also learned that i shouldn't use the at symbol in commit messages. Sorry Tom :)

eskombro

Thanks for this PR! 🎉

Added a few comments/questions, but in the overall, it improves the testing flow and content, so quite glad to see this contribution!

eskombro · 2020-10-12T12:48:46Z

.github/workflows/gradle.yml

@@ -16,13 +16,13 @@ jobs:

        steps:
            -   uses: actions/checkout@v2
+            -   name: Docker setup
+                run: docker run -d -p 7700:7700 getmeili/meilisearch:latest ./meilisearch --no-analytics=true --no-sentry=true --master-key='masterKey'


Why --no-sentry=true? I wouldn't hurt if there is a bug report :)

As i dont know what exactly triggers a sentry report, with the possible testing of the error handling and with the aspect that github actions is active for every fork i disabled sentry. A hard bug shoud be visible from the integration tests, but if you want sentry i have no problem with activating it.

No, it's ok, I just wanted to know if there was a specific reason for you to disable it, but it's totally fine if it isn't used in this context.

eskombro · 2020-10-12T12:51:30Z

build.gradle

@@ -48,7 +56,18 @@ task buildJar(type: Jar) {
 }

 test {
-	useJUnitPlatform()
+	useJUnitPlatform {


Will this configuration ignore integration tests when you do a gradle check or gradle test for example, but explicitly run them on the CI as you added run: ./gradlew build integrationTest?

I like it 👍

This config will exclude every Test or Class that is annotated with [at]Tag("integration")
This allows us to also specify other tags, for example for extrem slow tests.

eskombro · 2020-10-12T13:00:04Z

src/test/java/com/meilisearch/sdk/SearchRequestTest.java

+
+	@Test
+	void getQuery() {
+		SearchRequest classToTest = new SearchRequest("This is a Test");


I don't think that the default values should be handled by the SDK. I think that ideally, the query should be built only with the values that the user specifies, so in this case, a query "This is a test" shouldn't create the query ?q=This%20is%20a%20Test&offset=0&limit=20&attributesToRetrieve=*&cropLength=200&matches=false but just ?q=This%20is%20a%20Test and let the rest be handled by MeiliSearch itself. What do you think?

I know this is not an issue form these tests, but rather the SDK itself 👍 Just would like to have your opinion on it

I just included this test as an example for a unit test :) The unit test folder looked a bit empty :D

Indeed, quite a bit empty haha

eskombro mentioned this pull request Oct 6, 2020

Add integration tests, Docker on CI, and implement waitForPendingUpdate #32

Merged

niemannd added 4 commits October 9, 2020 14:42

add sourceSet for integration tests

73aef6c

Update integration-test.gradle

0776457

move integration tests to the integration-tests SourceSet

4c76d01

remove double docker container start

f411033

niemannd force-pushed the feat/integration-tests branch from f3cbaaf to f411033 Compare October 9, 2020 12:42

niemannd added 2 commits October 11, 2020 07:43

refactor SearchRequest

26348f9

move Movie class to test, so we can use it in unit tests

8e362a0

niemannd marked this pull request as ready for review October 11, 2020 06:29

rework integration testing by using JUnit5 @tag annotations

97213cb

eskombro approved these changes Oct 12, 2020

View reviewed changes

eskombro merged commit 720e0d1 into meilisearch:master Oct 12, 2020

niemannd deleted the feat/integration-tests branch February 5, 2021 18:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

split integration and unit tests #31

split integration and unit tests #31

niemannd commented Oct 3, 2020

eskombro commented Oct 5, 2020 •

edited

Loading

niemannd commented Oct 5, 2020

eskombro commented Oct 6, 2020 •

edited

Loading

eskombro commented Oct 6, 2020 •

edited

Loading

niemannd commented Oct 6, 2020

niemannd commented Oct 11, 2020

eskombro left a comment •

edited

Loading

eskombro Oct 12, 2020

niemannd Oct 12, 2020

eskombro Oct 12, 2020

eskombro Oct 12, 2020 •

edited

Loading

niemannd Oct 12, 2020

eskombro Oct 12, 2020

niemannd Oct 12, 2020

eskombro Oct 12, 2020

split integration and unit tests #31

split integration and unit tests #31

Conversation

niemannd commented Oct 3, 2020

eskombro commented Oct 5, 2020 • edited Loading

niemannd commented Oct 5, 2020

eskombro commented Oct 6, 2020 • edited Loading

eskombro commented Oct 6, 2020 • edited Loading

niemannd commented Oct 6, 2020

niemannd commented Oct 11, 2020

eskombro left a comment • edited Loading

Choose a reason for hiding this comment

eskombro Oct 12, 2020

Choose a reason for hiding this comment

niemannd Oct 12, 2020

Choose a reason for hiding this comment

eskombro Oct 12, 2020

Choose a reason for hiding this comment

eskombro Oct 12, 2020 • edited Loading

Choose a reason for hiding this comment

niemannd Oct 12, 2020

Choose a reason for hiding this comment

eskombro Oct 12, 2020

Choose a reason for hiding this comment

niemannd Oct 12, 2020

Choose a reason for hiding this comment

eskombro Oct 12, 2020

Choose a reason for hiding this comment

eskombro commented Oct 5, 2020 •

edited

Loading

eskombro commented Oct 6, 2020 •

edited

Loading

eskombro commented Oct 6, 2020 •

edited

Loading

eskombro left a comment •

edited

Loading

eskombro Oct 12, 2020 •

edited

Loading