Permalink
Browse files

Hack around Python import idiocy in feed parsing tests.

  • Loading branch information...
sjl committed Oct 31, 2012
1 parent d265446 commit e41f74fa5a17573b08001cc1431b615abca930ba
Showing with 18 additions and 6 deletions.
  1. +18 −6 apps/videos/tests/feeds.py
View
@@ -25,10 +25,7 @@
from apps.videos.feed_parser import FeedParser
from apps.videos.models import Video, VideoFeed
-from apps.videos.types.dailymotion import DailymotionVideoType
-from apps.videos.types.htmlfive import HtmlFiveVideoType
from apps.videos.types.vimeo import VimeoVideoType
-from apps.videos.types.youtube import YoutubeVideoType
class TestFeedsSubmit(TestCase):
@@ -118,11 +115,16 @@ def test_vimeo_feed_parsing(self):
self.assertTrue(video)
def test_youtube_feed_parsing(self):
+ # I hate you, Python.
+ from videos.types.youtube import YoutubeVideoType as YoutubeVideoTypeA
+ from apps.videos.types.youtube import YoutubeVideoType as YoutubeVideoTypeB
+
feed_url = self.youtube_feed_url_pattern % self.youtube_username
feed_parser = FeedParser(feed_url)
vt, info, entry = feed_parser.items().next()
- self.assertTrue(isinstance(vt, YoutubeVideoType))
+ self.assertTrue(isinstance(vt, YoutubeVideoTypeA)
+ or isinstance(vt, YoutubeVideoTypeB))
video, created = Video.get_or_create_for_url(vt=vt)
self.assertTrue(video)
@@ -133,9 +135,14 @@ def test_mit_feed_parsing(self):
MIT feed contain videos, so if sometime they delete some etries - test
can fail.
"""
+ # Python. What are you doing? Python. Stahp.
+ from videos.types.htmlfive import HtmlFiveVideoType as HtmlFiveVideoTypeA
+ from apps.videos.types.htmlfive import HtmlFiveVideoType as HtmlFiveVideoTypeB
+
feed_parser = FeedParser(self.mit_feed_url)
vt, info, entry = feed_parser.items().next()
- self.assertTrue(isinstance(vt, HtmlFiveVideoType))
+ self.assertTrue(isinstance(vt, HtmlFiveVideoTypeA)
+ or isinstance(vt, HtmlFiveVideoTypeB))
video, created = Video.get_or_create_for_url(vt=vt)
self.assertTrue(video)
@@ -152,11 +159,16 @@ def test_mit_feed_parsing(self):
# self.assertTrue(video)
def test_dailymotion_feed_parsing(self):
+ # Welp.
+ from videos.types.dailymotion import DailymotionVideoType as DailymotionVideoTypeA
+ from apps.videos.types.dailymotion import DailymotionVideoType as DailymotionVideoTypeB
+
feed_url = 'http://www.dailymotion.com/rss/ru/featured/channel/tech/1'
feed_parser = FeedParser(feed_url)
vt, info, entry = feed_parser.items().next()
- self.assertTrue(isinstance(vt, DailymotionVideoType))
+ self.assertTrue(isinstance(vt, DailymotionVideoTypeA)
+ or isinstance(vt, DailymotionVideoTypeB))
video, created = Video.get_or_create_for_url(vt=vt)
self.assertTrue(video)

12 comments on commit e41f74f

@fernandotakai

This comment has been minimized.

Show comment Hide comment
@fernandotakai

fernandotakai Oct 31, 2012

Contributor

Contributor

fernandotakai replied Oct 31, 2012

@sjl

This comment has been minimized.

Show comment Hide comment
@sjl

sjl Oct 31, 2012

Contributor

Funny story: my dad flies model airplanes as a hobby (and I did too when I was younger). That lawnmower plane belongs to one of the guys who frequents the flying field we use. I actually cut the grass in that gif as a summer job.

Also I'm 99% sure the actual video is footage from a news story about a charity event he organized.

Contributor

sjl replied Oct 31, 2012

Funny story: my dad flies model airplanes as a hobby (and I did too when I was younger). That lawnmower plane belongs to one of the guys who frequents the flying field we use. I actually cut the grass in that gif as a summer job.

Also I'm 99% sure the actual video is footage from a news story about a charity event he organized.

@sjl

This comment has been minimized.

Show comment Hide comment
@sjl

sjl Oct 31, 2012

Contributor

Yep, here's the video from the Wings Full of Wishes event in 2004: http://youtu.be/J1TWRMRwFng

Contributor

sjl replied Oct 31, 2012

Yep, here's the video from the Wings Full of Wishes event in 2004: http://youtu.be/J1TWRMRwFng

@tcrayford

This comment has been minimized.

Show comment Hide comment
@tcrayford

tcrayford Oct 31, 2012

wooooaoaooaoaoaooaooaaaoaoaooaoaoaoaoaoaoooa

wooooaoaooaoaoaooaooaaaoaoaooaoaoaoaoaoaoooa

@grota

This comment has been minimized.

Show comment Hide comment
@grota

grota Oct 31, 2012

MRW I read this wat

MRW I read this wat

@travisjeffery

This comment has been minimized.

Show comment Hide comment
@travisjeffery

travisjeffery Oct 31, 2012

@ncoghlan

This comment has been minimized.

Show comment Hide comment
@ncoghlan

ncoghlan Nov 1, 2012

This is the same "double import" bug caused by the old default Django project layout (see https://docs.djangoproject.com/en/dev/releases/1.4/#updated-default-project-layout-and-manage-py).

Because your top level directory is on sys.path and you add the "apps" and "libs" subdirectories in manage.py, the contents of those directories are accessible from Python under two different names.

The correct fix is to make sure you never make a module accessible from sys.path in two different ways - otherwise you will always run the risk of this "different copies under two different names" issue, and it's highly unlikely you'll be able to track down all the different ways of importing the affected modules to ensure consistency. You reall need to pick one way or the other: either always do the imports qualified with "apps" and skip adding the apps subdirectory to sys.path, or else restructure the repo in a similar way to the Django 1.4 project layout changes, so that manage.py is in a directory that doesn't get added to sys.path.

This is the same "double import" bug caused by the old default Django project layout (see https://docs.djangoproject.com/en/dev/releases/1.4/#updated-default-project-layout-and-manage-py).

Because your top level directory is on sys.path and you add the "apps" and "libs" subdirectories in manage.py, the contents of those directories are accessible from Python under two different names.

The correct fix is to make sure you never make a module accessible from sys.path in two different ways - otherwise you will always run the risk of this "different copies under two different names" issue, and it's highly unlikely you'll be able to track down all the different ways of importing the affected modules to ensure consistency. You reall need to pick one way or the other: either always do the imports qualified with "apps" and skip adding the apps subdirectory to sys.path, or else restructure the repo in a similar way to the Django 1.4 project layout changes, so that manage.py is in a directory that doesn't get added to sys.path.

@ncoghlan

This comment has been minimized.

Show comment Hide comment
@ncoghlan

ncoghlan Nov 1, 2012

Hmm, Github appears to have eaten my comment (even though it switched me to watching the thread).

This is an issue by Django's old default project layout. They fixed it in 1.4 (https://docs.djangoproject.com/en/dev/releases/1.4/#updated-default-project-layout-and-manage-py) but the problem still arises for anyone using the old layout.

Specifically, a directory inside a package should never be placed directly on sys.path. The reason doing this is a problem is exactly the issue you hit here: the "same" module is now importable under two different names, and thus, from Python's point of view, looks like two different modules depending on how you import it, which is almost certainly not what you want.

There are two possible fixes that ensure only one of the access methods works and thus server to eliminate the double-import problem:

  1. Remove the lines in manage.py that add package directories to sys.path. The code must be updated to always use the fully qualified name (i.e. no more omitting the "apps." prefix anywhere)
  2. Rearrange the project layout, such that manage.py is not inside the site package, and thus you don't get the same "importable under two different names" issue.

And if you copied your current layout from Pinax... consider filing a bug with them, because it's a broken layout.

Hmm, Github appears to have eaten my comment (even though it switched me to watching the thread).

This is an issue by Django's old default project layout. They fixed it in 1.4 (https://docs.djangoproject.com/en/dev/releases/1.4/#updated-default-project-layout-and-manage-py) but the problem still arises for anyone using the old layout.

Specifically, a directory inside a package should never be placed directly on sys.path. The reason doing this is a problem is exactly the issue you hit here: the "same" module is now importable under two different names, and thus, from Python's point of view, looks like two different modules depending on how you import it, which is almost certainly not what you want.

There are two possible fixes that ensure only one of the access methods works and thus server to eliminate the double-import problem:

  1. Remove the lines in manage.py that add package directories to sys.path. The code must be updated to always use the fully qualified name (i.e. no more omitting the "apps." prefix anywhere)
  2. Rearrange the project layout, such that manage.py is not inside the site package, and thus you don't get the same "importable under two different names" issue.

And if you copied your current layout from Pinax... consider filing a bug with them, because it's a broken layout.

@xiaods

This comment has been minimized.

Show comment Hide comment
@xiaods

xiaods Nov 1, 2012

test the joy comments.

test the joy comments.

@ncoghlan

This comment has been minimized.

Show comment Hide comment
@ncoghlan

ncoghlan Nov 1, 2012

3rd time lucky? (GitHub appears to have thrown away my first two attempts at commenting)

For a very long time, Django's default project layout and management script were set up in such a way that it broke the configuration of Python's import system. They finally fixed this in Django 1.4 (https://docs.djangoproject.com/en/dev/releases/1.4/#updated-default-project-layout-and-manage-py) but there are still a lot of projects out there, including this one, that are using the problematic layout that leads directly to double-import bugs like the one encountered here.

To eliminate the ambiguity, it is necessary to remove any code from manage.py that adds subdirectories of the directory containing manage.py to sys.path and fix all affected imports to use the appropriate prefixes (e.g. always "apps.video" and never just "video").

3rd time lucky? (GitHub appears to have thrown away my first two attempts at commenting)

For a very long time, Django's default project layout and management script were set up in such a way that it broke the configuration of Python's import system. They finally fixed this in Django 1.4 (https://docs.djangoproject.com/en/dev/releases/1.4/#updated-default-project-layout-and-manage-py) but there are still a lot of projects out there, including this one, that are using the problematic layout that leads directly to double-import bugs like the one encountered here.

To eliminate the ambiguity, it is necessary to remove any code from manage.py that adds subdirectories of the directory containing manage.py to sys.path and fix all affected imports to use the appropriate prefixes (e.g. always "apps.video" and never just "video").

@ncoghlan

This comment has been minimized.

Show comment Hide comment
@ncoghlan

ncoghlan Nov 1, 2012

Maybe the link was the problem?

Maybe the link was the problem?

@onelson

This comment has been minimized.

Show comment Hide comment
@onelson

onelson Nov 1, 2012

Leaving this here for those that can't: https://gist.github.com/3991579

Leaving this here for those that can't: https://gist.github.com/3991579

Please sign in to comment.