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

Add tests for section-linking #5918

Merged
merged 27 commits into from
Jul 16, 2019
Merged

Add tests for section-linking #5918

merged 27 commits into from
Jul 16, 2019

Conversation

dojutsu-user
Copy link
Member

@dojutsu-user dojutsu-user commented Jul 12, 2019

Tests

@dojutsu-user dojutsu-user added the PR: work in progress Pull request is not ready for full review label Jul 12, 2019
@ericholscher ericholscher changed the base branch from master to gsoc-19-indoc-search July 12, 2019 15:46
@ericholscher
Copy link
Member

Is this still a WIP?

@dojutsu-user
Copy link
Member Author

Just some last commits.

@dojutsu-user dojutsu-user removed the PR: work in progress Pull request is not ready for full review label Jul 15, 2019
Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

This is a good upgrade, but I'm worried this will all break when we change the site design and cause a lot of extra work. Can we access the test data directly from rendering, instead of via HTML? That would make it much less brittle, and not dependent on the templates.

@@ -20,40 +25,112 @@ def setup_class(cls):
# installed
cls.url = reverse('doc_search')

@pytest.mark.parametrize('data_type', ['content', 'headers', 'title'])
@pytest.mark.parametrize('data_type', ['title'])
Copy link
Member

Choose a reason for hiding this comment

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

We don't need param's here if there's only 1.

assert len(results) >= 2 # there are > 1 results without the filter

# checking if `std:confval` filter is present
content = page.find('.navigable .language-list').text()
Copy link
Member

Choose a reason for hiding this comment

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

Also we should fix the HTML, language-list isn't the class we should be using, it should be role-list.

Copy link
Member

Choose a reason for hiding this comment

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

I also worry about depending on the HTML structure. This will break all the tests when we do our site redesign. Can we not capture this data from the query result itself? I believe the Django test client gives us template data directly, which we could use: https://docs.djangoproject.com/en/2.2/topics/testing/tools/#django.test.Response.context

@dojutsu-user
Copy link
Member Author

@ericholscher
I have updated the tests to be independent of templates as per your suggestion in the comment above.

@ericholscher
Copy link
Member

Great, thanks!

@ericholscher ericholscher merged commit c4bdddd into readthedocs:gsoc-19-indoc-search Jul 16, 2019
@dojutsu-user dojutsu-user deleted the tests-section-linking branch July 16, 2019 15:01
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.

2 participants