Add more tests #149
Add more tests #149
Conversation
Codecov Report
@@ Coverage Diff @@
## master #149 +/- ##
==========================================
+ Coverage 86.27% 89.43% +3.15%
==========================================
Files 7 8 +1
Lines 430 549 +119
Branches 0 109 +109
==========================================
+ Hits 371 491 +120
+ Misses 59 37 -22
- Partials 0 21 +21
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
I added tests for nearly any configuration of |
|
I really like this initiative to add more "robustness" to asyncio-mqtt via tests. 👍 Great job on that. 😃 I don't have enough time tonight to give the proper review that this effort deserves (I already wrote a lengthy reply to another PR). I'll do a review later this weekend when I have the time. :) Sorry about the lack of time on my part. It's been a busy week at work. I'll try to make up for it this weekend. ~Frederik |
There was a problem hiding this comment.
LGTM. 👍 Well done! 😄
I put some minor remarks in the comments (mostly pointing to future API decisions). I don't have any requests for changes.
Really great to see our test coverage increase like this. I especially like how you exercised some of the subtle details in our API (the "backpressure" semantics). I think that most people don't realize how important that aspect actually is. Not before it's too late at least! 😄 See some of the early issues for reference.
|
|
||
| @pytest.fixture | ||
| def anyio_backend() -> Tuple[str, Dict[str, Any]]: | ||
| def anyio_backend() -> tuple[str, dict[str, Any]]: | ||
| if sys.platform == "win32": | ||
| from asyncio.windows_events import WindowsSelectorEventLoopPolicy |
There was a problem hiding this comment.
Maybe we should do something similar to this in the regular lib as well. We get a lot of issues from Windows users due to this.
| assert logger == client._client._logger # type: ignore[attr-defined] | ||
|
|
||
|
|
||
| async def test_client_max_concurrent_outgoing_calls( |
There was a problem hiding this comment.
Now this is an interesting test! Well done. 👍
I wonder how many people actually use this feature in production. 😄
This also showcases a weakness in our current API: We don't have any backpressure mechanics. That is difficult to solve "right", though. Definitely a 2.0 kind of thing (or with the anyio rewrite/replacement).
If you're interested in that kind of stuff there is a good discussion here: python-trio/trio#987
There was a problem hiding this comment.
Is not max_concurrent_outgoing_calls a backpressure mechanism?
There was a problem hiding this comment.
It is but it is a very crude one at that. 😄 My point is that we can and should do better in a future API.
|
I added a test that there is no warning for pending calls when |
No description provided.