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

Load-Distribute test cases by filename #242

Merged
merged 5 commits into from
Dec 22, 2017

Conversation

criemen
Copy link
Contributor

@criemen criemen commented Nov 9, 2017

Hi,
this PR includes a new way of load-distributing tests by the file they reside in.
The implementation is a very simple change of LoadScopeScheduling.

The reason to have this is that in a project I'm working on, there is a single shared ressource per test file, which can't be used concurrently. Unfortunately, just scheduling the tests in one class on the same executor does not help, as there are different test classes, representing different configurations of the shared ressource in each file.
Thus, the proposed LoadFileScheduling.

  • We use towncrier for changelog management, so please add a news file into the changelog folder following these guidelines:
    • Name it $issue_id.$type for example 588.bugfix;

    • If you don't have an issue_id change it to the PR id after creating it

    • Ensure type is one of removal, feature, bugfix, vendor, doc or trivial

    • Make sure to use full sentences with correct case and punctuation, for example:

      Fix issue with non-ascii contents in doctest text files.
      

@criemen
Copy link
Contributor Author

criemen commented Nov 9, 2017

The failing tests also fail on master.

@nicoddemus
Copy link
Member

First of all thanks @Corni for the contribution!

The reason to have this is that in a project I'm working on, there is a single shared ressource per test file, which can't be used concurrently.

I see. Perhaps the recent loadscope mode could be used for that? Your tests just need to access the resource through a scope="module" fixture. This is more flexible because you even organize the resource access by classes instead using scope="class" fixture as well.

@criemen
Copy link
Contributor Author

criemen commented Nov 10, 2017

Hi,

