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

Support brotlicffi in tests #2906

Closed
1 task done
mgorny opened this issue Oct 29, 2023 · 4 comments · Fixed by #2935
Closed
1 task done

Support brotlicffi in tests #2906

mgorny opened this issue Oct 29, 2023 · 4 comments · Fixed by #2935

Comments

@mgorny
Copy link
Contributor

mgorny commented Oct 29, 2023

While the package itself can work with either brotli or brotlicffi, tests/test_decoders.py explicitly requires brotli. We're currently working on having all packages support brotlicffi in Gentoo, since brotli doesn't work reliably on PyPy3. Could you please consider making the test accept brotlicffi as well?

I suppose the simplest way would be to reuse the existing logic, i.e.:

from httpx._compat import brotli

I can submit a PR for that.


@karpetrosyan
Copy link
Member

I can submit a PR for that.

Yes, that would be great!

@tomchristie
Copy link
Member

Alternate options:

  • We could just always brotlicffi in the tests instead of brotli. (Because we value platform independence over c-performance here.)
  • We don't actually need the brotli dependency in the tests at all, so we could drop it outright. We only use it on 3 lines, each of which we can replace using the byte encoding directly...
-    compressed_body = brotli.compress(body)
+    compressed_body = b"\x8b\x03\x80test 123\x03"
-    body = b"test 123"
-    compressed_body = brotli.compress(body)[3:]
+    compressed_body = b"invalid"

@mgorny
Copy link
Contributor Author

mgorny commented Nov 10, 2023

* We could just always `brotlicffi` in the tests instead of `brotli`. (Because we value platform independence over c-performance here.)

That would work for us since we care about brotlicffi only but I think it'd be better to match the implementation used by httpx in the more general case.

* We don't actually need the `brotli` dependency in the tests at all, so we could drop it outright. We only use it on 3 lines, each of which we can replace using the byte encoding directly...

Yeah, that would work for me.

Please let me know which approach you prefer and I can do it.

@tomchristie
Copy link
Member

Please let me know which approach you prefer and I can do it.

I'd suggest the second.

mgorny added a commit to mgorny/httpx that referenced this issue Nov 10, 2023
Inline the compressed Brotli samples in tests to make them independent
of Brotli implementation.  This makes it possible to run the test suite
both against Brotli and brotlicffi.

Fixes encode#2906
mgorny added a commit to mgorny/httpx that referenced this issue Nov 10, 2023
Inline the compressed Brotli samples in tests to make them independent
of Brotli implementation.  This makes it possible to run the test suite
both against Brotli and brotlicffi.

Fixes encode#2906
tomchristie pushed a commit that referenced this issue Nov 10, 2023
Inline the compressed Brotli samples in tests to make them independent
of Brotli implementation.  This makes it possible to run the test suite
both against Brotli and brotlicffi.

Fixes #2906
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 a pull request may close this issue.

3 participants