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

Optimize reorder_items in fixtures.py #3108

Merged
merged 4 commits into from
Jan 28, 2018

Conversation

cheezman34
Copy link

I've precached a map of fixtures to items that allows us to avoid iterating over the entire list of items for each fixture. It also reduces the number of lists created. On my project with a few thousand parametrized fixtures, test collection now takes ~2 seconds instead of ~20 seconds.

@RonnyPfannschmidt
Copy link
Member

at first glance this looks fabulous, unfortunately i wont be able to do deep review until next week

@cheezman34
Copy link
Author

Ok! Let me know if you have any questions/comments/suggestions whenever you get around to it.

@@ -0,0 +1 @@
Optimize reorder_items in fixtures.py.
Copy link
Member

Choose a reason for hiding this comment

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

This isn't really an useful changelog for a user of pytest to read, IMHO. What about something like:

Improve performance when collecting tests using many fixtures.

or so?

Copy link
Author

Choose a reason for hiding this comment

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

Sounds good to me.

Copy link
Author

Choose a reason for hiding this comment

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

@The-Compiler I added the requested change, is there anything else I need to do? I'm not an experienced githubber.

Copy link
Member

Choose a reason for hiding this comment

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

No, other than wait for someone to review this. I unfortunately don't have the time right now, but probably others will take a look soon 😄

@coveralls
Copy link

Coverage Status

Coverage increased (+0.002%) to 92.613% when pulling d314691 on cheezman34:features into b0032ba on pytest-dev:features.

@nicoddemus
Copy link
Member

Thanks a lot @cheezman34!

I've tried to come up with an example (for record purposes) that shows the performance improvement you see, but I did not quite manage to.

Here's the script I used:

import pytest

TESTS_COUNT = 20
AUTO_USE = True


def make_fix(c):
    @pytest.fixture(scope='session', params=range(2), autouse=AUTO_USE)
    def fix(request):
        pass

    fix.__name__ = 'fix%d' % c
    return fix


class Test:

    @pytest.mark.parametrize('i', range(TESTS_COUNT))
    def test(self, i):
        pass


for i in range(10):
    f = make_fix(i)
    setattr(Test, f.__name__, f)

del f

Here are the timings I got:

TESTS_COUNT=20:

master cheezman34:features
AUTO_USE=True 3.417s 3.516s
AUTO_USE=False 0.532s 0.486s

TESTS_COUNT=200:

master cheezman34:features
AUTO_USE=True 30.440s 31.781s
AUTO_USE=False 0.494s 0.496s

Can you post a script which shows the performance improvement or point out what I'm missing on my tests? Thanks!

(Don't get me wrong, I'm sure your code is an improvement, I would just like to get some code on record that demonstrates it that's all 😁)

@cheezman34
Copy link
Author

cheezman34 commented Jan 27, 2018

import pytest

@pytest.fixture(scope="module", params=range(2000))
def things(request):
    pass

def test_stuff(things):
    pass

If I run this from the command line with collect-only, it takes about 1 second with optimized code, but 4.8 seconds with the old code.

Assuming this is in a file named "test.py", the command line call might be something like:
python -m pytest test.py --collect-only

@nicoddemus
Copy link
Member

nicoddemus commented Jan 28, 2018

Awesome @cheezman34, thanks! Indeed this made the collection time go from 14s to 1.2s parametrizing 6k fixtures on my computer. 😁

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.

5 participants