vinissimus / async-asgi-testclient

A framework-agnostic library for testing ASGI web applications
MIT License
160 stars 20 forks source link

Add tests for ASGI specification compliance #50

Open LucidDan opened 2 years ago

LucidDan commented 2 years ago

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.

codecov-commenter commented 2 years ago

Codecov Report

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

Impacted file tree graph

@@            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%) :arrow_up:
async_asgi_testclient/exceptions.py 100.00% <100.00%> (ø)
async_asgi_testclient/utils.py 95.00% <0.00%> (+1.25%) :arrow_up:

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.

LucidDan commented 2 years ago

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.

LucidDan commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.

LucidDan commented 1 year ago

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.