Add tests for ASGI specification compliance #50

LucidDan · 2021-12-29T03:55:29Z

WIP: test suite for ASGI specification compliance.

It seems like a great idea for a ASGI test library to have tests itself
to check it is compliant with the spec, and if there are new versions
that come out, will make it easier to update to match the spec.

There are a number of tests that currently xfail in this suite -
one or two of those tests might be things that I'm wrong in my
interpretation of the spec, but I'm mostly pretty confident in them.

I've intentionally made these xfails rather than just fixing the code in
the same PR, so the idea would be to fix the issues one by one and
remove the xfail, or in cases where the issue is a WONTFIX, change
the test and/or indicate as such in the xfail reason.

There is still a need for a couple more tests in the websocket
section, and probably could do with some more type-oriented
negative tests (e.g. testing what happens if you send a message
with the wrong type in the event values, like with 'status').

Of particular note, the scope['path'] test (test_http:test_http_path_is_not_escaped)
I'm not entirely sure that test should exist, it might not be valid.
More explanation in the comments. This one is one of the main things
that makes me want to keep this PR draft a bit longer, both for comment
and while I decide what to do with that test.

There are a number of tests that currently xfail in this suite. The intention is to fix the issues one by one, or in cases where the issue is a WONTFIX, change the test and/or indicate as such in the xfail reason. There is still a need for a couple more tests in the websocket section, and probably could do with some more type-oriented negative tests (e.g. testing what happens if you send a message with the wrong type in the event values, like with 'status')

codecov-commenter · 2021-12-29T03:56:51Z

Codecov Report

Merging #50 (2037ec9) into master (0e92f98) will increase coverage by 1.33%.
The diff coverage is 95.00%.

@@            Coverage Diff             @@
##           master      #50      +/-   ##
==========================================
+ Coverage   87.53%   88.86%   +1.33%     
==========================================
  Files           6        7       +1     
  Lines         409      422      +13     
==========================================
+ Hits          358      375      +17     
+ Misses         51       47       -4

Impacted Files	Coverage Δ
async_asgi_testclient/websocket.py	`74.11% <ø> (ø)`
async_asgi_testclient/testing.py	`91.50% <92.85%> (+2.46%)`	⬆️
async_asgi_testclient/exceptions.py	`100.00% <100.00%> (ø)`
async_asgi_testclient/utils.py	`95.00% <0.00%> (+1.25%)`	⬆️

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 0e92f98...2037ec9. Read the comment docs.

Fixes test_http:test_response_body_can_be_missing()

Fixes test_http:test_response_headers_can_be_missing()

Fixes test_websocket : test_asgi_version_is_present_in_websocket_scope() and test_websocket_spec_version_is_missing_or_correct()

Fixes test_lifespan:test_lifespan_not_supported_is_allowed() This fix was somewhat challenging to achieve without breaking other things or backward compatibility issues. In the end, I think the most elegant solution is to adopt more Pythonic exceptions in the TestClient, as demonstrated in the commit. However, since this is a less trivial change, I have only used TestClientError in the lifespan related code, leaving all other code using raw Exceptions still. I think TestClientError (or possibly even multiple exception classes for different purposes) should actually be adopted throughout all the code, albeit with the caveat that it might cause some issues if anyone is doing really rigid exception assertions in their test code. For most cases (including my own code that is already using async_asgi_testclient) I believe the average 'with pytest.raises(Exception)' assertions should still work, since TestClientError is a sub-class of Exception. A side-benefit of the TestClientError is it would allow access to the original Message structure, instead of having to decode a string repr of the event message.

Removed test for contamination between calls with a custom scope. Probably deserves documentation because it could be a gotcha for people, but it doesn't deserve to be in this test suite.

LucidDan · 2021-12-29T12:11:27Z

The CI black is pretty old, and I can't get it to run on my computer at all (symbol not found in flat namespace '__PyUnicode_DecodeUnicodeEscape')...but the newer black that I could get to work doesn't match formatting...so I can't get this to pass. Ugh. :-( (fixed it eventually)

In any case...

I broke the commits out so it is easy to read through and see the fixes for each xfail test, but they don't necessarily need to be split out like that; might be better to just squash them when merging.

Had a few issues with formatting and the black version, this commit cleans that up.

LucidDan · 2021-12-29T12:35:10Z

This PR now includes fixes for a few of the most prominent issues I wrote tests for (read: the ones personally causing me immediate problems right now :-D).

The main one that deserves the most attention is the one that originally sent me down this rabbit hole - the lifespan protocol handling. When finding a solution for that, I considered a few approaches, but I ended up using a custom exception - I would actually recommend using TestClientError everywhere instead of the current use of raw Exceptions which is generally considered bad practice. The commit has some detailed notes on the thinking behind it.

masipcat · 2021-12-30T15:48:43Z

Hi @LucidDan, I've done a quick review and it looks really good! I'll do a proper review when I come back from the holiday in about two weeks. Let me know when this is no longer a WIP and it's ready for review.

LucidDan · 2021-12-30T19:49:35Z

Great!
Yeah it's no longer a WIP, I think there is a little more that could be done (I don't have any test for websocket send/receive) but otherwise it is in good shape.

async_asgi_testclient/testing.py

masipcat · 2022-01-17T11:39:22Z

async_asgi_testclient/tests/asgi_spec/conftest.py

+            if msg[-1]["type"] == "http.disconnect" or not msg[-1].get(
+                "more_body", False
+            ):


