check for matching alias before subproject slug #2787

funkyHat · 2017-04-10T14:04:51Z

funkyHat · 2017-04-10T14:08:14Z

readthedocs/rtd_tests/tests/test_subprojects.py

        with override_settings(USE_SUBDOMAIN=False):
            resp = self.client.get('/docs/pip/projects/sub_alias/')
            self.assertEqual(resp.status_code, 302)
            self.assertEqual(
                resp._headers['location'][1],
                'http://readthedocs.org/docs/pip/projects/sub_alias/ja/latest/'
            )
+
+    def test_resolver_subproject_subdomain_alias(self):


This test causes test_resolver_subproject_alias to fail if it runs first ("fixed" by creatively renaming so it runs after).
I had a bit of a look around to see if there's a cache I can easily clean but couldn't find anything.
Depends how much of an issue you think this is, but if you're worried & can give me a pointer I'll happily update it to dtrt

…ias_before_slug

funkyHat · 2017-10-25T15:14:10Z

Hi all, it's been 6 months since I've seen an update on this PR, and the issue is still affecting readthedocs.org ... any chance we can get this merged/let me know if changes are required?

Thanks

safwanrahman · 2017-10-25T15:57:51Z

readthedocs/rtd_tests/tests/test_subprojects.py

        with override_settings(USE_SUBDOMAIN=False):
            resp = self.client.get('/docs/pip/projects/sub_alias/')
            self.assertEqual(resp.status_code, 302)
            self.assertEqual(
                resp._headers['location'][1],
                'http://readthedocs.org/docs/pip/projects/sub_alias/ja/latest/'
            )
+
+    def test_resolver_subproject_subdomain_alias(self):
+        with override_settings(USE_SUBDOMAIN=True):


Can you use override_settings as decorator?

funkyHat · 2017-11-10T17:56:11Z

Do you need anything else before this can be merged? Happy to make any required changes

berkerpeksag · 2018-01-07T17:57:32Z

readthedocs/core/views/serve.py

-                    )
-                    subproject = rel.child
-                except (ProjectRelationship.DoesNotExist, KeyError):
+                    subproject = Project.objects.get(slug=subproject_slug)


We could replace the whole try...except...raise block with get_object_or_404(): https://docs.djangoproject.com/en/1.9/topics/http/shortcuts/#get-object-or-404

berkerpeksag · 2018-01-07T17:59:46Z

readthedocs/rtd_tests/tests/test_resolver.py

+
+class ResolverAltSetUp(object):
+
+    def setUp(self):


The use of setUpTestData would make tests faster: https://docs.djangoproject.com/en/1.9/topics/testing/tools/#django.test.TestCase.setUpTestData

It does save 8s (out of ~20 for the module 👍), but it also seems to change the results:

self = <readthedocs.rtd_tests.tests.test_resolver.SmartResolverPathTests testMethod=test_resolver_subproject_subdomain> def test_resolver_subproject_subdomain(self): with override_settings(USE_SUBDOMAIN=False): import pdb; pdb.set_trace() url = resolve_path(project=self.subproject, filename='index.html') > self.assertEqual(url, '/docs/pip/projects/sub/ja/latest/') E AssertionError: u'/docs/pip/projects/sub/' != '/docs/pip/projects/sub/ja/latest/' rtd_tests/tests/test_resolver.py:105: AssertionError _______________________________ SmartResolverPathTestsAlt.test_resolver_subproject_subdomain _______________________________ self = <readthedocs.rtd_tests.tests.test_resolver.SmartResolverPathTestsAlt testMethod=test_resolver_subproject_subdomain> def test_resolver_subproject_subdomain(self): with override_settings(USE_SUBDOMAIN=False): import pdb; pdb.set_trace() > url = resolve_path(project=self.subproject, filename='index.html') E AssertionError: u'/docs/pip/projects/sub/' != '/docs/pip/projects/sub/ja/latest/'

I'm not really sure what's going on to cause that, unfortunately. Any clues?

Is SmartResolverPathTestsAlt.test_resolver_subproject_subdomain the only failing test? SmartResolverPathTests is the base class of SmartResolverPathTestsAlt and ResolverBase is the base class of SmartResolverPathTests. There is a setUp method defined in ResolverBase:

https://github.com/rtfd/readthedocs.org/blob/6a669349cf5349087a2369de8a6011b434ac858e/readthedocs/rtd_tests/tests/test_resolver.py#L18-L26

That method can be cause of the test failure.

Adding

setUp = None

to SmartResolverPathTestsAlt may help.

There is also the following warning in setUpTestData documentation:

Be careful not to modify any objects created in setUpTestData() in your test methods. Modifications to in-memory objects from setup work done at the class level will persist between test methods. If you do need to modify them, you could reload them in the setUp() method with refresh_from_db(), for example.

