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

TestClient doesn't merge urls in the same way as the httpx client, and I think it should. #2306

Closed
Waghabond opened this issue Oct 18, 2023 Discussed in #2300 · 0 comments · Fixed by #2376
Closed

TestClient doesn't merge urls in the same way as the httpx client, and I think it should. #2306

Waghabond opened this issue Oct 18, 2023 Discussed in #2300 · 0 comments · Fixed by #2376
Labels
need confirmation This issue needs confirmation.

Comments

@Waghabond
Copy link

Waghabond commented Oct 18, 2023

Discussed in #2300

Originally posted by Waghabond October 12, 2023
The TestClient just delegates the url merging to the join method of the httpx.URL object.

This causes the base path configuration to be ignored in some cases.

eg.
This how the merge is done in the Starlette TestClient's request() method.

url = self.base_url.join(url)

It creates unintuitive behaviour in the following example (which i think is a common way to use the test client)

client = testclient.TestClient(app, base_url="http://testserver/api/v1/")

result = client.base_url.join("/this-should-append-onto-the-end-of-the-base-url")
# result in reality is URL('http://testserver/this-should-append-onto-the-end-of-the-base-url'), the /api/v1 has disappeared.
result = client.base_url.join("this-works-as-expected")
# result is URL('http://testserver/api/v1/this-works-as-expected'), 

the way that httpx performs this merge is via the following code (i've lifted it straight from the httpx source code):

def _merge_url(self, url: URLTypes) -> URL:
    """
    Merge a URL argument together with any 'base_url' on the client,
    to create the URL used for the outgoing request.
    """
    merge_url = URL(url)
    if merge_url.is_relative_url:
        # To merge URLs we always append to the base URL. To get this
        # behaviour correct we always ensure the base URL ends in a '/'
        # separator, and strip any leading '/' from the merge URL.
        #
        # So, eg...
        #
        # >>> client = Client(base_url="https://www.example.com/subpath")
        # >>> client.base_url
        # URL('https://www.example.com/subpath/')
        # >>> client.build_request("GET", "/path").url
        # URL('https://www.example.com/subpath/path')
        merge_raw_path = self.base_url.raw_path + merge_url.raw_path.lstrip(b"/")
        return self.base_url.copy_with(raw_path=merge_raw_path)
    return merge_url

Starlette should be doing this the same way.

Important

  • We're using Polar.sh so you can upvote and help fund this issue.
  • We receive the funding once the issue is completed & confirmed by you.
  • Thank you in advance for helping prioritize & fund our backlog.
Fund with Polar
@Kludex Kludex added the need confirmation This issue needs confirmation. label Oct 18, 2023
shabani1 added a commit to lexy-ai/lexy that referenced this issue Mar 1, 2024
# What

- Refactored `lexy-py` tests to use the new testing configuration 
  + Use test instances of DB, IndexManager, and Celery
- Upgraded FastAPI and Starlette
+ Resolves an [issue](encode/starlette#2306)
with Starlette's `TestClient` class
- Added ASGI lifespan manager
  + Used for launching and shutting down `IndexManager` during tests
- Updated test fixtures
  + Fixtures for `test_app`, `lx_client` and `lx_async_client`
  + `client` fixture is now function scoped
  + `seed_data` is now a requirement of the `test_app` fixture
- New packages
  + `asgi-lifespan` for testing

Remaining TODOs related to testing:

- Move the logic in `tutorial.ipynb` and `tests.ipynb` into the new
testing framework
- Add more tests for the server
  + Bindings, Indexes, Transformers
  
# Why

`lexy-py` tests were still using the main lexy database.


# Test plan

- [x] Run `make run-tests` and verify that all tests pass
- [x] Run `examples/tests.ipynb` and verify that there are no errors
- [x] Run `examples/tutorial.ipynb` and verify that the tutorial works
as expected
- [ ] Run `examples/images.ipynb` and verify that the tutorial works as
expected

# Misc

Contains new packages, so make sure to rebuild dev containers after
pulling.

```shell
make update-dev-containers
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need confirmation This issue needs confirmation.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants