Skip to content

#122 Fix unit tests failed #137

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

Merged
merged 4 commits into from
Jun 1, 2022
Merged

#122 Fix unit tests failed #137

merged 4 commits into from
Jun 1, 2022

Conversation

TuanHungVU1202
Copy link
Collaborator

Fixes #122 with the suggested approach using CompletableFuture

@TuanHungVU1202 TuanHungVU1202 added the bug Something isn't working label May 24, 2022
@TuanHungVU1202 TuanHungVU1202 self-assigned this May 24, 2022
@TuanHungVU1202 TuanHungVU1202 deleted the KEVA-122 branch May 24, 2022 16:14
@tuhuynh27 tuhuynh27 restored the KEVA-122 branch May 24, 2022 21:24
@tuhuynh27 tuhuynh27 reopened this May 24, 2022
@tuhuynh27 tuhuynh27 marked this pull request as ready for review May 31, 2022 15:55
@tuhuynh27
Copy link
Member

@TuanHungVU1202 It's fine to let these codes sleep:

https://github.com/keva-dev/keva/blob/develop/core/src/test/java/dev/keva/core/server/AbstractServerTest.java#L196-L199

Because it's not about to wait for the server to start, it's about waiting for the command to execute

@tuhuynh27
Copy link
Member

@yampml @axblueblader @haphananhtuan Please help to review

Copy link
Member

@tuhuynh27 tuhuynh27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@taivtse
Copy link
Collaborator

taivtse commented May 31, 2022

Hi @TuanHungVU1202, based on your idea, I have a suggestion like the one below.
In KevaServer, we will have an await method like this:

    @Override
    public void await() {
        ready.join();    // your ready future
    }

In where we want to wait until the server is available (e.g.: AbstractServerTest), just add this:

server.await();

@tuhuynh27
Copy link
Member

@taivtse Nice idea, updated

@tuhuynh27 tuhuynh27 merged commit 3e2b12a into develop Jun 1, 2022
@tuhuynh27 tuhuynh27 deleted the KEVA-122 branch June 1, 2022 02:06
@tuhuynh27 tuhuynh27 linked an issue Jun 1, 2022 that may be closed by this pull request
@tuhuynh27 tuhuynh27 mentioned this pull request Jun 2, 2022
tuhuynh27 added a commit that referenced this pull request Jun 2, 2022
* fix unit tests failed to create socket

* update getReady method

* add await() method

Co-authored-by: Huynh, Tu | RASIA <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unit tests failed - Failed to create socket consistently
3 participants