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 an xfailing test expecting HTTP500 for invalid handler return values #4574

Merged
merged 1 commit into from
Feb 14, 2020

Conversation

Fogapod
Copy link
Contributor

@Fogapod Fogapod commented Feb 13, 2020

This change documents a contract of disallowing non-response
return values in request handlers that currently has a bug and
is therefore marked as xfail.

Issue: #4572

Created test as @webknjaz suggested.

@Fogapod Fogapod requested a review from asvetlov as a code owner February 13, 2020 17:56
@Fogapod
Copy link
Contributor Author

Fogapod commented Feb 13, 2020

Should test for returning HTTPException also be included? I'm not sure how it should be checked because it doesn't work at all currently.

@codecov-io
Copy link

codecov-io commented Feb 13, 2020

Codecov Report

Merging #4574 into master will increase coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4574      +/-   ##
==========================================
+ Coverage   97.53%   97.57%   +0.04%     
==========================================
  Files          43       43              
  Lines        8907     8907              
  Branches     1404     1404              
==========================================
+ Hits         8687     8691       +4     
+ Misses         98       96       -2     
+ Partials      122      120       -2     
Impacted Files Coverage Δ
aiohttp/web_runner.py 97.78% <0.00%> (ø) ⬆️
aiohttp/tcp_helpers.py 93.33% <0.00%> (ø) ⬆️
aiohttp/pytest_plugin.py 97.43% <0.00%> (+1.92%) ⬆️
aiohttp/connector.py 96.28% <0.00%> (ø) ⬆️
aiohttp/test_utils.py 99.68% <0.00%> (ø) ⬆️
aiohttp/web_protocol.py 92.69% <0.00%> (+0.29%) ⬆️

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 f80029b...75b4fa6. Read the comment docs.

client = await aiohttp_client(app)

async with client.get('/1') as resp:
assert 500 == resp.status
Copy link
Member

Choose a reason for hiding this comment

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

I'd also check if the text includes something like "Internal Server Error".

@webknjaz webknjaz changed the title Add failing test for #4572 Add an xfailing test expecting HTTP 500 for invalid handler return values Feb 13, 2020
@webknjaz webknjaz changed the title Add an xfailing test expecting HTTP 500 for invalid handler return values Add an xfailing test expecting HTTP500 for invalid handler return values Feb 14, 2020
@webknjaz webknjaz merged commit 1740c99 into aio-libs:master Feb 14, 2020
@webknjaz
Copy link
Member

Should test for returning HTTPException also be included?

Feel free to add a separate PR.

v3.6 deprecates returning exceptions but it still works (see the red warning box): https://docs.aiohttp.org/en/stable/web_quickstart.html#exceptions

master/v4.0 doesn't support this anymore and doesn't mention returning exceptions: https://docs.aiohttp.org/en/latest/web_exceptions.html

Based on this, when returning errors under 4.0/master, check for HTTP 500 Internal Server Error.

@Fogapod Fogapod deleted the patch-1 branch February 14, 2020 06:59
asvetlov pushed a commit that referenced this pull request Oct 16, 2020
PR #4574 by @Fogapod

This change documents a contract of disallowing non-response
return values in request handlers that currently has a bug and
is therefore marked as xfail.

Ref: #4572
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.

3 participants