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

check for matching alias before subproject slug #2787

Merged
merged 6 commits into from Mar 7, 2018

Conversation

Projects
None yet
6 participants
@funkyHat
Contributor

funkyHat commented Apr 10, 2017

fixes #2731

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 comment has been minimized.

@funkyHat

funkyHat Apr 10, 2017

Contributor

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

@funkyHat

This comment has been minimized.

Contributor

funkyHat commented Oct 25, 2017

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

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):

This comment has been minimized.

@safwanrahman

safwanrahman Oct 25, 2017

Member

Can you use override_settings as decorator?

This comment has been minimized.

@funkyHat

funkyHat Oct 28, 2017

Contributor

done :)

@funkyHat

This comment has been minimized.

Contributor

funkyHat commented Nov 10, 2017

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

@RichardLitt RichardLitt requested a review from ericholscher Nov 10, 2017

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

This comment has been minimized.

@berkerpeksag

berkerpeksag Jan 7, 2018

Member

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

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

This comment has been minimized.

@berkerpeksag

This comment has been minimized.

@funkyHat

funkyHat Jan 18, 2018

Contributor

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?

This comment has been minimized.

@berkerpeksag

berkerpeksag Jan 19, 2018

Member

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:

def setUp(self):
with mock.patch('readthedocs.projects.models.broadcast'):
self.owner = create_user(username='owner', password='test')
self.tester = create_user(username='tester', password='test')
self.pip = get(Project, slug='pip', users=[self.owner], main_language_project=None)
self.subproject = get(Project, slug='sub', language='ja', users=[self.owner], main_language_project=None)
self.translation = get(Project, slug='trans', language='ja', users=[self.owner], main_language_project=None)
self.pip.add_subproject(self.subproject)
self.pip.translations.add(self.translation)

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:

self.pip.documentation_type = 'sphinx'

This comment has been minimized.

@funkyHat

funkyHat Jan 19, 2018

Contributor

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)?

This comment has been minimized.

@berkerpeksag

berkerpeksag Jan 19, 2018

Member

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

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=[

This comment has been minimized.

@berkerpeksag

berkerpeksag Jan 7, 2018

Member

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

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

This comment has been minimized.

@berkerpeksag

berkerpeksag Jan 19, 2018

Member

Shouldn't this line be read as

subproject = get_object_or_404(Project, slug=subproject_slug)

?

@tannewt

This comment has been minimized.

tannewt commented Mar 7, 2018

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

This comment has been minimized.

Member

ericholscher commented Mar 7, 2018

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

This comment has been minimized.

Member

ericholscher commented Mar 7, 2018

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

@ericholscher

This comment has been minimized.

Member

ericholscher commented Mar 7, 2018

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

@ericholscher ericholscher merged commit d127ea5 into rtfd:master Mar 7, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment