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

Have more control over search tests #6644

Merged
merged 8 commits into from Feb 25, 2020

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Feb 11, 2020

  • Allow us to override the request to add HTTP_HOST.
  • Don't modify the markers at run time (this doesn't play nice with .com, so now we can run the search tests in .com)

With the second change there is a design decision. Now tox -e py36 will run the search tests, and they will fail if users don't have ES installed. I can modify it so tests only run the search tests on travis (kind of hacky), or I can update the docs about how to run the tests to mention to run tox -e py36 -- -m 'no search' if you don't have ES installed.

@stsewd stsewd added the Needed: design decision label Feb 12, 2020
@stsewd stsewd requested a review from Feb 12, 2020
@ericholscher
Copy link
Member

@ericholscher ericholscher commented Feb 12, 2020

I'd like to default to no search, and have it documented how to run tests with search.

@stsewd stsewd removed the Needed: design decision label Feb 13, 2020
@stsewd
Copy link
Member Author

@stsewd stsewd commented Feb 13, 2020

Done, by default posargs will have -m 'not search'. It can be overridden with the normal flow using tox -- additional posargs or using TOX_POSARGS.

@@ -39,7 +42,7 @@ def test_search_works_with_title_query(self, api_client, project, page_num):
'version': version.slug,
'q': query
}
resp = api_client.get(self.url, search_params)
resp = self.get_search(api_client, search_params)
Copy link
Member

@ericholscher ericholscher Feb 25, 2020

Choose a reason for hiding this comment

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

I don't understand this refactor -- it seems to just make it less clear what's happening?

Copy link
Member Author

@stsewd stsewd Feb 25, 2020

Choose a reason for hiding this comment

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

@@ -39,7 +42,7 @@ def test_search_works_with_title_query(self, api_client, project, page_num):
'version': version.slug,
'q': query
}
resp = api_client.get(self.url, search_params)
resp = self.get_search(api_client, search_params)
Copy link
Member

@ericholscher ericholscher Feb 25, 2020

Choose a reason for hiding this comment

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

I don't understand this refactor -- it seems to just make it less clear what's happening?

Copy link
Member

@humitos humitos left a comment

I'm +1 on having more control over search tests, but I'd like to have better defaults that helps us in the day to day but also being compatible with travis to run all tests by default.

If you need to override tox's posargs, but you still don't want to run the search tests,
you need to include ``-m 'not search'`` to your command::

tox -- -m 'not search' -x
Copy link
Member

@humitos humitos Feb 25, 2020

Choose a reason for hiding this comment

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

I liked the idea of using the --including-search that was before this PR, which by default added -m 'not search' automatically by inspecting the -m argument first.

If I understand correctly, now when running tox -- -k mytest we will be forced to add -m 'not search' as well.

Copy link
Member

@humitos humitos Feb 25, 2020

Choose a reason for hiding this comment

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

Can we keep this behavior?

Copy link
Member Author

@stsewd stsewd Feb 25, 2020

Choose a reason for hiding this comment

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

I wasn't able to make it work with .com. And if tox -- -k mytest doesn't match a test from search you don't have to add -m 'not search'

If you are testing inside :ref:`docker compose <development/standards:Working with Docker Compose>`,
you'll need to override the ``ES_HOSTS`` and ``ELASTICSEARCH_DSL`` settings to point to ``search:9200`` instead.
Copy link
Member

@humitos humitos Feb 25, 2020

Choose a reason for hiding this comment

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

Why this is not set by default when running inside docker?

Copy link
Member Author

@stsewd stsewd Feb 25, 2020

Choose a reason for hiding this comment

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

We use the same settings file to run tests outside and inside docker, so if we change that we need to modify the hosts file on travis (not sure if we can do that in a simple way).

Do we have a way of knowing if we are running inside docker?

Copy link
Member Author

@stsewd stsewd Feb 25, 2020

Choose a reason for hiding this comment

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

We use the same settings file to run tests outside and inside docker, so if we change that we need to modify the hosts file on travis (not sure if we can do that in a simple way).

Do we have a way of knowing if we are running inside docker?

Copy link
Member Author

@stsewd stsewd Feb 25, 2020

Choose a reason for hiding this comment

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

Looks like we can set a custom host on travis https://docs.travis-ci.com/user/hosts/

Copy link
Member Author

@stsewd stsewd Feb 25, 2020

Choose a reason for hiding this comment

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

Are we ok with setting the defaults to search:9200?

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Feb 25, 2020

I'd like to merge this, to get the search proxy stuff working for .com -- let's create issues for any unaddressed things @humitos

@ericholscher ericholscher merged commit a2004ed into readthedocs:master Feb 25, 2020
2 checks passed
@stsewd stsewd deleted the run-search-tests branch Feb 25, 2020
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.

None yet

3 participants