-
Notifications
You must be signed in to change notification settings - Fork 6
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
32 feature handling more than one connection #33
Conversation
…aring and starting server.
…CopyOnWriteArrayList
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good if you remove the while-loop in clienthandler. It might have conflicts with #24 though?
This PR might have to make some changes after #24 is done and merged. |
…ature-handling-more-than-one-connection
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The client gets disconnected after connecting because isValidConnection runs every loop and returns false after first loop, closing the socket. Other than that the code looks good.
… a connect message and yet have a valid connection if package does not contain connect message.
@kappsegla Do I add the tests in this PR to cover SonarCloud expectations, or do I create an issue that creates the tests for all the old and new classes before merging this? |
If we can write unit tests for the code they belong to the PR. It's a needed part both for our automatic CI but also for reviewers to check and learn how the new code is expected to work from. |
Quality Gate failedFailed conditions 51.4% Coverage on New Code (required ≥ 80%) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good and it works when I test it. Sonarclouds Quality Gate failed though, is this something we can ignore right now and merge anyway? Maybe something to look at in another issue?
I will approve right now since I want it to be able to merge if we can ignore the Qualite Gate.
The sonarcloud reacts because no old code has tests. So I also think that would be issued to fix, but ignored for now 😄 . |
Related Issue
Closes #32
Proposed Changes
Start a new thread when handling client connections