search

ruflin / Elastica

Elastica is a PHP client for elasticsearch
http://elastica.io/
MIT License
2.25k stars 733 forks source link

Speed up tests #832

Closed ruflin closed 8 years ago

ruflin commented 9 years ago

The test runs should be speed up to have every build below 3 minutes if possible. There are several steps that can be taken to improve it lake the setup of the machines or tear down to tests. The setup could be done with Docker images (not supported by Travis yet).

ruflin commented 9 years ago

@im-denisenko You were the one that implemented https://github.com/ruflin/Elastica/blob/master/test/lib/Elastica/Test/Base.php#L50 I see the reasoning behind it and it makes sense. There are two potential problems here and I would like to get your opinion on that. The first problem is that I "assume" that cleaning all indexes takes longer then just the ones that were created. On the other side, the same amount of indexes should exist as was created. The second problem is, that some let the tests run on the same cluster as their staging or even production environment (yes, they shouldn't). All these indexes would also be removed. The good thing here is they will learn not to do it. At the same time I think tests should only mess with the data / indexes they created. What are your thoughts here?

im-denisenko commented 9 years ago

There are two reasons why I delete all indices instead of just created.

First is that index could be created not only by this method. If we want to really remove all indices that were created during tests, we should find all existing indices in setUp, find all existing indices in tearDown, diff these lists, and then delete the difference. This is will be even slower than now.

Second is that ES should always have clean state to run tests on it. If someone run Elastica's tests on their cluster where some indices already exists, how you could garantee that tests will not randomly failed or that their indices will be unbroken? I even don't mention that last test in Elastica testsuite shuts down entire cluster.

im-denisenko commented 9 years ago

We could split tests by their group and don't perform complete removal on every test.

I see two groups there: unit and functional. Unit is pure test where ES is not involved. Functional is about ES integration. There are no reason to delete all indices from ES for tests from unit group.

This separation could be done either using phpunit groups, or by two test's trees instead of just one.

ruflin commented 9 years ago

Good idea. I would probably not change the test trees as it will "duplicate" a lot of classes. About the speed gain / loss I agree. Currently the slowest past is probably the setup. Unfortunately there is no Docker support from Travis yet.

ruflin commented 9 years ago

About clean es instance: Agree that we should expect that to have. So lets keep removing all indices and not lets add complexity here.

im-denisenko commented 9 years ago

Good idea. I would probably not change the test trees as it will "duplicate" a lot of classes.

Well, phpunit groups then. I would implement this feature on weekend.

Unfortunately there is no Docker support from Travis yet.

AFAIK, their problem is that they use docker internally, hence that support is really mean run docker container inside docker container, that sounds quite strange :)

ruflin commented 9 years ago

I like the group approach as it also makes it possible to "tag" one test with multiple groups.

I would assume they don't have to run docker on top of docker, but spin it up similar to the rest of their infrasturcture. I'm quite sure they are working on that. Docker Swarm etc should help here.

ruflin commented 8 years ago

Closing as https://github.com/ruflin/Elastica/pull/848 was merged.