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

Test for search functionality #4116

Merged
merged 18 commits into from Jun 5, 2018
Merged

Conversation

@safwanrahman
Copy link
Member

@safwanrahman safwanrahman commented May 19, 2018

For refactoring search, need to have proper a test suite.
This is a first phase of test. It run successfully locally with elasticsearch installed.
Currently, only project searching is being tested.

Its written in pytest style as we are using pytest.

Searching page test is still needed. (implemented)
@ericholscher @agjohnson @humitos Have a thought?

@safwanrahman safwanrahman added this to Up next in Search update May 19, 2018
@safwanrahman safwanrahman moved this from Up next to In progress in Search update May 19, 2018
@@ -47,6 +47,9 @@ def DATABASES(self): # noqa
'test:8000',
)

DOCKER_ENABLE = True
Copy link
Member

@stsewd stsewd May 19, 2018

I think this shouldn't be here, at least for now, since we don't have a good guide for development using docker, new contributors wouldn't have this easy.

Copy link
Member Author

@safwanrahman safwanrahman May 19, 2018

Oh! Sorry. I did not want to commit the settings file

@safwanrahman safwanrahman force-pushed the search_test branch 2 times, most recently from 0805763 to 61fa709 May 20, 2018
@safwanrahman safwanrahman force-pushed the search_test branch 2 times, most recently from 1fc2893 to ecbb128 May 21, 2018
@safwanrahman safwanrahman mentioned this pull request May 21, 2018
Copy link
Member

@ericholscher ericholscher left a comment

This looks like a great start! I'm glad to have ES running in Travis, and actually doing some integration tests against it.

I think we should probably add a flag that runs these tests. We don't want to require users to be running ES locally to have the tests pass, so something like a search-integration flag or something that we pass to pytest would be good to enable these only when we want them to run.

@humitos
Copy link
Member

@humitos humitos commented May 21, 2018

@safwanrahman marked the tests as search, so we could add this to the tox.ini file that runs the tests:

py.test -m "not search" {posargs}

that way, search tests are not run by default.

Although, I would like to have these tests run by default on Travis and not in local development. So, we should find a way for this.

@ericholscher
Copy link
Member

@ericholscher ericholscher commented May 21, 2018

Yea, I think we should probably have it run_when a tag is present, instead of when it isn't. That way we can just set the tag on Travis, and locally if we want to run it.

I think something like this should work: https://docs.pytest.org/en/latest/example/markers.html#marking-platform-specific-tests-with-pytest

@safwanrahman safwanrahman changed the title [WIP] Test for search functionality Test for search functionality May 27, 2018
@safwanrahman
Copy link
Member Author

@safwanrahman safwanrahman commented May 27, 2018

@ericholscher @humitos I think its ready for a round of review.

Copy link
Member

@ericholscher ericholscher left a comment

Looks like a great start. I had a few comments, but really like the approach of having the full JSON -> index -> search flow tested. 👍

# There should be only one version of the project
assert len(versions) == 1

data = DUMMY_PAGE_JSON[project.slug][0]
Copy link
Member

@ericholscher ericholscher May 29, 2018

We should iterate over all the pages and check if the title search works, for a bit more coverage.

Copy link
Member Author

@safwanrahman safwanrahman May 29, 2018

Yeah. I am thinking about writing this test using pytest parameters!


page = pq(resp.content)
content = page.find('.module-list-wrapper .module-item-title')
assert project.name.encode('utf-8') in content.text().encode('utf-8')
Copy link
Member

@ericholscher ericholscher May 29, 2018

I wonder if you could use resp.context to make this easier: https://docs.djangoproject.com/en/1.9/topics/testing/tools/#django.test.Response.context

It should let you text the template context that got rendered, without depending on the HTML structure.

Copy link
Member Author

@safwanrahman safwanrahman May 29, 2018

Hmm! I was considering testing the templates also, but maybe its out of the scope of the tests.
What do you think?

Copy link
Member

@ericholscher ericholscher May 30, 2018

Meaning the template context might change, and you want to test the actual page content? Seems like a good reason to not use the context.

We should at least create a helper method here, so we don't have the CSS classes copied all over the tests.

title = data['title']
query = title.split()[0]

resp = client.get(self.url, {'q': query, 'type': 'file'})
Copy link
Member

@ericholscher ericholscher May 29, 2018

You can test versions with a 'version': '', because we default to filtering by latest.

Copy link
Member Author

@safwanrahman safwanrahman May 29, 2018

But I wonder, while I search for a file, it should show me all the versions available. not only the latest one! why should I pass version: '' in order to see all the versions?
Is it how its designed or its a bug?

Copy link
Member

@ericholscher ericholscher May 30, 2018

Thats the proper behavior. Otherwise a default search will return the same file across multiple versions of the same project, which is not the best UX.

Copy link
Member Author

@safwanrahman safwanrahman Jun 3, 2018

As per our discussion over Skype, we need to find out a way to show all the versions list but filter by only the latest version

