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

fix!: WebSocketApp.run_forever() returning None after close() is called #788

Merged

Conversation

fujiapple852
Copy link
Contributor

Closes #785

BREAKING CHANGE: the return value of WebSocketApp.run_forever() will no longer return the (undocumented) None value.

…s called

BREAKING CHANGE: the return value of `WebSocketApp.run_forever()` will no longer return the (undocumented) `None` value.
@engn33r
Copy link
Collaborator

engn33r commented Feb 11, 2022

The CI output shows this breaks two tests in websocket/tests/test_websocket.py: the testIter test and the testNext test. It could be because the tests are poorly written, but I don't have time to investigate the exact cause right now.

…h `wss://api.bitfinex.com/ws/2` in several tests
@fujiapple852
Copy link
Contributor Author

@engn33r it looks like wss://stream.meetup.com/2/rsvps no longer exists (returns 404). I replaced this with wss://api.bitfinex.com/ws/2 in these two tests (and one other, which wasn't strictly necessary) which allows them to pass locally.

@fujiapple852
Copy link
Contributor Author

In passing, I spotted that testRunForeverDispatcher passes but triggers a (silently ignored) error because of a seemingly bogus call to WebSocketApp.recv(). I haven't pushed a change for this as there are a few other oddities with this test and testKeepRunning that I don't understand so is probably worth a separate issue.

@fujiapple852
Copy link
Contributor Author

I added two new test cases to cover the graceful and exceptional shutdown of run_forever(). They are both triggered by a timer so could be racy (0.2 secs for now)

@codecov
Copy link

codecov bot commented Feb 11, 2022

Codecov Report

Merging #788 (93df810) into master (f719d0a) will increase coverage by 0.08%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #788      +/-   ##
==========================================
+ Coverage   84.45%   84.54%   +0.08%     
==========================================
  Files          13       13              
  Lines        1293     1294       +1     
  Branches      275      275              
==========================================
+ Hits         1092     1094       +2     
+ Misses        129      128       -1     
  Partials       72       72              
Impacted Files Coverage Δ
websocket/_app.py 85.02% <100.00%> (+3.84%) ⬆️
websocket/_abnf.py 89.82% <0.00%> (-2.66%) ⬇️

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 f719d0a...93df810. Read the comment docs.

@engn33r engn33r merged commit 23df64f into websocket-client:master Feb 25, 2022
@engn33r
Copy link
Collaborator

engn33r commented Feb 25, 2022

Thanks for the PR, and for fixing the broken WebSocket endpoint URLs!

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.

Clarify documentation for teardown return from run_forever()
2 participants