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

Support for fragmented Messages #103

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

A-J-Bauer
Copy link

Added support for fragmented messages as described in RFC 6455 - Page 27 - 5.4.

@codecov
Copy link

codecov bot commented Dec 5, 2018

Codecov Report

Merging #103 into master will decrease coverage by 0.87%.
The diff coverage is 19.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #103      +/-   ##
==========================================
- Coverage   39.23%   38.36%   -0.88%     
==========================================
  Files          41       41              
  Lines        2039     2075      +36     
==========================================
- Hits          800      796       -4     
- Misses       1239     1279      +40
Impacted Files Coverage Δ
src/main/c/seasocks/Connection.h 23.07% <ø> (ø) ⬆️
src/main/c/Connection.cpp 21.88% <0%> (-0.41%) ⬇️
src/main/c/internal/HybiPacketDecoder.h 100% <100%> (ø) ⬆️
src/main/c/HybiPacketDecoder.cpp 55.69% <26.92%> (-28.79%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb54d16...a801e02. Read the comment docs.

@codecov
Copy link

codecov bot commented Dec 5, 2018

Codecov Report

Merging #103 into master will decrease coverage by 0.45%.
The diff coverage is 19.56%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #103      +/-   ##
==========================================
- Coverage   39.23%   38.78%   -0.46%     
==========================================
  Files          41       41              
  Lines        2039     2073      +34     
==========================================
+ Hits          800      804       +4     
- Misses       1239     1269      +30
Impacted Files Coverage Δ
src/main/c/seasocks/Connection.h 23.07% <ø> (ø) ⬆️
src/main/c/Connection.cpp 21.88% <0%> (-0.41%) ⬇️
src/main/c/internal/HybiPacketDecoder.h 100% <100%> (ø) ⬆️
src/main/c/HybiPacketDecoder.cpp 67.53% <28%> (-16.96%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb54d16...7b7e0f2. Read the comment docs.

@A-J-Bauer A-J-Bauer closed this Dec 5, 2018
@A-J-Bauer A-J-Bauer reopened this Dec 5, 2018
@offa
Copy link
Collaborator

offa commented Dec 5, 2018

The CI build doesn't tell much why the tests fail. We should improve this (though that's not part of this PR).

Update: Issue opened #104

@offa offa self-requested a review December 5, 2018 12:52
@A-J-Bauer
Copy link
Author

Guess the check now fails because Valgrind complains. It would be helpful if the memory logs would be accessible through a link. I will spend some more time using Valgrind locally and if I don't find anything that can be fixed in a reasonable amount of time I have to leave it alone.

Copy link
Collaborator

@offa offa left a 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 so far! 👍

What's open:

  • Fix the CI build (first we have to figure out, why it's broken …)
  • Probably improve the commit messages (simple git operation, not much to do though)
  • Avoid coverage degradation and get some tests in

.gitignore Outdated Show resolved Hide resolved
@offa
Copy link
Collaborator

offa commented Dec 5, 2018

Guess the check now fails because Valgrind complains. It would be helpful if the memory logs would be accessible through a link.

Indeed. For now, you can run the ctest commands from the .travis.yml locally.

@offa
Copy link
Collaborator

offa commented Dec 5, 2018

Hmm … running the unit tests locally (via AllTests) showed up some failures:

test cases:  61 |  53 passed |  8 failed
assertions: 198 | 145 passed | 53 failed

ctest -D ExperimentalBuild -j${JOBS} (CI and locally) doesn't show those; ctest without further arguments fails too.

@offa
Copy link
Collaborator

offa commented Dec 5, 2018

@A-J-Bauer I've improved the test execution on Travis. The tests are now executed correctly and show what's wrong on failure. Please rebase onto master to get your PR updated.

@A-J-Bauer A-J-Bauer closed this Dec 5, 2018
@A-J-Bauer A-J-Bauer reopened this Dec 5, 2018
@A-J-Bauer
Copy link
Author

A-J-Bauer commented Dec 5, 2018

@offa Thank you for the improved test execution, pointed me in the right direction. I removed a check for unmasked client messages which broke tests. Now travis seems to be happy. Codecov still isn't.

@offa
Copy link
Collaborator

offa commented Dec 6, 2018

@offa Thank you for the improved test execution, pointed me in the right direction. I removed a check for unmasked client messages which broke tests. Now travis seems to be happy.

👍 You can rebase onto the updated master, so you get an improved output when updating your PR here.

Codecov still isn't.

The newly added code isn't covered by tests. Please visit the codecov link, it already provides a useful overview of uncovered lines – no need to struggle with building coverage data locally.

The Hybi related tests are all in HybiTests.cpp; connection related in ConnectionTests.cpp.

Please let me know, if you need any help here.

@offa offa self-assigned this Dec 6, 2018
@offa offa removed their assignment Jan 24, 2020
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.

2 participants