@@ -0,0 +1,9 @@
def pytest_addoption(parser):
parser.addoption('--including-search', action='store_true', dest="searchtests",
Copy link
Member

@ericholscher ericholscher May 29, 2018

We should document this in the testing docs: https://github.com/rtfd/readthedocs.org/blob/master/docs/tests.rst so that folks know about it.

Copy link
Member Author

@safwanrahman safwanrahman May 29, 2018

Yeah. documenting


@pytest.fixture(autouse=True)
def mock_elastic_index(mocker):
mocker.patch.object(Index, '_index', fake.word().lower())
Copy link
Member

@ericholscher ericholscher May 29, 2018

Do we need a third party package here? Could we just be generating a random index name? Or should the index name be the same for each test run perhaps?

Copy link
Member Author

@safwanrahman safwanrahman May 29, 2018

It seems like its not much necessary to have a third party package, but as we grow, we may need to generate fake data and which will be very much easier with the faker library.

Copy link
Member

@ericholscher ericholscher May 30, 2018

I think we can include it when needed. I don't want to continue to add dependencies for only 1 usage.

# patch the function from `projects.tasks` because it has been point to there
# http://www.voidspace.org.uk/python/mock/patch.html#where-to-patch
mocked_function = mocker.patch('readthedocs.projects.tasks.process_all_json_files')
mocked_function.side_effect = get_dummy_page_json
Copy link
Member

@ericholscher ericholscher May 29, 2018

I wonder if more of this logic should live in the actual test class file?

I originally missed this mocking because I was just looking at the test_views file, and not the conftest file.

Copy link
Member Author

@safwanrahman safwanrahman May 29, 2018

I think it should be in a fixture becasue if we write the logic in tests class file, we need to write it in every class.
As its in the conftest.py file, its availalble through the module and we do need this for almost all the tests of this search module.
pytest seems like a bit of magic for the fixture injection actually!

Copy link
Member

@ericholscher ericholscher May 30, 2018

👍


assert sorted(project_versions) == sorted(content_versions)

def test_file_search_subprojects(self, client, all_projects, search):
Copy link
Member Author

@safwanrahman safwanrahman Jun 1, 2018

@ericholscher I am trying somedays, but I can not make this test pass for some strange reason!
can you please take a look?

Copy link
Member Author

@safwanrahman safwanrahman Jun 2, 2018

It has been fixed! routing was the culprit! 😥

@safwanrahman safwanrahman force-pushed the search_test branch 3 times, most recently from f747543 to aaee53d Jun 2, 2018
@safwanrahman safwanrahman force-pushed the search_test branch 2 times, most recently from bee4945 to a400285 Jun 3, 2018
@safwanrahman
Copy link
Member Author

@safwanrahman safwanrahman commented Jun 3, 2018

@ericholscher I have completed (hopefully) writing test for all the scenario as well as user stories in #4121.
I think its ready for review now.

Copy link
Member

@ericholscher ericholscher left a comment

This looks great. I mostly commented on small code style and logic fixes. Once those are resolved, I think we can merge 👍


versions = project.versions.all()
# There should be only one version of the project
assert len(versions) == 1
Copy link
Member

@ericholscher ericholscher Jun 4, 2018

Not sure this should live in a helper method, so it doesn't seem like it will always be true.

Copy link
Member Author

@safwanrahman safwanrahman Jun 5, 2018

I think, it should not be here. I added it for some debug purpose. removing it

versions = project.versions.all()
# There should be only one version of the project
assert len(versions) == 1
print(query, page_num, data_type)
Copy link
Member

@ericholscher ericholscher Jun 4, 2018

This should probably use logging, or just not be here.

Copy link
Member Author

@safwanrahman safwanrahman Jun 5, 2018

It should not be here! mistake!


# But there should be 2 projects in the left side column
# as the query is preset in both projects
content = page.find('.navigable .project-list')
Copy link
Member

@ericholscher ericholscher Jun 4, 2018

I do worry about this breaking with a redesign, but I think it's OK for now.

# as the query is preset in both projects
content = page.find('.navigable .project-list')
if len(content) != 2:
pytest.xfail("failing because currently project list not show")
Copy link
Member

@ericholscher ericholscher Jun 4, 2018

This error could be improved. "not show" what?

Why is this an expected failure?

Copy link
Member Author

@safwanrahman safwanrahman Jun 5, 2018

So the thing is, though you are filtering by a project, you should able to see all the projects matching with the query in the project list. currently, its showing only one project, but it should show both.

subproject = all_projects[1]
project.add_subproject(subproject)
self._reindex_elasticsearch(es_index=es_index)
# Add another project as subproject of the project
Copy link
Member

@ericholscher ericholscher Jun 4, 2018

This should go above the logic



def get_search_query(project_slug, page_num=0, data_type='title'):
all_pages = DUMMY_PAGE_JSON[project_slug]
Copy link
Member

@ericholscher ericholscher Jun 4, 2018

Could use a docstring and perhaps a better name. Maybe get_title_of_project_page or something like this, which specifies a bit more what it's doing.

Copy link
Member Author

@safwanrahman safwanrahman Jun 5, 2018

Yeah. I think naming it something like get_search_query_from_project_file would be better. because its not only getting the title. Its getting as per the data_type provided to the function.
I will add docstring surely.

@agjohnson agjohnson removed their request for review Jun 4, 2018
@safwanrahman
Copy link
Member Author

@safwanrahman safwanrahman commented Jun 5, 2018

@ericholscher I have fixed according the review and more 2 tests. Can you check them and merge?

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Jun 5, 2018

Looks great to me!

@ericholscher ericholscher merged commit b10c493 into readthedocs:master Jun 5, 2018
1 check passed
Search update automation moved this from In progress to Done Jun 5, 2018
@safwanrahman safwanrahman deleted the search_test branch Jun 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Search update
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants