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

Remove test client #2009

Merged
merged 6 commits into from
Jan 28, 2021
Merged

Remove test client #2009

merged 6 commits into from
Jan 28, 2021

Conversation

ahopkins
Copy link
Member

This is a more recent implementation of #1850. This is to take out the testing module (and httpx dependency) in favor of sanic-testing.

Still some broken tests to fix.

@ahopkins ahopkins marked this pull request as draft January 19, 2021 13:56
@ahopkins ahopkins marked this pull request as draft January 19, 2021 13:56
Copy link
Member

@ashleysommer ashleysommer left a comment

Choose a reason for hiding this comment

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

Looking great so far.

@@ -8,10 +8,11 @@
import httpx
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove httpx from here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe at some point, but I don't think we are there yet. We really need a new way to test timeouts. I think I have an idea, but there are higher priorities that are on my list to accomplish before the next release.

With that said, if anyone wants to take on these tests, I would be happy to help.

Copy link
Member

Choose a reason for hiding this comment

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

I don't see a problem in using third-party requirements on our testing, since they are actually required in our own testing package, unless we remove httpx from sanic_testing in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

I meant that I really don't like the tests themselves that require us to hack httpx here. I have no intention of removing httpx from the testing client because the ASGI reach in is awesome.

@ahopkins
Copy link
Member Author

ahopkins commented Jan 20, 2021

Yes. There are two tests that are failing and I'm not really sure why. The clients are nearly identical. It seems when we do a websocket call on the test_client, the server doesn't completely shutdown. I'm trying to see if it is a problem with the test, the client, or the websockets implementation. 🤔

I created a third client in my local reo that opens the server in a subprocess instead of using listeners. I can get the test passing with that.

@codecov
Copy link

codecov bot commented Jan 25, 2021

Codecov Report

Merging #2009 (c32e7fd) into master (976a4c7) will decrease coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2009      +/-   ##
==========================================
- Coverage   91.52%   91.48%   -0.05%     
==========================================
  Files          29       28       -1     
  Lines        3341     3182     -159     
  Branches      585      573      -12     
==========================================
- Hits         3058     2911     -147     
+ Misses        201      192       -9     
+ Partials       82       79       -3     
Impacted Files Coverage Δ
sanic/app.py 95.02% <100.00%> (+0.09%) ⬆️

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 976a4c7...c32e7fd. Read the comment docs.

@ahopkins ahopkins marked this pull request as ready for review January 25, 2021 00:44
Copy link
Member

@ashleysommer ashleysommer left a comment

Choose a reason for hiding this comment

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

I'm happy for this to be merged.

Copy link
Member

@ashleysommer ashleysommer left a comment

Choose a reason for hiding this comment

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

Should add these too?

return SanicTestClient(self)
if self._test_client:
return self._test_client
from sanic_testing.testing import SanicTestClient # type: ignore
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
from sanic_testing.testing import SanicTestClient # type: ignore
elif hasattr(self, "_test_manager"):
return self._test_manager.test_client # type: ignore
from sanic_testing.testing import SanicTestClient # type: ignore

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's add that in another PR. I want to get this one merged.

return SanicASGITestClient(self)
if self._asgi_client:
return self._asgi_client
from sanic_testing.testing import SanicASGITestClient # type: ignore
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
from sanic_testing.testing import SanicASGITestClient # type: ignore
elif hasattr(self, "_test_manager"):
return self._test_manager.asgi_client # type: ignore
from sanic_testing.testing import SanicASGITestClient # type: ignore

@ahopkins ahopkins merged commit 5545264 into master Jan 28, 2021
@ahopkins ahopkins deleted the sanic-testing branch January 28, 2021 07:22
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