NSLS-II / olog

Python client to the Olog
BSD 3-Clause "New" or "Revised" License
2 stars 4 forks source link

Need to consider a new test approach OR work with vcrpy #11

Closed ke-zhang-rd closed 4 years ago

ke-zhang-rd commented 4 years ago

we use package vcrpy to record http response for test.

However, vcrpy doesn't support httpx. https://vcrpy.readthedocs.io/en/latest/installation.html#compatibility

httpx support PR haven't merged. https://github.com/kevin1024/vcrpy/pull/523

ke-zhang-rd commented 4 years ago

@danielballan @mrakitin I test this PR with httpx. Looks it's stable, meaning,

Are there more features we need to check with this PR?

Also the strategy of test looks:

  1. hardcode EXPECT response in test_olog.py
  2. get REAL from cassettes which was record by vcrpy, and no network needed
  3. assert EXPERT == REAL

am I right?

mrakitin commented 4 years ago

Sounds right to me.

mrakitin commented 4 years ago

My reply was to @ke-zhang-rd's comment, but not the issue itself.

Regarding the issue subject, httpx seems to be a pretty young project, so it would be nice to preserve the use of vcrpy before the httpx/vcrpy integration is done/released.

How much work/replication of the code would it be to support 2 libraries - requests and httpx? Maybe some configuration and decorators could help to use the corresponding library based on an environment variable.

danielballan commented 4 years ago

httpx is young, but it has more community backing and weight behind it than most young projects because it is the result of careful consensus-building conversations within the Python web community about the right successor to requests.

The support for async methods in httpx, reflected in the async methods on our Client, align well with our other work and our general preference for cooperative, non-blocking APIs. That said, I think @mrakitin is probably right that having a "boring" option that uses the tried-and-true requests is a good idea. It would probably be a pretty simple copy/paste/find-and-replace operation. That goes against the DRY principle, but given that httpx is fundamentally async and requests is fundamentally sync, it's the right trade-off.

As to the implementation: an environment variable-based switch is a good tool when you have unavoidable global state that affects how the whole program runs---such as, "Do EpicsSignal instances connect using pyepics by default or caproto by default?" or "Do sparse arrays raise an exception when an operation would implicitly force them to become dense or do they silently explode their memory usage and become dense?" This is not that kind of situation. If we wanted to support both httpx and requests, I think the code to do so could coexisting peacefully in the same process. You could, for example, have pyolog2.requests.Client and pyolog2.httpx.Client. This would make it easier to test them and just generally reduce the complexity of using them.

stuartcampbell commented 4 years ago

I agree, requests is not the new shiny but it is well used and just works. It would be a shame to lose functionality just to use the latest thing. If we can have both, but make one the default so the user does not need to pick - I doubt most will not care which is used, but if both are there, the people who do understand the differences will have the choice.

ke-zhang-rd commented 4 years ago

@mrakitin @stuartcampbell @danielballan

Thanks for all comments and suggestions. My thinking is sincehttpx is depend on requests, no matter which one we choose or both, it's always requests does the dirty work. So I assume there isn't really difference in performance especially people in sync domain.

But yes because httpx and requests return almost exactly identical obj. Any code after response should be able to just copy and paste. Pytest almost 99% reusable. I could quickly implement it.

danielballan commented 4 years ago

My thinking is since httpx is depend on requests, no matter which one we choose or both, it's always requests does the dirty work.

What makes you say that httpx depends on requests? I can't find a requests dependency in its source code, and I can install and import httpx without installing requests:

$ python
Python 3.7.7 (default, May  7 2020, 21:25:33) 
[GCC 7.3.0] :: Anaconda, Inc. on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import httpx
>>> import requests
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ModuleNotFoundError: No module named 'requests'

Unless I'm very confused, httpx is a fully separate implementation.

So I assume there isn't really difference in performance especially people in sync domain.

The concern voiced by @mrakitin and echoed by @stuartcampbell isn't related to performance; it's related to reliability/maturity of the project.

ke-zhang-rd commented 4 years ago

I saw httpx.request somewhere in their code but I was thinking httpx.requests in my my mind. Right, it's new one.

danielballan commented 4 years ago

I think this is ready to close. The VCR approach is sound, and we will track the development of a requests-based variant, acting on @mrakitin's suggestion, in #18. As noted in that issue, the development should proceed once the httpx implementation is fully worked out.

mrakitin commented 4 years ago

Thanks for the thoughts, everyone. I agree with closing this issue.