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

test_get_brotli failed in whitenoise 4.1.3 #225

Closed
felixonmars opened this issue Jul 14, 2019 · 18 comments
Closed

test_get_brotli failed in whitenoise 4.1.3 #225

felixonmars opened this issue Jul 14, 2019 · 18 comments

Comments

@felixonmars
Copy link

@felixonmars felixonmars commented Jul 14, 2019

I am getting the following test failures with the new release:

======================================================================
ERROR: test_get_brotli (tests.test_django_whitenoise.DjangoWhiteNoiseTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/build/python-whitenoise/src/whitenoise-4.1.3/tests/test_django_whitenoise.py", line 94, in test_get_brotli
    self.assertEqual(brotli.decompress(response.content), self.static_files.js_content)
brotli.error: BrotliDecompress failed

----------------------------------------------------------------------

Test environment contains the following Arch Linux x86_64 packages:

Packages (13) python-appdirs-1.4.3-2  python-chardet-3.0.4-2  python-idna-2.8-1  python-packaging-19.0-1  python-pyparsing-2.4.0-1
              python-pytz-2019.1-1  python-six-1.12.0-1  python-sqlparse-0.2.4-2  python-urllib3-1.25.3-1  python-brotli-1.0.7-1
              python-django-2.2.3-1  python-requests-2.22.0-1  python-setuptools-1:41.0.1-1
@evansd
Copy link
Owner

@evansd evansd commented Jul 14, 2019

Thanks for reporting this. Are you able to run git bisect against the previous release and find which commit introduced the failure?

@felixonmars
Copy link
Author

@felixonmars felixonmars commented Jul 31, 2019

Looks like the description is not accurate - I can reproduce this with 4.1.2 as well, so it should be related to a brotli update.

@evansd
Copy link
Owner

@evansd evansd commented Aug 14, 2019

Hi, I can't easily reproduce this myself as I don't run Arch. But nothing has changed with respect to our Brotli handling for quite a long time. The last commit which touched anything to do with it was this, which changed the dependency from brotlipy to the official brotli package: 3892ad6

Is that the commit which causes the probem?

@dmsimard
Copy link

@dmsimard dmsimard commented Sep 10, 2019

@evansd FWIW I also found this error in Fedora when attempting to package whitenoise 4.1:

BUILDSTDERR: ..../builddir/build/BUILD/whitenoise-8883095d68e6f9792e1dac97f63225b859b775c8/whitenoise/middleware.py:73: RemovedInDjango31Warning: The FILE_CHARSET setting is deprecated. Starting with Django 3.1, all files read from disk must be UTF-8 encoded.
BUILDSTDERR:   self.charset = settings.FILE_CHARSET
BUILDSTDERR: E........................................................................................
BUILDSTDERR: ======================================================================
BUILDSTDERR: ERROR: test_get_brotli (tests.test_django_whitenoise.DjangoWhiteNoiseTest)
BUILDSTDERR: ----------------------------------------------------------------------
BUILDSTDERR: Traceback (most recent call last):
BUILDSTDERR:   File "/builddir/build/BUILD/whitenoise-8883095d68e6f9792e1dac97f63225b859b775c8/tests/test_django_whitenoise.py", line 94, in test_get_brotli
BUILDSTDERR:     self.assertEqual(brotli.decompress(response.content), self.static_files.js_content)
BUILDSTDERR: brotli.error: BrotliDecompress failed
BUILDSTDERR: ----------------------------------------------------------------------
BUILDSTDERR: Ran 93 tests in 0.577s
BUILDSTDERR: FAILED (errors=1)

The full logs of the whitenoise build are here: https://koji.fedoraproject.org/koji/taskinfo?taskID=37472344

I'll investigate and try 4.1.3 to see what happens.
Will let you know if I find anything.

@dmsimard
Copy link

@dmsimard dmsimard commented Sep 10, 2019

I am able to reproduce the issue on both Fedora and Ubuntu with the following:

#!/bin/bash
rm -rf /tmp/test
mkdir -p /tmp/test
python3 -m venv /tmp/test/venv
source /tmp/test/venv/bin/activate

pip install whitenoise
pip install requests
pip install Django
pip install "coverage<4.3"
pip install brotli

git clone https://github.com/evansd/whitenoise /tmp/test/whitenoise

pushd /tmp/test/whitenoise
export DJANGO_SETTINGS_MODULE=tests.django_settings
python3 -m unittest discover
popd

However, on these same systems, running the tests from source with tox doesn't return any failures.
I don't know why yet but I'm out of time for this issue today, will continue looking when I have a chance.

@dmsimard
Copy link

@dmsimard dmsimard commented Sep 11, 2019

I've isolated the issue to the requests python library.

tox installs requests>=2.11,<2.12:

requests>=2.11,<2.12

The issue occurs only with the current latest version of requests, 2.22.0.
It does not happen with 2.21.0.

This works:

#!/bin/bash
rm -rf /tmp/test
mkdir -p /tmp/test
python3 -m venv /tmp/test/venv
source /tmp/test/venv/bin/activate

pip install whitenoise
pip install "requests==2.21.0"
pip install Django
pip install "coverage<4.3"
pip install brotli

git clone https://github.com/evansd/whitenoise /tmp/test/whitenoise

pushd /tmp/test/whitenoise
export DJANGO_SETTINGS_MODULE=tests.django_settings
python3 -m unittest discover
popd

And this doesn't:

#!/bin/bash
rm -rf /tmp/test
mkdir -p /tmp/test
python3 -m venv /tmp/test/venv
source /tmp/test/venv/bin/activate

pip install whitenoise
pip install "requests==2.22.0"
pip install Django
pip install "coverage<4.3"
pip install brotli

git clone https://github.com/evansd/whitenoise /tmp/test/whitenoise

pushd /tmp/test/whitenoise
export DJANGO_SETTINGS_MODULE=tests.django_settings
python3 -m unittest discover
popd

@evansd do you know what could be happening ?

@evansd
Copy link
Owner

@evansd evansd commented Sep 11, 2019

@dmsimard Thanks for the excellent issue isolation here. Based on that it was quite easy to figure out.

Between v2.21.0 and v2.22.0 requests upgraded its urllib3 dependency, so in your first example it installs v1.24.3 and in the second it installs v1.25.3.

In between those version, urllib3 added native brotli support so it ends up transparently decoding the compressed response, which makes the test fail.

It should be fairly easy to update the test to handle this. I'll try to do that as soon as I have a minute.

@dmsimard
Copy link

@dmsimard dmsimard commented Sep 11, 2019

@evansd great find and thanks for your help. Happy to test an eventual fix to make sure the issue is resolved.

evansd added a commit that referenced this issue Sep 12, 2019
Newer versions of `requests` depend on an updated version of `urllib3`
which transparently decodes Brotli just as it does for gzip.

References #225
@dmsimard
Copy link

@dmsimard dmsimard commented Sep 16, 2019

@evansd It looks like the issue no longer occurs on master with ca67144.

Would it be possible to tag and ship a release with this commit ? It'll allow me to fix the chain of failed builds in Fedora.

Thanks !

@evansd
Copy link
Owner

@evansd evansd commented Sep 16, 2019

@dmsimard I've actually had a slight change of heart on this fix. It involves dropping Python 3.4 support for no other reason than the latest version of requests has dropped it.

I wonder if another solution is to just to make the tests easier to run outside of tox by specifying the test requirements in a standalone file e.g. requirements.test.txt so your test run would look something like:

#!/bin/bash
rm -rf /tmp/test
mkdir -p /tmp/test
python3 -m venv /tmp/test/venv
source /tmp/test/venv/bin/activate

git clone https://github.com/evansd/whitenoise /tmp/test/whitenoise

pushd /tmp/test/whitenoise
pip install -r requirements.test.txt
export DJANGO_SETTINGS_MODULE=tests.django_settings
python3 -m unittest discover
popd

Would that work for you?

@dmsimard
Copy link

@dmsimard dmsimard commented Sep 16, 2019

Works for me.

@evansd
Copy link
Owner

@evansd evansd commented Sep 16, 2019

@dmsimard Can you just check how 94d6517 works for you, and if that's good I'll ship a new release right away.

@dmsimard
Copy link

@dmsimard dmsimard commented Sep 16, 2019

@evansd tried a scratch build of the current master branch and the good news is that the brotli error is gone but there's a new one now:

+ python3 -m unittest discover
BUILDSTDERR: ....E.......................................................................................
BUILDSTDERR: ======================================================================
BUILDSTDERR: ERROR: setUpClass (tests.test_django_whitenoise.DjangoWhiteNoiseTest)
BUILDSTDERR: ----------------------------------------------------------------------
BUILDSTDERR: Traceback (most recent call last):
BUILDSTDERR:   File "/builddir/build/BUILD/whitenoise-master/tests/test_django_whitenoise.py", line 45, in setUpClass
BUILDSTDERR:     cls.static_files = Files("static", js="app.js", nonascii="nonascii\u2713.txt")
BUILDSTDERR:   File "/builddir/build/BUILD/whitenoise-master/tests/utils.py", line 59, in __init__
BUILDSTDERR:     with open(os.path.join(self.directory, path), "rb") as f:
BUILDSTDERR: FileNotFoundError: [Errno 2] No such file or directory: '/builddir/build/BUILD/whitenoise-master/tests/test_files/static/nonascii✓.txt'
BUILDSTDERR: ----------------------------------------------------------------------
BUILDSTDERR: Ran 91 tests in 1.147s
BUILDSTDERR: FAILED (errors=1)

That said, I noticed that the version of requests is still pinned in the test requirements and unpinning it brings back the brotli issue, reproducible with tox.

@evansd
Copy link
Owner

@evansd evansd commented Sep 16, 2019

That's very odd. Can you provide a link to the full build log which contains that error?

As I understood the issue it was just that newer versions of requests are incompatible with the existing test suite, so pinning the version of requests used should fix it. I don't really understand why that would cause the failure you're getting now.

@dmsimard
Copy link

@dmsimard dmsimard commented Sep 24, 2019

@evansd sorry, missed your reply -- I just tried a build off of a forked/tagged 4.1.4 here: https://koji.fedoraproject.org/koji/taskinfo?taskID=37841593

I'm not sure how I ended up with that previous error but it's failing on the brotli thing again:

+ python3 -m unittest discover
....E.............................................................................................
======================================================================
ERROR: test_get_brotli (tests.test_django_whitenoise.DjangoWhiteNoiseTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/builddir/build/BUILD/whitenoise-4.1.4/tests/test_django_whitenoise.py", line 98, in test_get_brotli
    brotli.decompress(response.content), self.static_files.js_content
brotli.error: BrotliDecompress failed
----------------------------------------------------------------------
Ran 98 tests in 0.458s
FAILED (errors=1)

It's not easily possible for us to pin to an older version of requests in Fedora -- we have just the latest versions available: https://koji.fedoraproject.org/koji/packageinfo?packageID=12446

The alternative would be to disable unit tests during the build but that would be unfortunate as it is a good sanity to check to make sure we haven't messed up somewhere.

evansd added a commit that referenced this issue Sep 24, 2019
This is to enable running the tests as part of Fedora's build pipeline
when packing WhiteNoise.

See #225
@evansd
Copy link
Owner

@evansd evansd commented Sep 24, 2019

@dmsimard OK, that makes sense. I've just stuck a conditional in which allows the tests to run under older and newer versions of requests. Let me know if that works and I'll cut the 4.1.4 release.

dmsimard added a commit to dmsimard/whitenoise that referenced this issue Sep 24, 2019
This is to enable running the tests as part of Fedora's build pipeline
when packing WhiteNoise.

See evansd#225
@dmsimard
Copy link

@dmsimard dmsimard commented Sep 24, 2019

@evansd works for me, I managed to get a successful scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=37843426

I can submit the updated spec to get the successful build into the distro as soon as 4.1.4 is tagged. Thanks a lot for your help :)

@evansd
Copy link
Owner

@evansd evansd commented Sep 24, 2019

No problem. Glad it's all working now. Just pushed out the 4.1.4 release.

@evansd evansd closed this Sep 24, 2019
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this issue Nov 13, 2019
v4.1.4
 * Make tests more deterministic and easier to run outside of ``tox``.
 * Fix Fedora packaging `issue <https://github.com/evansd/whitenoise/issues/225>`_.
 * Use `Black <https://github.com/psf/black>`_ to format all code.

v4.1.3
 * Fix handling of zero-valued mtimes which can occur when running on some
   filesystems.
 * Fix potential path traversal attack while running in autorefresh mode on
   Windows.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants