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

Replace urllib3 with httpx #333

Closed
wants to merge 9 commits into from
Closed

Conversation

scottwallacesh
Copy link
Contributor

As per our discussion (#332), here's my naïve attempt at simply replacing urllib3 with httpx. I've run it with a number of real-world tests and it seems to work. However, the internal Gabbi tests are not all passing due to the WSGI-Intercept stuff.

@scottwallacesh scottwallacesh force-pushed the main branch 3 times, most recently from 2afbd5a to 52dc02a Compare March 14, 2025 14:52
@cdent
Copy link
Owner

cdent commented Mar 14, 2025

Nice work. I'll look more closely a bit later but for the most part it seems reasonable. I'm uncertain on how best to untangle the wsgi-intercept aspect of the test failures. That will require a bit of thought.

I notice that your editor, or some other part of your process, has reformatted a lot of code that didn't otherwise need to change. That makes it hard to distinguish your actual changes from "cosmetic" changes.

If it is easy for you to do, could you update the PR so it is just the main changes? At some point we should probably black the entire codebase, but for a change like this it would be good to have a commit that is clearly only about the main change. Thanks.

If it's a pain, don't worry, we'll work through it.

@scottwallacesh
Copy link
Contributor Author

Yeah, my IDE has called black, etc.

Code updated.

@scottwallacesh scottwallacesh force-pushed the main branch 2 times, most recently from 29ec0b6 to 3c92f78 Compare March 14, 2025 15:28

Verified

This commit was signed with the committer’s verified signature.
scottwallacesh Scott Wallace
@cdent
Copy link
Owner

cdent commented Mar 16, 2025

Looks good. I'll see if I can untangle the intercept stuff in a branch below this one.

Once we get that working we should be good to move this forward.

Thanks very much for pushing this along. It is great that gabbi continues to evolve and improve.

@cdent
Copy link
Owner

cdent commented Mar 16, 2025

I'm very very close. Looking good. More tomorrow.

@cdent
Copy link
Owner

cdent commented Mar 17, 2025

I've now got it down to everything working except for the tests in https://github.com/cdent/gabbi/blob/main/gabbi/runner.py . These used wsgi-intercept in a way that can't be replicated with httpx's transport so I'm switching it to use external processes. This is taking longer than expected because there's some complex stuff going on with stdin/stdout/stderr handling that has buffering issues. It's feeling easier to switch to managing subprocesses that untangling the buffers. Will be a few more days.

@cdent
Copy link
Owner

cdent commented Mar 18, 2025

Down to just two failing tests

@cdent
Copy link
Owner

cdent commented Mar 18, 2025

All passing. Will see what needs to be done to target this branch.

cdent added 5 commits March 18, 2025 19:53
It was eol October of 2024 and does not have support
for removeprefix, which we now use.
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.

None yet

2 participants