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

Make tests extensible from corporate site #4095

Merged
merged 28 commits into from Jul 2, 2018

Conversation

Projects
None yet
3 participants
@humitos
Member

humitos commented May 15, 2018

This PR allows us to import the community test suite under the corporate test suite and extend them as well as skip some test cases that are specific for the community site and we don't need in the corporate project.

Related rtfd/readthedocs-corporate#329
Related rtfd/readthedocs-corporate#330

class CoreTagsTests(TestCase):
fixtures = ["eric", "test_data"]
# Determine if we are under community (http) or corporate site (https)
scheme = 'https' if settings.CELERY_APP_NAME == 'readthedocsinc' else 'http'

This comment has been minimized.

@humitos

humitos May 15, 2018

Member

I don't like this.

We should find a different way to know where we are here to decide the user of http or https.

This comment has been minimized.

@agjohnson

agjohnson Jun 8, 2018

Contributor

Same. Perhaps there is a setting we could use or add? This is very fragile and not something we should reference in the .org code (we don't ever reference our commercial hosting code from the community side)

This comment has been minimized.

@humitos

humitos Jun 28, 2018

Member

I'm using conftest.py to set these options. Example,

PYTEST_OPTIONS = (
    # Include `not community` to parameters so that community tests do not run
    ('markexpr', 'not community'),

    # Options to set test environment
    ('community', False),
    ('environment', 'readthedocsinc'),

    ('url_scheme', 'https'),
)


def pytest_configure(config):
    for option, value in PYTEST_OPTIONS:
        setattr(config.option, option, value)

So, this way in the test case I'm doing pytest.config.option.url_scheme which will be https in the corporate site and http in the community.

This way, we will have all the test options under this file and easy to tweak/modify/understand where they come from.

@@ -102,6 +105,7 @@ def test_canonical_change(self):
self.assertEqual(domain.domain, 'example2.com')
@pytest.mark.only_community

This comment has been minimized.

@humitos

humitos May 15, 2018

Member

We are running pytest -m "not only_community" from the corporate project.

This comment has been minimized.

@agjohnson

agjohnson Jun 8, 2018

Contributor

This is 100% trivial, but only_community is mildly unnatural for english in this context, its clearer to me written in reverse: community_only. Perhaps just community as the tag?

This comment has been minimized.

@humitos

humitos Jun 28, 2018

Member

I renamed it to just community

@humitos humitos requested a review from rtfd/core May 23, 2018

@humitos

This comment has been minimized.

Member

humitos commented May 23, 2018

Although this is a work in progress PR since there are more work to do, I'd like to have some feedback from the Core Team to know if I'm going in the right direction. There are more comments/ideas in the related/linked PRs.

@agjohnson

Most of the changes look good so far. I'm not 100% certain on the use of a tag like only_community to skip these. I think something more explicit or descriptive is better.

For instance, perhaps tests should mark xfail on a condition:

@pytest.mark.xfail(not pytest.config.getvalue("community"),
                    reason="Overridden logic on the queryset can cause this to fail")
def test_case_something_something(...):
    pass

Again, this is all a little weird as we should not be referencing commercial code in any of this. That would be really confusing for contributors, as they will never be able to see this code.

Alternatively, inside the test was can xfail with even more specificity.

A few of the patterns used in this PR can be replaced with more pytest internals -- such as replacing any mention of readthedocsinc and such with a pytest config option.

@@ -9,10 +11,26 @@
from readthedocs.core.templatetags import core_tags
@override_settings(USE_SUBDOMAIN=False, PUBLIC_DOMAIN='readthedocs.org')
@override_settings(USE_SUBDOMAIN=False, PRODUCTION_DOMAIN='readthedocs.org')

This comment has been minimized.

@agjohnson

agjohnson Jun 8, 2018

Contributor

Why is this change required?

This comment has been minimized.

@humitos

humitos Jun 18, 2018

Member

It was wrong. The test wants to use PRODUCTION_DOMAIN not PUBLIC_DOMAIN.

It's using USE_SUBDOMAIN=False, so the URL used for the documentation comes from PRODUCTION_DOMAIN. If you change the value override of PUBLIC_DOMAIN here to something different, all the tests keep passing which is wrong.

pip_abc_xyz_document = '{scheme}://readthedocs.org/docs/pip/en/abc/index.html#document-xyz'.format(scheme=scheme)
pip_abc_xyz_fr_document = '{scheme}://readthedocs.org/docs/pip/fr/abc/index.html#document-xyz'.format(scheme=scheme)
pip_latest_document_url = '{scheme}://readthedocs.org/docs/pip/en/latest/document/'.format(scheme=scheme)
pip_latest_document_page_url = '{scheme}://readthedocs.org/docs/pip/en/latest/document.html'.format(scheme=scheme)

This comment has been minimized.

@agjohnson

agjohnson Jun 8, 2018

Contributor

instead of duplicating a lot of text here, we could reduce this section to just reuse a common base. that is:

url_base = '{scheme}://{production_domain}/docs/pip'

def test_something(self):
    ...
    self.assertEqual(url, self.url_base + '/en/latest/')

This comment has been minimized.

@humitos

humitos Jun 28, 2018

Member

I did something similar than what you are proposing here. Not exactly the same, though.

@@ -102,6 +105,7 @@ def test_canonical_change(self):
self.assertEqual(domain.domain, 'example2.com')
@pytest.mark.only_community

This comment has been minimized.

@agjohnson

agjohnson Jun 8, 2018

Contributor

This is 100% trivial, but only_community is mildly unnatural for english in this context, its clearer to me written in reverse: community_only. Perhaps just community as the tag?

Return the absolute path of the ``readthedocs`` app.
"""
path = getcwd()
if path.endswith('readthedocsinc'):

This comment has been minimized.

@agjohnson

agjohnson Jun 8, 2018

Contributor

This is also fragile and references commercial code that won't exist on the community side (other contributors will be puzzled by this). Perhaps there is another flag or something we can use from pytest here? Can we check to see if the tag community_only exists here maybe? Or is there a more general tag we could be using from pytest or settings?

This comment has been minimized.

@humitos

humitos Jun 28, 2018

Member

I just used:

import readthedocs
readthedocs.__path__[0]

which works not matter where you have the readthedocs module installed.

class CoreTagsTests(TestCase):
fixtures = ["eric", "test_data"]
# Determine if we are under community (http) or corporate site (https)
scheme = 'https' if settings.CELERY_APP_NAME == 'readthedocsinc' else 'http'

This comment has been minimized.

@agjohnson

agjohnson Jun 8, 2018

Contributor

Same. Perhaps there is a setting we could use or add? This is very fragile and not something we should reference in the .org code (we don't ever reference our commercial hosting code from the community side)

path = getcwd()
if path.endswith('readthedocsinc'):
# readthedocs-corporate
path = pjoin(path, '..', '..', 'readthedocs.org', 'readthedocs')

This comment has been minimized.

@agjohnson

agjohnson Jun 8, 2018

Contributor

Also, -1 on this pattern in general. We need to stop making assumptions that these two repos are siblings in a similar path.

@agjohnson agjohnson added this to the 2.7 milestone Jun 8, 2018

humitos added some commits May 15, 2018

Mark some OAuth tests as "only_community"
These tests won't be ran when imported from the corporate site.
Split project tests in classes by topic
All the translation tests manage different user permissions on each
different project, which is more complicated to test them from
Corporate. So, they are marked as `only_community`
Define the URL inside setUp because it fails when loading the test
When loading this test from outside (readthedocsinc) the ``reverse``
can't be calculated because the ``search`` name is not defined in the URLConf.
Use /dashboard/ instead of / to avoid redirects
Accessing ``/`` while logged in redirects to ``/dashboard/`` in the
corporate site. This change makes this test compatible with both environments
try:
# TODO: this file is read/executed even when called from ``readthedocsinc``,
# so it's overriding the options that we are defining in the ``conftest.py``
# from the corporate site. We need to find a better way to avoid this.

This comment has been minimized.

@humitos

humitos Jun 28, 2018

Member

conftest.py is directory dependent. That's why we can't override these pytest's options here. I'd like to find a better way to do this.

This comment has been minimized.

@humitos

humitos Jun 28, 2018

Member

Does it make sense to define these PYTEST_OPTIONS in the settings.py file?

This comment has been minimized.

@agjohnson

agjohnson Jul 2, 2018

Contributor

Perhaps this would be a good addition. Alternatively, maybe something in setup.cfg instead? I'm not sure if the load order would allow us to use django settings, given pytest-django likely sets up the application later in the process. We can make an issue to track updating this pattern.

This comment has been minimized.

@humitos

humitos Jul 2, 2018

Member

I opened #4317 for this

# Most of these tests don't handle Corporate permissions (Organizations, Teams,
# etc) in a proper way
@pytest.mark.community

This comment has been minimized.

@humitos

humitos Jun 28, 2018

Member

Do we want to include these comments as "reason"? Do we prefer the reason= argument? Should we remove all of these comments from this repo?

I like to explain the why. I did that in the conftest.py from the corporate when I'm ignoring a whole .py file, but I'd like to do something similar when I'm excluding only one/several tests from a file that it's not completely ignored.

This comment has been minimized.

@humitos

humitos Jul 2, 2018

Member

These marks are not needed anymore :D

@humitos

This comment has been minimized.

Member

humitos commented Jun 28, 2018

At this point, I don't want to make the whole suite to be executed in both repositories and pass. I think that marking all community test as xfail does not add value since they are not thought to fail for any specific reason. I'd say that we can think a better way for those tests and later and improve what we have.

I had to modify a couple of tests to be compatible with both repositories and I found some patterns that we will need to follow when writing new tests in the community site:

  • do not base our tests in default value of settings. If we need a setting with a specific value, we should use override_setting for the setting's name and value we want (example, PRODUCTION_DOMAIN)
  • be careful when defining class level variables. There are some values that can't be calculated because the configs differs between different projects (example, reverse('urlname'))

@humitos humitos requested a review from agjohnson Jun 28, 2018

humitos added some commits Jun 28, 2018

Use reverse_lazy to avoid issues
Since this is executed even when the test is skipped, we need to use
the ``_lazy`` function.
Depend on reliable data from the db for these tests
This Feature is created inside a db migration. As we depend strictly
on this Feature we can't rely on data from the migration since it's
ran only once when the db is created and not re-populated on each
test.

We need to be sure that the data we rely on is in the database in a
valid state and that it wasn't modified/deleted by any other test
case. Because of this, we need to use a fixture or delete and create
the data we rely on inside the ``setUp`` for this test.
Mark building test as community only
These tests performs a call to our API from a signal hook in our
corporate site and the API call in that file is not mocked, so they fail.
Remove all community marks from pytests
These tests are excluded from the corporate site using pytest internals.
@agjohnson

I like this pattern, besides overriding PYTEST_OPTIONS, which we'll find a better way to do later, this is all really clean on both sets of code. 👍

try:
# TODO: this file is read/executed even when called from ``readthedocsinc``,
# so it's overriding the options that we are defining in the ``conftest.py``
# from the corporate site. We need to find a better way to avoid this.

This comment has been minimized.

@agjohnson

agjohnson Jul 2, 2018

Contributor

Perhaps this would be a good addition. Alternatively, maybe something in setup.cfg instead? I'm not sure if the load order would allow us to use django settings, given pytest-django likely sets up the application later in the process. We can make an issue to track updating this pattern.

@humitos

This comment has been minimized.

Member

humitos commented Jul 2, 2018

I'm merging this. We can follow that TODO in the issue I created.

@humitos humitos merged commit f895af5 into master Jul 2, 2018

1 check passed

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

@humitos humitos deleted the humitos/corporate/tests branch Jul 2, 2018

@ericholscher

This comment has been minimized.

Member

ericholscher commented Jul 3, 2018

🎆

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