Code

Opened 10 months ago

Closed 9 months ago

#21162 closed Bug (fixed)

regression: changeset e909ceae9b3e72b72e0a2baaa92bba9714f18cd2 breaks some tests for Windows with sqlite

Reported by: manfre Owned by: ramiro
Component: Core (Management commands) Version: master
Severity: Release blocker Keywords:
Cc: ramiro Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Regression added with changeset e909ceae9b3e72b72e0a2baaa92bba9714f18cd2.

python tests\runtests.py --settings=test_sqlite admin_scripts.tests.StartProject.test_custom_project_template_from_tarball_by_url


======================================================================
FAIL: test_custom_project_template_from_tarball_by_url (admin_scripts.tests.StartProject)
Make sure the startproject management command is able to use a different project template from a tarball via a url
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\projects\django\django\tests\admin_scripts\tests.py", line 1605, in test_custom_project_template_from_tarball_by_url
    self.assertNoOutput(err)
  File "C:\projects\django\django\tests\admin_scripts\tests.py", line 182, in assertNoOutput
    self.assertEqual(len(stream), 0, "Stream should be empty: actually contains '%s'" % stream)
AssertionError: Stream should be empty: actually contains 'Traceback (most recent call last):
  File "C:\projects\django\django\django\bin\django-admin.py", line 5, in <module>
    management.execute_from_command_line()
  File "C:\projects\django\django\django\core\management\__init__.py", line 397, in execute_from_command_line
    utility.execute()
  File "C:\projects\django\django\django\core\management\__init__.py", line 390, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "C:\projects\django\django\django\core\management\base.py", line 242, in run_from_argv
    self.execute(*args, **options.__dict__)
  File "C:\projects\django\django\django\core\management\base.py", line 289, in execute
    output = self.handle(*args, **options)
  File "C:\projects\django\django\django\core\management\commands\startproject.py", line 31, in handle
    super(Command, self).handle('project', project_name, target, **options)
  File "C:\projects\django\django\django\core\management\templates.py", line 125, in handle
    base_subdir)
  File "C:\projects\django\django\django\core\management\templates.py", line 208, in handle_template
    return self.extract(absolute_path)
  File "C:\projects\django\django\django\core\management\templates.py", line 302, in extract
    archive.extract(filename, tempdir)
  File "C:\projects\django\django\django\utils\archive.py", line 49, in extract
    with Archive(path) as archive:
  File "C:\projects\django\django\django\utils\archive.py", line 58, in __init__
    self._archive = self._archive_cls(file)(file)
  File "C:\projects\django\django\django\utils\archive.py", line 138, in __init__
    self._archive = tarfile.open(file)
  File "C:\Python27\Lib\tarfile.py", line 1665, in open
    raise ReadError("file could not be opened successfully")
tarfile.ReadError: file could not be opened successfully
'
e909ceae9b3e72b72e0a2baaa92bba9714f18cd2 is the first bad commit
commit e909ceae9b3e72b72e0a2baaa92bba9714f18cd2
Author: Ramiro Morales <cramm0@gmail.com>
Date:   Sat Jun 1 14:24:46 2013 -0300

    Made django.test.testcases not depend on staticfiles contrib app.

    Do this by introducing a django.contrib.staticfiles.testing.StaticLiveServerCase
    unittest TestCase subclass.

    Fixes #20739.

:040000 040000 3602788911b0c9f9d6c95805b321968cfed98571 11b64417027fd4776f0123e3c0022d4155aea4b9 M      django
:040000 040000 0699494f0516bb71df49f1d88ab3b3a096ddd33c 3e238009f30bc5223ac6711c560f041af8949fa7 M      docs
:040000 040000 47ae44d2101b694c1d570fa29a483de2e43cdd54 c341edbef9d7c04bbf5760cf4a652152eba62e59 M      tests

Attachments (0)

Change History (6)

comment:1 Changed 10 months ago by manfre

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Summary changed from regression: admin_scripts.tests.StartProject test fails on Windows using sqlite to regression: changeset e909ceae9b3e72b72e0a2baaa92bba9714f18cd2 breaks some tests for Windows with sqlite

Same changeset also responsible for servers.tests.LiveServerViews.test_no_collectstatic_emulation failure.

======================================================================
FAIL: test_no_collectstatic_emulation (servers.tests.LiveServerViews)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\projects\django\django\tests\servers\tests.py", line 148, in test_no_collectstatic_emulation
    self.assertEqual(err.code, 404, 'Expected 404 response')
AssertionError: Expected 404 response

comment:2 Changed 10 months ago by ramiro

  • Has patch set
  • Owner changed from nobody to ramiro
  • Status changed from new to assigned

Original committer here.

This solves both problems:

diff --git a/django/test/testcases.py b/django/test/testcases.py
index a657530..64a80ab 100644
--- a/django/test/testcases.py
+++ b/django/test/testcases.py
@@ -958,7 +958,8 @@ class FSFilesHandler(WSGIHandler):

     def serve(self, request):
         os_rel_path = self.file_path(request.path)
-        final_rel_path = posixpath.normpath(unquote(os_rel_path)).lstrip('/')
+        os_rel_path = posixpath.normpath(unquote(os_rel_path))
+        final_rel_path = os_rel_path.replace('\\', '/').lstrip('/')
         return serve(request, final_rel_path, document_root=self.get_base_dir()

     def __call__(self, environ, start_response):

We have historic, deeper inconsistencies, namely:

  1. The file_path() method of contrib.staticfiles StaticFilesHandler middleware (and of the django.test.testcases static and media ones introduced by my commit too because they are copied from it) changes the request path to one with OS-specific path separators (e.g. '/a/b/c.jpg' to '\a\b\c.jpg' on Windows.)
  2. That OS-specific path is passed to the respective serve() method where it is, among other things, massaged with posixpath.normpath() (note how it has no effect on Windows) and then handed to a relevant serve() view (django.contrib.staticfiles.views.serve() and django.views.static.serve() respectively).
  3. Then the staticfiles serve() view invokes that app's finders machinery which consumes OS-specific paths and returns forward-slash delimited paths. The newly-introduced middlewares don't do this and this is where things diverged because all further processing is done on a OS-specific path and as such most of the processing performed by django.views.static.serve() has no effect because it expects a '/'-delimited path.

The patch above changes this by using a hack similar to one used by django.views.static.serve() to replace '\' with '/' to restore behavior parity.

A longer term project would be to fix (to always consume '/'-delimited paths and so make it possible to use that kind of paths through all the above described workflow and leave handling of OS-specific ones to the finders), formalize and document the (internal, so far undocumented) API with staticfiles finders backends, although it might break third party backends.

comment:3 Changed 10 months ago by manfre

The patch fixes the test failures I reported.

comment:4 follow-up: Changed 10 months ago by apollo13

Why not just use .lstrip(os.separator) instead of the replacing?

comment:5 in reply to: ↑ 4 Changed 10 months ago by ramiro

Replying to apollo13:

Why not just use .lstrip(os.separator) instead of the replacing?

Because at that point _all_ the path path separators in 'os_rel_path' are platform-specific not just the leading one.

The lstrip() was alredy there (see also e.g. django.contrib.staticfiles.views.serve()), it's the the replacement of path separators (that needs to happen before) what is new . I've just refactored the code a bit.

comment:6 Changed 9 months ago by Ramiro Morales <cramm0@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In 783620ccc827057d1d0f2c6f4808350be9193bf9:

Fixed #21162 -- Better emulation of staticfiles middleware.

Code had been added in e909ceae9b. Solves test suite failures observed
on Windows.

Thanks Michael Manfre for the report. Refs #20739.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.