And we indeed do modify objects created in setUpTestData. For example:

https://github.com/rtfd/readthedocs.org/blob/6a669349cf5349087a2369de8a6011b434ac858e/readthedocs/rtd_tests/tests/test_resolver.py#L61

test_resolver_subproject_subdomain from both SmartResolverPathTests and SmartResolverPathTestsAlt (i.e. yes, the one defined test).
These 2 failures are happening with both setUp methods (i.e. in ResolverBase and ResolverAltSetUp) converted to seUpTestData classmethods.

Are you happy to leave this optimisation for now? Reworking all the modifications seems like overkill for 8s savings (maybe should be a separate PR in any case)?

Yes, let's leave it to a separate PR.

berkerpeksag · 2018-01-07T18:06:35Z

readthedocs/rtd_tests/tests/test_subprojects.py

-            self.translation = fixture.get(Project, slug='trans', language='ja',
-                                           users=[ self.owner],
-                                           main_language_project=None)
+            self.subproject = fixture.get(Project, slug='sub', language='ja', users=[


I'd revert these style changes if the linting tool is happy with the old style.

…ias_before_slug

berkerpeksag · 2018-01-19T12:45:30Z

readthedocs/core/views/serve.py

-                    subproject = Project.objects.get(slug=subproject_slug)
-                except Project.DoesNotExist:
-                    raise Http404
+                get_object_or_404(Project, slug=subproject_slug)


Shouldn't this line be read as

subproject = get_object_or_404(Project, slug=subproject_slug)

?

tannewt · 2018-03-07T02:35:45Z

I just ran into this as well. I would love to see this fixed so we don't need to encode language and version into URLs unnecessarily.

ericholscher · 2018-03-07T08:44:07Z

Can you include a link to something that reproduces this issue currently? There are likely other bugs that are causing this code to be hit.

ericholscher · 2018-03-07T08:44:32Z

Ah, I found #2731 -- will give it a peek.

ericholscher · 2018-03-07T08:52:40Z

Looks good. Thanks for the patch -- sorry it took so long to review!

check for matching alias before subproject slug

9130f38

fixes readthedocs#2731

agjohnson added the PR: ready for review label Apr 10, 2017

funkyHat commented Apr 10, 2017

View reviewed changes

Merge remote-tracking branch 'origin/master' into subproject_check_al…

e07be83

…ias_before_slug

funkyHat force-pushed the subproject_check_alias_before_slug branch from f856067 to e07be83 Compare June 29, 2017 15:12

safwanrahman reviewed Oct 25, 2017

View reviewed changes

Address PR comments

c3d2ddc

RichardLitt requested a review from ericholscher November 10, 2017 22:14

berkerpeksag reviewed Jan 7, 2018

View reviewed changes

funkyHat added 2 commits January 12, 2018 19:17

use get_object_or_404 shortcut

a4bb8e3

revert style changes (where did they even come from?)

9bb0f28

funkyHat force-pushed the subproject_check_alias_before_slug branch from 701e221 to 9bb0f28 Compare January 18, 2018 19:29

Merge remote-tracking branch 'origin/master' into subproject_check_al…

6a66934

…ias_before_slug

berkerpeksag reviewed Jan 19, 2018

View reviewed changes

ericholscher merged commit d127ea5 into readthedocs:master Mar 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

check for matching alias before subproject slug #2787

check for matching alias before subproject slug #2787

funkyHat commented Apr 10, 2017

funkyHat Apr 10, 2017

funkyHat commented Oct 25, 2017

safwanrahman Oct 25, 2017

funkyHat Oct 28, 2017

funkyHat commented Nov 10, 2017

berkerpeksag Jan 7, 2018

berkerpeksag Jan 7, 2018

funkyHat Jan 18, 2018 •

edited

Loading

berkerpeksag Jan 19, 2018

funkyHat Jan 19, 2018

berkerpeksag Jan 19, 2018

berkerpeksag Jan 7, 2018

berkerpeksag Jan 19, 2018

tannewt commented Mar 7, 2018

ericholscher commented Mar 7, 2018

ericholscher commented Mar 7, 2018

ericholscher commented Mar 7, 2018

check for matching alias before subproject slug #2787

check for matching alias before subproject slug #2787

Conversation

funkyHat commented Apr 10, 2017

Choose a reason for hiding this comment

funkyHat commented Oct 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

funkyHat commented Nov 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

funkyHat Jan 18, 2018 • edited Loading

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

tannewt commented Mar 7, 2018

ericholscher commented Mar 7, 2018

ericholscher commented Mar 7, 2018

ericholscher commented Mar 7, 2018

funkyHat Jan 18, 2018 •

edited

Loading