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

Add more websocket connection tests and fix bugs #1085

Merged
merged 2 commits into from
Nov 23, 2022

Conversation

blink1073
Copy link
Contributor

This exercise found a few bugs in ZMQChannelsWebsocketConnection.
Also, it seems odd to me that handle_outgoing_message is defined on the abc but isn't used anywhere. Was that an oversight @Zsailer?

@codecov
Copy link

codecov bot commented Nov 23, 2022

Codecov Report

Attention: Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 76.72%. Comparing base (890b882) to head (58747cc).
Report is 278 commits behind head on main.

Files Patch % Lines
...ter_server/services/kernels/connection/channels.py 75.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1085      +/-   ##
==========================================
- Coverage   77.35%   76.72%   -0.64%     
==========================================
  Files          68       68              
  Lines        8299     8301       +2     
  Branches     1649     1650       +1     
==========================================
- Hits         6420     6369      -51     
- Misses       1462     1501      +39     
- Partials      417      431      +14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Zsailer
Copy link
Member

Zsailer commented Nov 23, 2022

Thanks @blink1073!

Also, it seems odd to me that handle_outgoing_message is defined on the abc but isn't used anywhere. Was that an oversight @Zsailer?

And yes, this was an oversight. I've opened #1089 to fix this.

@Zsailer
Copy link
Member

Zsailer commented Nov 23, 2022

Actually let's merge #1089 first, then rebase this PR to make sure the tests pass okay.

@blink1073
Copy link
Contributor Author

I'll cut another RC once this is merged.

@blink1073 blink1073 enabled auto-merge (squash) November 23, 2022 17:11
@blink1073 blink1073 merged commit 339038b into jupyter-server:main Nov 23, 2022
@blink1073 blink1073 deleted the channels-coverage branch November 23, 2022 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants