Skip to content

Comments

Speedup tests#1144

Merged
leplatrem merged 3 commits intomasterfrom
speed-up-tests
Mar 8, 2017
Merged

Speedup tests#1144
leplatrem merged 3 commits intomasterfrom
speed-up-tests

Conversation

@leplatrem
Copy link
Contributor

@leplatrem leplatrem commented Mar 8, 2017

I hadn't run the whole tests suite locally for a while. It became incredibly slow.

The Kinto app is initialized in every test method of the views tests (via __init__) and kinto.main() takes from 0.2 to 0.4 sec (!!). I tried to dig in and could not find obvious bottleneck. So I went for a more radical change: why do we have to initialize the app in every test whereas once per test case would be enough?

I hence moved app initialization to setUpClass() and the first attempt was quite successful:

Before:

$ py.test -k tests/test_views_
Results (94.58s):
     200 passed

After:

$ py.test -k tests/test_views_
Results (21.61s):
     200 passed

The current approach introduces a breaking change since get_app_settings() is now a classmethod. But we could find a hack to introspect the test class and pick the test instance method if available (to avoid migrating every plugin just now).

@leplatrem
Copy link
Contributor Author

Results (158.43s):
    1715 passed

@gabisurita
Copy link
Member

gabisurita commented Mar 8, 2017

Time is not the only issue but specially memory. Pytest design doesn't free the memory from a test after it's completed which can make travis go on swap. That's why the tests were erroring in #1138.

I had similar issues and discussed this with @glasserc on #977. This should have been partially fixed on pytest-dev/pytest#2046, but apparently they only freed pytest own objects and left test specific objects.

Some cross referencing:
pytest-dev/pytest#1649
pytest-dev/pytest#2046
http://stackoverflow.com/questions/20900380/py-test-excessive-memory-usage-with-large-number-of-tests

@leplatrem leplatrem changed the title [WIP] Speedup tests Speedup tests Mar 8, 2017
@leplatrem
Copy link
Contributor Author

On Travis, the total time goes from 37 min to 11 min.

Copy link
Member

@gabisurita gabisurita left a comment

Choose a reason for hiding this comment

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

Overall I think this is a good improvement especially to make tests faster, but I do think we need also to reiterate that the memory problem is also a bug on pytest.


cls.spec_dict = app.get('/__api__').json
super().setUpClass()
cls.spec_dict = cls.app.get('/__api__').json
Copy link
Member

Choose a reason for hiding this comment

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

That's what I've mentioned 👍

@leplatrem leplatrem merged commit a0b3146 into master Mar 8, 2017
@leplatrem leplatrem deleted the speed-up-tests branch March 8, 2017 15:13
@glasserc
Copy link
Contributor

I think this is a good change even disregarding pytest behavior. Repeated initialization isn't really valuable for us.

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