Fixed import which caused error in cookie unquoting #1292

Closed
wants to merge 6 commits into
from

Projects

None yet

3 participants

@yakky
Collaborator
yakky commented Jun 14, 2012

No description provided.

@yakky
Collaborator
yakky commented Jun 14, 2012

This test is the best I could figure out.
A better test would be needed, though, but I guess it require Selenium to test the jstree behaviour.
I commented the test to track this

@andrewschoen andrewschoen commented on the diff Jun 15, 2012
cms/tests/admin.py
+ """
+ admin = self._get_guys(True)
+ first_level_page = create_page('level1', 'nav_playground.html', 'en')
+ second_level_page_top = create_page('level21', "nav_playground.html", "en",
+ created_by=admin, published=True, parent= first_level_page)
+ second_level_page_bottom = create_page('level22', "nav_playground.html", "en",
+ created_by=admin, published=True, parent= self.reload(first_level_page))
+ third_level_page = create_page('level3', "nav_playground.html", "en",
+ created_by=admin, published=True, parent= second_level_page_top)
+
+ url = reverse('admin:cms_%s_changelist' % Page._meta.module_name)
+ client = Client()
+ client.login(username='admin', password='admin')
+ client.cookies['djangocms_nodes_open'] = 'page_1%2Cpage_2'
+ response = client.get(url)
+ self.assertContains(response,"<title>List of pages</title>")
@andrewschoen
andrewschoen Jun 15, 2012

Let's test the context variable open_menu_trees here. If it contains the id's for the two pages we put in the cookie then we know the unquoting worked.

Something like this.

self.assertEquals(response.context["open_menu_trees"], [1,2])

@yakky
yakky Jun 16, 2012 collaborator

Cool.
Thanks. I added a new test (see below) with a different testing strategy with the assert you suggested.
I overlooked the context var

@andrewschoen

I pulled this patch down and ran it, works great. Besides adding the testing of open_menu_trees it looks good to me.

I don't know much about Selenium testing so I can't comment there. Seems like this should be enough for this test case though.

@piquadrat

this triggers a failure for me:

ERROR: test_changelist_unquote (cms.tests.admin.AdminTestCase)
This test checks for proper jstree cookie unquoting.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/benjamin/projects/django-cms/cms/tests/admin.py", line 419, in test_changelist_unquote
    self.assertEquals(response.context["open_menu_trees"], [1,2])
AttributeError: 'HttpResponse' object has no attribute 'context'

response is a plain old HttpResponse instance in this case, since you are not using the django test client. Any other ideas how to test this?

Collaborator

Actually, both assertTemplateUsed and accessing response.context will not work in this case, because response is a normal HttpResponse, not a response from the test client.

The above failure is with Django 1.4. With 1,3, I get a different failure (not sure why 1.4 doesn't fail on this line already as well):

======================================================================
ERROR: test_changelist_unquote (cms.tests.admin.AdminTestCase)
This test checks for proper jstree cookie unquoting.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/benjamin/projects/django-cms/cms/tests/admin.py", line 418, in test_changelist_unquote
    self.assertTemplateUsed("admin/cms/page/change_list.html")
TypeError: assertTemplateUsed() takes at least 3 arguments (2 given)
@piquadrat

I suggest to cherry-pick bffb609 into develop and merge the tests when they're ready. On the current develop, the changelist is basically broken.

@yakky
Collaborator
yakky commented Jun 19, 2012

To check for the current bug just asserting anything is ok as the bug result in an Exception.
I think having a proper response object and asserting the context variable is the more proper way.
Sorry for the broken test, coding in hurry is bad :(

@piquadrat

OK, I cherry-picked the import fix. I'll leave the pull request open until the tests are merged as well, but move it to the 2.3.1 milestone.

@andrewschoen

If we just go back to using the test client everything works. IMHO if we test that status code and the open_menu_trees context variable we'd be good.

Here is a diff of what I changed from your original test case. I've tested this on Django 1.3.1 and 1.4 and all tests pass.

http://dpaste.com/761189/

@yakky
Collaborator
yakky commented Jun 20, 2012

Studied issue a bit more, it's more a test-style issue than a coding one.
Using test client enables a more thorough testing at the expense of performance hit, while the use of the API result in a very narrow test scope.
Comments welcome

@andrewschoen andrewschoen added a commit to andrewschoen/django-cms that referenced this pull request Jun 27, 2012
@yakky yakky Test for #1292. 94926e5
@andrewschoen andrewschoen referenced this pull request Jun 27, 2012
Merged

Lazy load tests #1325

@piquadrat

The remaining commits of this PR have been merged with #1325. Closing.

@piquadrat piquadrat closed this Jun 29, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment