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

Make tests extensible from corporate site #4095

Merged
merged 28 commits into from Jul 2, 2018
Merged

Conversation

@humitos
Copy link
Member

@humitos 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 readthedocs/readthedocs-corporate#329
Related readthedocs/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'
Copy link
Member Author

@humitos humitos May 15, 2018

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.

Copy link
Contributor

@agjohnson agjohnson Jun 8, 2018

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)

Copy link
Member Author

@humitos humitos Jun 28, 2018

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
Copy link
Member Author

@humitos humitos May 15, 2018

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

Copy link
Contributor

@agjohnson agjohnson Jun 8, 2018

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?

Copy link
Member Author

@humitos humitos Jun 28, 2018

I renamed it to just community

@humitos humitos requested a review from May 23, 2018
@humitos
Copy link
Member Author

@humitos 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.

@humitos humitos force-pushed the humitos/corporate/tests branch from 10b6d53 to 65a9ebb Jun 6, 2018
Copy link
Contributor

@agjohnson agjohnson left a comment

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')
Copy link
Contributor

@agjohnson agjohnson Jun 8, 2018

Why is this change required?

Copy link
Member Author

@humitos humitos Jun 18, 2018

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)
Copy link
Contributor

@agjohnson agjohnson Jun 8, 2018

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

Copy link
Member Author

@humitos humitos Jun 28, 2018

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
Copy link
Contributor

@agjohnson agjohnson Jun 8, 2018

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'):
Copy link
Contributor

@agjohnson agjohnson Jun 8, 2018

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?

Copy link
Member Author

@humitos humitos Jun 28, 2018

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'
Copy link
Contributor

@agjohnson agjohnson Jun 8, 2018

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')
Copy link
Contributor

@agjohnson agjohnson Jun 8, 2018

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 humitos force-pushed the humitos/corporate/tests branch from a8ca72b to e6c879d Jun 28, 2018
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.
Copy link
Member Author

@humitos humitos Jun 28, 2018

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.

Copy link
Member Author

@humitos humitos Jun 28, 2018

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

Copy link
Contributor

@agjohnson agjohnson Jul 2, 2018

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.

Copy link
Member Author

@humitos humitos Jul 2, 2018

I opened #4317 for this


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

@humitos humitos Jun 28, 2018

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.

Copy link
Member Author

@humitos humitos Jul 2, 2018

These marks are not needed anymore :D

@humitos
Copy link
Member Author

@humitos 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 force-pushed the humitos/corporate/tests branch from 16728be to 7c2ec2e Jun 28, 2018
humitos added 6 commits Jun 28, 2018
Since this is executed even when the test is skipped, we need to use
the ``_lazy`` function.
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.
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.
These tests are excluded from the corporate site using pytest internals.
Copy link
Contributor

@agjohnson agjohnson left a comment

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.
Copy link
Contributor

@agjohnson agjohnson Jul 2, 2018

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
Copy link
Member Author

@humitos 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
@humitos humitos deleted the humitos/corporate/tests branch Jul 2, 2018
@ericholscher
Copy link
Member

@ericholscher ericholscher commented Jul 3, 2018

🎆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants