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 leak fixes #1360

Closed

Conversation

beniwohli
Copy link
Contributor

this fixes all the test leaks that are currently appearing

WSGIRequest has no attribute current_page

This can be solved by setting the current_page attribute on the fake request

NonRootCase URL reversal

as @yakky pointed out in #1335, it seems to be necessary to call client.get() or client.post() before doing URL-reversal. While I'm not 100% sure why this is necessary, I guess that django only sets up the whole URL-reversal infrastructure when the client is used the first time.

…current_page`` attribute on the fake request
I'm not quite 100% sure why the self.client.get('/') is necessary, but my guess is that it sets up the URL reversal machinery the first time that the test client is used.
@travisbot
Copy link

This pull request passes (merged f0911d7 into 33a994c).

@@ -254,6 +254,7 @@ def get_request(self, path=None, language=None, post_data=None, enforce_csrf_che
request.user = getattr(self, 'user', AnonymousUser())
request.LANGUAGE_CODE = language
request._dont_enforce_csrf_checks = not enforce_csrf_checks
request.current_page = page
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to me this looks like covering up the real issue. which is that request.current_page apparently leaked in some test to another test. Shouldn't that be fixed instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that the change per se is a bad one (it makes the fake request look more like a real one). But I'll admit that it may cover the real problem. Let's try to fix that first, then merge this

@yakky
Copy link
Member

yakky commented Jul 29, 2012

I tried runtests-isolated.py from #1336 with beniwohli/django-cms@f0911d7 with success.
Still don't get why, but it looks the only sensible solution ATM.

@yakky
Copy link
Member

yakky commented Jul 29, 2012

As for the current_page issue, maybe the cause is exactly the same.
I concentrated on FixturesMenuTests: test_basic_cms_menu() calls self.client.get() (https://github.com/divio/django-cms/blob/develop/cms/tests/menu.py#L86) which calls the middlewares, including CurrentPageMiddleware which inserts the current page in the request.
I bruteforced a fix inserting a self.client.get("/") in get_request() (https://github.com/divio/django-cms/blob/develop/cms/test_utils/testcases.py#L256) this is obviously not a fix, but makes the failing tests pass (other tests which check the number of the queries or cache content obviously fails due to the different environment).

@ojii
Copy link
Contributor

ojii commented Aug 3, 2012

real bug for nonroot tests is that get_pages_root returns '/' instead of '/content/' before the first request.

@yakky
Copy link
Member

yakky commented Aug 3, 2012

Yes but actually that happens only if no client.get() has been issued before (in any previous test)
We discovered that while executing that on test at a time.
As reported by me (in #1335) and @piquadrat here is not easy to figure out why does this happens (whether a new_reverse issue, or a too lazy loading of cms urls by django or an incomplete request object)

@ojii
Copy link
Contributor

ojii commented Aug 7, 2012

See https://github.com/ojii/django-cms/tree/get-pages-root-problem for a test that shows get_pages_root seems to be the culprit

Run python runtests.py NonRootCase.test_get_page_root.

@ojii
Copy link
Contributor

ojii commented Aug 8, 2012

fixed in 998bceb

@ojii ojii closed this Aug 8, 2012
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling f0911d7 on piquadrat:feature/test-leak-fixes into * on divio:develop*.

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

5 participants