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

Conversation

6 participants
@safwanrahman
Member

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

This comment has been minimized.

@stsewd

stsewd May 19, 2018

Member

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.

This comment has been minimized.

@safwanrahman

safwanrahman May 19, 2018

Member

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

safwanrahman added some commits May 19, 2018

@safwanrahman safwanrahman referenced this pull request May 21, 2018

Closed

Search testing #3825

@ericholscher

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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

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 added some commits May 21, 2018

@agjohnson agjohnson added this to the Search improvements milestone May 22, 2018

@robhudson

This comment has been minimized.

Contributor

robhudson commented May 24, 2018

I think this is a great start too! I'll make one suggestion based on working with and testing elasticsearch in other projects...

Make a small corpus of hand written test documents for searching, e.g. 5 projects with data associated with each, not using automatically generated data. Give them close to real names and just make up some stuff. That helps think about the queries a lot easier but also allows to test that the search finds the right result out of many vs adding 1 project and testing search finds the only one there.

I don't recall if there's any fuzzy matching for catching typos but having a few hard coded projects helps test this kind of thing also.

safwanrahman added some commits May 27, 2018

@safwanrahman safwanrahman changed the title from [WIP] Test for search functionality to Test for search functionality May 27, 2018

@safwanrahman

This comment has been minimized.

Member

safwanrahman commented May 27, 2018

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

@ericholscher

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]

This comment has been minimized.

@ericholscher

ericholscher May 29, 2018

Member

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

This comment has been minimized.

@safwanrahman

safwanrahman May 29, 2018

Member

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

This comment has been minimized.

@ericholscher

ericholscher May 29, 2018

Member

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.

This comment has been minimized.

@safwanrahman

safwanrahman May 29, 2018

Member

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

This comment has been minimized.

@ericholscher

ericholscher May 30, 2018

Member

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'})

This comment has been minimized.

@ericholscher

ericholscher May 29, 2018

Member

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

This comment has been minimized.

@safwanrahman

safwanrahman May 29, 2018

Member

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?

This comment has been minimized.

@ericholscher

ericholscher May 30, 2018

Member

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.

This comment has been minimized.

@safwanrahman

safwanrahman Jun 3, 2018

Member

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",

This comment has been minimized.

@ericholscher

ericholscher May 29, 2018

Member

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.

This comment has been minimized.

@safwanrahman

safwanrahman May 29, 2018

Member

Yeah. documenting

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

This comment has been minimized.

@ericholscher

ericholscher May 29, 2018

Member

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?

This comment has been minimized.

@safwanrahman

safwanrahman May 29, 2018

Member

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.

This comment has been minimized.

@ericholscher

ericholscher May 30, 2018

Member

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

This comment has been minimized.

@ericholscher

ericholscher May 29, 2018

Member

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.

This comment has been minimized.

@safwanrahman

safwanrahman May 29, 2018

Member

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!

This comment has been minimized.

@ericholscher
assert sorted(project_versions) == sorted(content_versions)
def test_file_search_subprojects(self, client, all_projects, search):

This comment has been minimized.

@safwanrahman

safwanrahman Jun 1, 2018

Member

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

This comment has been minimized.

@safwanrahman

safwanrahman Jun 2, 2018

Member

It has been fixed! routing was the culprit! 😥

safwanrahman added some commits Jun 1, 2018

@safwanrahman

This comment has been minimized.

Member

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.

@ericholscher

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

This comment has been minimized.

@ericholscher

ericholscher Jun 4, 2018

Member

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

This comment has been minimized.

@safwanrahman

safwanrahman Jun 5, 2018

Member

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)

This comment has been minimized.

@ericholscher

ericholscher Jun 4, 2018

Member

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

This comment has been minimized.

@safwanrahman

safwanrahman Jun 5, 2018

Member

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

This comment has been minimized.

@ericholscher

ericholscher Jun 4, 2018

Member

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

This comment has been minimized.

@ericholscher

ericholscher Jun 4, 2018

Member

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

Why is this an expected failure?

This comment has been minimized.

@safwanrahman

safwanrahman Jun 5, 2018

Member

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

This comment has been minimized.

@ericholscher

ericholscher Jun 4, 2018

Member

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]

This comment has been minimized.

@ericholscher

ericholscher Jun 4, 2018

Member

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.

This comment has been minimized.

@safwanrahman

safwanrahman Jun 5, 2018

Member

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

This comment has been minimized.

Member

safwanrahman commented Jun 5, 2018

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

@ericholscher

This comment has been minimized.

Member

ericholscher commented Jun 5, 2018

Looks great to me!

@ericholscher ericholscher merged commit b10c493 into rtfd:master Jun 5, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Search update automation moved this from In progress to Done Jun 5, 2018

@safwanrahman safwanrahman deleted the safwanrahman:search_test branch Jun 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment