#360 Test for wrong redirects behaviour #381

duker33 · 2018-06-22T09:58:18Z

Closes #360

ArtemijRodionov · 2018-06-27T13:04:25Z

shopelectro/tests/tests_views.py

+        # take some existing `url_from`
+        url_from = '/catalog/categories/category-0/tags/6-v/'
+        response = self.client.get(url_from)
+        self.assertEqual(response.status_code, 200)


It is an unnecessary assertion, we should test only redirect logic here

@artemiy312 , it's specific of this test case.

django.contrib.redirects provide different behaviour if url_from returns 200 or 404

@duker33 but we already test it in other tests, isn't it?

@artemiy312 , it's new test, that fails.
Maybe we touch redirects in some way in different code. In products scope, for example.
But it's doesn't matter for this task.

Here we test mech for redirects concept itself

ArtemijRodionov · 2018-06-27T13:07:14Z

shopelectro/tests/tests_views.py

+    # will be resurrected at rf#140
+    @unittest.expectedFailure
+    def test_redirect_on_existing_page(self):
+        """DB based redirect from existing url should do, but should not avoid it."""


a test is too big and complicated. Let's divide it into smaller pieces

@artemiy312 , it's single test case.

Do test this case, we should check two things:
url_from returns 200 and url_to returns 301

@duker33 We can test it separate.

self.redirect = Redirect.objects.create( # also we may move it to fixtures site=Site.objects.first(), old_path=url_from, new_path=url_to ) def test_redirect_301(self): self.assertEqual(self.client.get(self.redirect.old_path).status_code, 300) def test_following_redirect_200(self): self.assertEqual(self.client.get(self.redirect.old_path, follow=True).status_code, 200)

@artemiy312 , url_from should not return 200 in all cases. Only for our single case.
So, this two methods have strong semantical binding. They both mean nothing in separate.

For a different test case, we should create a different test class.

Big test method's size doesn't look bad for me. It consists of simple and sequential steps

discussed directly

ArtemijRodionov · 2018-07-02T10:52:57Z

shopelectro/tests/tests_views.py

+    def test_redirect_on_existing_page(self):
+        """DB based redirect from existing url should do, but should not avoid it."""
+        # take some existing `url_from`
+        url_from = '/catalog/categories/category-0/tags/6-v/'


Let's get a category's url from Model: Category.objects.first().url

@artemiy312 , here i should take not the first category. But the one, that has at least one tag.

And it'll be too complex to take it. We have fixtures in cvs, so we can completely rely on them in code

ArtemijRodionov · 2018-07-02T10:59:49Z

shopelectro/tests/tests_views.py

+    # will be resurrected at rf#140
+    @unittest.expectedFailure
+    def test_redirect_on_existing_page(self):
+        """DB based redirect from existing url should do, but should not avoid it."""


@duker33 We can test it separate.

self.redirect = Redirect.objects.create( # also we may move it to fixtures site=Site.objects.first(), old_path=url_from, new_path=url_to ) def test_redirect_301(self): self.assertEqual(self.client.get(self.redirect.old_path).status_code, 300) def test_following_redirect_200(self): self.assertEqual(self.client.get(self.redirect.old_path, follow=True).status_code, 200)

ArtemijRodionov · 2018-07-02T11:01:02Z

shopelectro/tests/tests_views.py

+        # take some existing `url_from`
+        url_from = '/catalog/categories/category-0/tags/6-v/'
+        response = self.client.get(url_from)
+        self.assertEqual(response.status_code, 200)


@duker33 but we already test it in other tests, isn't it?

duker33 · 2018-07-03T13:13:32Z

discussed directly

ArtemijRodionov

A wrong version number of refarm-site

ArtemijRodionov · 2018-07-03T15:32:36Z

requirements.txt

 sorl-thumbnail==12.4a1
 https://github.com/selwin/django-user_agents/archive/master.zip
-https://github.com/fidals/refarm-site/archive/0.1.6.zip
+https://github.com/fidals/refarm-site/archive/0.1.3.zip


It is a wrong version number

0.1.6 fails, as i remember. I'll check it TO-DO

ArtemijRodionov · 2018-07-03T15:34:02Z

shopelectro/tests/tests_views.py

+
+    # will be resurrected at rf#140
+    @unittest.expectedFailure
+    def test_redirect_on_existing_page(self):


Maybe test_redirect_from_existing_page will be more relevant. decide by yourself

ArtemijRodionov · 2018-07-03T15:40:17Z

shopelectro/tests/tests_views.py

+    # will be resurrected at rf#140
+    @unittest.expectedFailure
+    def test_redirect_on_existing_page(self):
+        """DB based redirect from existing url should do, but should not avoid it."""


"""DB based redirect from existing url should do, but should not avoid it."""
->
"""Redirect middleware of refarm-site redirects from existing url too"""

decide by yourself

Refarm-site's custom redirects app should redirect from existing url too

TO-DO

ArtemijRodionov

Refarm-site version

ArtemijRodionov · 2018-07-06T14:31:17Z

requirements.txt

+tblib==1.3.2
+Unidecode==1.0.22
+ua-parser==0.8.0
+user-agents==1.1.0
 sorl-thumbnail==12.4a1
 https://github.com/selwin/django-user_agents/archive/master.zip
 https://github.com/fidals/refarm-site/archive/0.1.6.zip


You have to bump the refarm-site version to 0.1.7, isn't it?

@artemiy312 , nope, 0.1.7 was created after this code

duker33 self-assigned this Jun 22, 2018

duker33 requested a review from ArtemijRodionov June 22, 2018 09:58

duker33 force-pushed the 360_fix_redirects branch 2 times, most recently from ca151d5 to a3c499b Compare June 22, 2018 13:25

duker33 mentioned this pull request Jun 22, 2018

rf#140 Change redirects dep #384

Closed

duker33 added the blocker this issue blocks another issue label Jun 22, 2018

ArtemijRodionov requested changes Jun 27, 2018

View reviewed changes

duker33 added the discuss issue needs to finish discussion before start working label Jul 1, 2018

ArtemijRodionov requested changes Jul 2, 2018

View reviewed changes

duker33 removed the discuss issue needs to finish discussion before start working label Jul 3, 2018

duker33 requested a review from ArtemijRodionov July 3, 2018 13:18

duker33 added 5 commits July 3, 2018 16:19

#360 Refresh py reqs according to stb

5c602ba

#360 Create test with unexpected behaviour explaining

c4ee94c

#360 Fix commented test and refarm-site dependency

e393db9

#360 Skip expected failure

a31dffe

#360 Review#1. Rm several asserts

7b80834

duker33 force-pushed the 360_fix_redirects branch from 4636a94 to 7b80834 Compare July 3, 2018 13:19

duker33 added the review waits for review label Jul 3, 2018

ArtemijRodionov requested changes Jul 3, 2018

View reviewed changes

duker33 added refine issue already have resolving code, but code needs some fixes and removed review waits for review labels Jul 3, 2018

#360 Review#2. Names and some reqs

28a932e

duker33 requested a review from ArtemijRodionov July 3, 2018 17:43

duker33 mentioned this pull request Jul 3, 2018

#394 Redirects dep. stb2 #397

Merged

#360 Skip problem test

df2b032

duker33 force-pushed the 360_fix_redirects branch from 951054e to df2b032 Compare July 5, 2018 14:56

duker33 added 2 commits July 5, 2018 18:51

#360 Skip problem test

5b3ae38

#360 Lint's minor fix

8085903

ArtemijRodionov approved these changes Jul 6, 2018

View reviewed changes

duker33 merged commit daee6d9 into master Jul 7, 2018

duker33 deleted the 360_fix_redirects branch July 7, 2018 12:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#360 Test for wrong redirects behaviour #381

#360 Test for wrong redirects behaviour #381

duker33 commented Jun 22, 2018

ArtemijRodionov Jun 27, 2018

duker33 Jul 1, 2018

ArtemijRodionov Jul 2, 2018

duker33 Jul 2, 2018

ArtemijRodionov Jun 27, 2018

duker33 Jul 1, 2018

ArtemijRodionov Jul 2, 2018

duker33 Jul 2, 2018

duker33 Jul 3, 2018

ArtemijRodionov Jul 2, 2018

duker33 Jul 2, 2018

ArtemijRodionov Jul 2, 2018

ArtemijRodionov Jul 2, 2018

duker33 commented Jul 3, 2018

ArtemijRodionov left a comment

ArtemijRodionov Jul 3, 2018

duker33 Jul 3, 2018 •

edited

Loading

ArtemijRodionov Jul 3, 2018

duker33 Jul 3, 2018 •

edited

Loading

ArtemijRodionov Jul 3, 2018

duker33 Jul 3, 2018 •

edited

Loading

ArtemijRodionov left a comment

ArtemijRodionov Jul 6, 2018

duker33 Jul 7, 2018

#360 Test for wrong redirects behaviour #381

#360 Test for wrong redirects behaviour #381

Conversation

duker33 commented Jun 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duker33 commented Jul 3, 2018

ArtemijRodionov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duker33 Jul 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duker33 Jul 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duker33 Jul 3, 2018 • edited Loading

Choose a reason for hiding this comment

ArtemijRodionov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duker33 Jul 3, 2018 •

edited

Loading

duker33 Jul 3, 2018 •

edited

Loading

duker33 Jul 3, 2018 •

edited

Loading