Should we check that if have not received a http.disconnect, it is a http.request?

Like:

Suggested change

if msg[-1]["type"] == "http.disconnect" or not msg[-1].get(

"more_body", False

):

if msg[-1]["type"] == "http.disconnect" or (msg[-1] == "http.request" and not msg[-1].get(

"more_body", False

)):

So we can catch unexpected events sent by asgi-testclient.

Yes this makes sense. That whole function could probably be rewritten to be more readable too, now that I look at it after a break. ;-)

async_asgi_testclient/tests/asgi_spec/test_http.py

masipcat · 2022-01-17T12:51:56Z

async_asgi_testclient/tests/asgi_spec/test_http.py

+
+
+# Ok, this one I'm not sure about...
+# The grey area here is TestClient doesn't currently decode an encoded URL; but should it?


I think the TestClient should encode the path internally, the same way we are doing for the query_string:

async-asgi-testclient/async_asgi_testclient/utils.py

Lines 49 to 53 in 56779ef

if query_string is None:

path, _, query_string_raw = path.partition("?")

query_string_raw = quote(query_string_raw, safe="&=")

else:

query_string_raw = urlencode(query_string, doseq=True)

If you want to change it and add/modify one of the integration tests to ensure the frameworks still work I think it's enough :)

Re: https://github.com/vinissimus/async-asgi-testclient/pull/50/files#r785889284 The asserts where we access the log message in the test is a bit rough, it could probably be refactored to be a little cleaner.

LucidDan · 2023-06-02T15:54:50Z

Been a long time, but I'm taking another look at this...still a couple of things to resolve but have addressed a few of the questions from the (Jan 2022!!) review.

LucidDan mentioned this pull request Dec 29, 2021

Issues with compliance to the ASGI specification #49

Open

LucidDan added 5 commits December 29, 2021 21:50

Allow http.response.body event with no 'body' key

b5f4d74

Fixes test_http:test_response_body_can_be_missing()

Allow http.response.start event with no 'headers' key

dbe96c4

Fixes test_http:test_response_headers_can_be_missing()

Ensure websocket events include 'asgi' in scope

f655b85

Fixes test_websocket : test_asgi_version_is_present_in_websocket_scope() and test_websocket_spec_version_is_missing_or_correct()

Remove test for custom scope

732b1e6

Removed test for contamination between calls with a custom scope. Probably deserves documentation because it could be a gotcha for people, but it doesn't deserve to be in this test suite.

LucidDan force-pushed the master branch 2 times, most recently from d1bfa57 to 797b876 Compare December 29, 2021 12:25

Fix formatting

2037ec9

Had a few issues with formatting and the black version, this commit cleans that up.

LucidDan force-pushed the master branch from 797b876 to 2037ec9 Compare December 29, 2021 12:27

LucidDan marked this pull request as ready for review December 29, 2021 12:30

masipcat self-requested a review December 30, 2021 15:47

masipcat reviewed Jan 17, 2022

View reviewed changes

async_asgi_testclient/testing.py Outdated Show resolved Hide resolved

masipcat reviewed Jan 17, 2022

View reviewed changes

async_asgi_testclient/tests/asgi_spec/test_http.py Outdated Show resolved Hide resolved

masipcat reviewed Jan 17, 2022

View reviewed changes

LucidDan added 3 commits June 2, 2023 12:40

Merge branch 'vinissimus:master' into master

b6f93cd

Notify the user if lifespan protocol raised an exception

3f79e78

Re: https://github.com/vinissimus/async-asgi-testclient/pull/50/files#r785889284 The asserts where we access the log message in the test is a bit rough, it could probably be refactored to be a little cleaner.

Fix method is uppercased in HTTP

cbb6100

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for ASGI specification compliance #50

Add tests for ASGI specification compliance #50

LucidDan commented Dec 29, 2021

codecov-commenter commented Dec 29, 2021 •

edited

LucidDan commented Dec 29, 2021 •

edited

LucidDan commented Dec 29, 2021

masipcat commented Dec 30, 2021

LucidDan commented Dec 30, 2021

masipcat Jan 17, 2022

LucidDan Jan 17, 2022

masipcat Jan 17, 2022

masipcat Jan 17, 2022

LucidDan commented Jun 2, 2023



		# Ok, this one I'm not sure about...
		# The grey area here is TestClient doesn't currently decode an encoded URL; but should it?

	if query_string is None:
	path, _, query_string_raw = path.partition("?")
	query_string_raw = quote(query_string_raw, safe="&=")
	else:
	query_string_raw = urlencode(query_string, doseq=True)

Add tests for ASGI specification compliance #50

Are you sure you want to change the base?

Add tests for ASGI specification compliance #50

Conversation

LucidDan commented Dec 29, 2021

codecov-commenter commented Dec 29, 2021 • edited

Codecov Report

LucidDan commented Dec 29, 2021 • edited

LucidDan commented Dec 29, 2021

masipcat commented Dec 30, 2021

LucidDan commented Dec 30, 2021

masipcat Jan 17, 2022

Choose a reason for hiding this comment

LucidDan Jan 17, 2022

Choose a reason for hiding this comment

masipcat Jan 17, 2022

Choose a reason for hiding this comment

masipcat Jan 17, 2022

Choose a reason for hiding this comment

LucidDan commented Jun 2, 2023

codecov-commenter commented Dec 29, 2021 •

edited

LucidDan commented Dec 29, 2021 •

edited