falconry / falcon

The no-magic web data plane API and microservices framework for Python developers, with a focus on reliability, correctness, and performance at scale.
https://falcon.readthedocs.io/en/stable/
Apache License 2.0
9.53k stars 945 forks source link

TestClient methods return Result, not _ResultBase #2209

Closed davetapley closed 9 months ago

davetapley commented 9 months ago

Fixes https://github.com/falconry/falcon/issues/2207

codecov[bot] commented 9 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (4910dd7) 100.00% compared to head (9c86b93) 100.00%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #2209 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 62 62 Lines 6880 6880 Branches 1099 1099 ========================================= Hits 6880 6880 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

CaselIT commented 9 months ago

Thanks, the CI has some failing tests, can you fix them too?

davetapley commented 9 months ago

@CaselIT this turned out to be more complicated because of:

https://github.com/falconry/falcon/blob/4910dd73ecd1b9c8cf6cae045b26ad432fa56128/falcon/testing/client.py#L838

That breaks my naive typing, because simulate_request could return StreamedResult.

I could change to -> Union[Result, StreamedResult], but that doesn't help for #2207 because StreamedResult doesn't implement text or json (but does explain why it was _ResultBase in the first place).


However: StreamedResult can only be returned from _simulate_request_asgi, so it would be nice (for my purposes) if I could narrow to just Result (since I'm using WSGI).

I thought I could hint my way around it by adding a TypeGuard to:

https://github.com/falconry/falcon/blob/4910dd73ecd1b9c8cf6cae045b26ad432fa56128/falcon/testing/client.py#L567-L568

But that doesn't work because an ASGIApp is a WSGIApp 😞

And that led me to realize that what I want on #2207 (text and json) isn't possible so long as:

  1. An ASGIApp is (i.e. subclasses) a WSGIApp, and:
  2. We want the simulate_get, etc. functions to work transparently with WSGI or ASGI applications, because:
  3. Liskov substitution principle

In light of that I'm going to close this and #2207, and open a new discussion to address the underlying issue.

CaselIT commented 9 months ago

let me try something

CaselIT commented 9 months ago

I can't seem to be able to push an update. I'll create a new PR