unfortunately, this does not help.
Let me explain my problem a bit better:
All tests in test_redis.py access a (single, shared) redis-server instance provided by travis.
Before and after test cases are run, the entire content of the redis database is cleared, so different tests don't affect each other.
This does not work when tests from this file are run concurrently. Using a scope="module" fixture does not help either, because module-level fixtures unfortunately do not affect scheduling (I think this would be #18 ), but are re-instantiated on each worker.

We also do need several classes in the file, because we have a common body of tests we run in a shared superclass, which we then instantiate with different configurations, potentially overriding some tests to adjust for the different configuration. Yet, all of these use the shared redis instance.

In the end, I would agree that this PR is a cheaper workaround around not having #18 implemented, a bit like loadscope is as well.

@nicoddemus
Copy link
Member

This does not work when tests from this file are run concurrently. Using a scope="module" fixture does not help either, because module-level fixtures unfortunately do not affect scheduling (I think this would be #18 ), but are re-instantiated on each worker.

Actually using a scope='module' fixture together with loadscope mode should schedule all tests that use that fixture to be scheduled to the same worker. You are not seeing that happening? All you needs is a single dummy auto-use fixture:

# test_redis.py
@pytest.fixture(autouse=True, scope='module')
def force_run_in_single_worker():
    pass

Now run with pytest -n auto --dist=loadscope.

Alternatively, for your specific user case specifically, where you have a single test_redis.py file which cannot run in concurrently, you can run the test suite in two batches:

  1. Pass -n auto -k -test_redis.py: this excludes the entire test_redis.py from the run.
  2. Pass -n 0 -k test_redis.py, this will run only the test_redis.py file, non-concurrently.

But I believe the solution using loadscope really should solve your problem.

Sorry if it appears I'm rejecting your patch outright, I just want to make sure we understand the problem and perhaps propose an alternative solution first.

@criemen
Copy link
Contributor Author

criemen commented Nov 13, 2017

Hi,

I'm totally fine with this patch being rejected if the functionality is there, I understand the goal of keeping the codebase small and maintainable.

Unfortunately, while I would be fine with the first solution (implicit module-level fixture), it doesn't work in my tests.
My test file:

import pytest

@pytest.fixture(autouse=True, scope="module")
def fix():
    pass


class TestCaseA:
    def test_one(self, fix):
        assert True


class TestCaseB:
    def test_two(self, fix):
        assert True

Which I start via:
py.test -n2 --dist=loadscope -v test.py

Which gives the following output, indicating that TestCaseA and TestCaseB have been scheduled to different executors, right?

============================================================ test session starts ============================================================
platform darwin -- Python 3.6.1, pytest-3.2.3, py-1.4.34, pluggy-0.4.0 -- /Users/criemenschneider/venv-py36/bin/python
cachedir: .cache
rootdir: /Users/criemenschneider/PycharmProjects/pytest-xdist, inifile: tox.ini
plugins: xdist-1.20.2.dev9+g97507e7.d20171113, forked-0.2
[gw0] darwin Python 3.6.1 cwd: /Users/criemenschneider/PycharmProjects/pytest-xdist
[gw1] darwin Python 3.6.1 cwd: /Users/criemenschneider/PycharmProjects/pytest-xdist
[gw0] Python 3.6.1 (v3.6.1:69c0db5050, Mar 21 2017, 01:21:04)  -- [GCC 4.2.1 (Apple Inc. build 5666) (dot 3)]
[gw1] Python 3.6.1 (v3.6.1:69c0db5050, Mar 21 2017, 01:21:04)  -- [GCC 4.2.1 (Apple Inc. build 5666) (dot 3)]
gw0 [2] / gw1 [2]
scheduling tests via LoadScopeScheduling

test.py::TestCaseB::test_two
test.py::TestCaseA::test_one
[gw1] PASSED test.py::TestCaseB::test_two
[gw0] PASSED test.py::TestCaseA::test_one

========================================================= 2 passed in 0.90 seconds ==========================================================

The alternate solution is, unfortunately, a non-solution for my case, as nearly all other test files suffer from the same problem, just the shared ressource (redis in the example) is a MySQL-server, a Postgre-server etc.

@nicoddemus
Copy link
Member

My bad, that's not how loadscope works; the idea of using fixtures is one we had, but it is different than what loadscope provides.

loadscope will automatically group all test functions and test methods to the same workers, automatically. In your example, all test methods of each class will be sent to separate workers. If you had a single class, then all the methods will be sent to the same worker:

class TestCaseA:
    def test_one(self, fix):
        assert True

    def test_two(self, fix):
        assert True
test_foo.py::TestCaseA::test_one
[gw0] PASSED test_foo.py::TestCaseA::test_one
test_foo.py::TestCaseA::test_two
[gw0] PASSED test_foo.py::TestCaseA::test_two

How are your tests organized inside the test files? Are they plain test classes or unittest-style classes?

@nicoddemus
Copy link
Member

Having said that, the new addition is pretty trivial in terms of code and maintainability, we should consider adding it.

@RonnyPfannschmidt what's your opinion on this?

@criemen
Copy link
Contributor Author

criemen commented Nov 13, 2017

Hi,

the test classes are plain test classes.
You can have a look for yourself, see https://github.com/mbr/simplekv/blob/master/tests/test_redis.py
Because we need to setup the store() fixture in a different way, but then run the same tests with it, it is unfortunately not possible to have just one test class in the file, thus leveraging the loadscope functionality.

@criemen
Copy link
Contributor Author

criemen commented Dec 15, 2017

ping - the failing tests also fail on master.

@nicoddemus
Copy link
Member

Thanks @Corni for the ping!

Unless @RonnyPfannschmidt needs more time to review this, I will look into merging this next week and make a release! 👍

@RonnyPfannschmidt
Copy link
Member

@nicoddemus please go ahead

@nicoddemus
Copy link
Member

@Corni I merged with the latest master and added some quick docs to the README.

@nicoddemus nicoddemus merged commit 3f09e15 into pytest-dev:master Dec 22, 2017
@nicoddemus
Copy link
Member

@Corni 1.21.0 has been released, thanks again for the PR!

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.

4 participants