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(WebSocket): properly handle HTTPStatus and HTTPError cases #2150

Merged
merged 9 commits into from
Jun 3, 2023

Conversation

CaselIT
Copy link
Member

@CaselIT CaselIT commented May 15, 2023

Can you think of further tests to try?

Fixes #2146

@CaselIT CaselIT requested review from vytas7 and kgriffs May 15, 2023 21:08
@codecov
Copy link

codecov bot commented May 15, 2023

Codecov Report

Merging #2150 (6a2ceba) into master (b16b6c3) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #2150   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           62        62           
  Lines         6791      6802   +11     
  Branches      1095      1097    +2     
=========================================
+ Hits          6791      6802   +11     
Impacted Files Coverage Δ
falcon/asgi/app.py 100.00% <100.00%> (ø)
falcon/asgi/ws.py 100.00% <100.00%> (ø)
falcon/util/misc.py 100.00% <100.00%> (ø)

Copy link
Member

@vytas7 vytas7 left a comment

Choose a reason for hiding this comment

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

LGTM, I like that the proposed tests are quite comprehensive 👍

This just needs 2146.bugfix.rst newsfragment to be added and I can approve.

@CaselIT
Copy link
Member Author

CaselIT commented May 25, 2023

thanks. I'll look into adding it and also by the coverage is not happy.

falcon/asgi/app.py Outdated Show resolved Hide resolved
@vytas7 vytas7 changed the title Properly handle HTTPStatus and HTTPError cases when using websockets fix(websocket): properly handle HTTPStatus and HTTPError cases Jun 3, 2023
@CaselIT
Copy link
Member Author

CaselIT commented Jun 3, 2023

thanks for fixing the coverage, I guess the old code was covering that part by chance

Copy link
Member

@vytas7 vytas7 left a comment

Choose a reason for hiding this comment

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

I took the liberty of rewording the newsfragment to sound more like a short story, and fixed Sphinx links therein.

LGTM now 💯

@vytas7 vytas7 changed the title fix(websocket): properly handle HTTPStatus and HTTPError cases fix(WebSocket): properly handle HTTPStatus and HTTPError cases Jun 3, 2023
@vytas7 vytas7 merged commit c7c790f into falconry:master Jun 3, 2023
@CaselIT
Copy link
Member Author

CaselIT commented Jun 3, 2023

Thanks!

@CaselIT CaselIT deleted the websocker_error_handler branch June 4, 2023 08:36
vytas7 added a commit that referenced this pull request Nov 12, 2023
* fix(websocket): properly handle HTTPStatus and HTTPError cases when using websockets

* test: fix failing tests

* test: add missing test coverage

* docs: add newsfragment

* chore: explicit raise for unsupported call of internal methods

* style: fix imports

* docs(towncrier): improve the bugfix newsfragment

---------

Co-authored-by: Vytautas Liuolia <vytautas.liuolia@gmail.com>
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.

Websocket handler fails in some cases of HTTPError/HTTPStatus
2 participants