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

Test are too slow #339

Open
2 tasks done
xihh87 opened this issue Jun 16, 2022 · 4 comments
Open
2 tasks done

Test are too slow #339

xihh87 opened this issue Jun 16, 2022 · 4 comments

Comments

@xihh87
Copy link
Contributor

xihh87 commented Jun 16, 2022

A test suite should run as fast as possible to allow a fast feedback loop.

Most of the tests take too long because they:

If fixing those two issues is not enough, we could maybe identify slow tests and mark them as slow to address any concern left.

xihh87 added a commit to xihh87/manubot that referenced this issue Jun 16, 2022
xihh87 added a commit to xihh87/manubot that referenced this issue Jun 16, 2022
@xihh87
Copy link
Contributor Author

xihh87 commented Jun 17, 2022

By fixing #337 and running with pytest-xdist all tests run in 25 s.

pytest -n 4 # because my laptop has as many cores 

Would you be interested in me modifying the default setup?

@dhimmel
Copy link
Member

dhimmel commented Jun 19, 2022

Faster tests would be nice, although haven't found this test suite unreasonably slow. But thanks for the interest! All tests in under 30 seconds sounds appealing!

I'm worried that test parallelization would end up ignoring the API requests rate limits like at:

@functools.lru_cache()
def _get_eutils_rate_limiter() -> "RateLimiter":
"""
Rate limiter to cap NCBI E-utilities queries to <= 3 per second as per
https://ncbiinsights.ncbi.nlm.nih.gov/2017/11/02/new-api-keys-for-the-e-utilities/
"""
with warnings.catch_warnings():
# https://github.com/RazerM/ratelimiter/issues/10
# https://github.com/manubot/manubot/issues/257
warnings.filterwarnings("ignore", category=DeprecationWarning)
from ratelimiter import RateLimiter
if "CI" in os.environ:
# multiple CI jobs might be running concurrently
return RateLimiter(max_calls=1, period=1.5)
return RateLimiter(max_calls=2, period=1)

Not sure if pytest-xdist has support for grouping certain tests together.

Is this for local test execution or CI? If for local, users can possibly use pytest-xdist without changing on CI?

we could maybe identify slow tests and mark them as slow to address any concern left

I think marking slow or network related tests might be a good solution here as well.

@xihh87
Copy link
Contributor Author

xihh87 commented Jun 21, 2022

Test time does depends on whether the tested services are responsive, when all services are responsive, 25 seconds on 4 theads is feasible.

Today I ran test on main using 20 threads and took 42 minutes:

============================= 11 failed, 290 passed, 16 skipped, 4 xfailed in 2559.00s (0:42:39) ==============================

On #338 I ran tests and got the same results (20 theads as well) on 1.5 minutes:

============================== 11 failed, 290 passed, 16 skipped, 4 xfailed in 83.12s (0:01:23) ==============================

I'm interested mainly on local development, as I'm using the tests to know whether I break something, but waiting 42 minutes for a simple change is a no go.

It seems like multiple workers running tests would be rate limited by thread and could indeed surpass the rate limited services threshold, but if that is a problem when running the suite we could reduce the rate at which we make request (on the tests), or set a lock for a rate limited service so that only one test can run concurrently on the service.

@xihh87
Copy link
Contributor Author

xihh87 commented Jun 21, 2022

It may also be a good idea to acquire network input in a cache which is valid only for a few hours and run the tests on those when they are available.

However, the current architecture does not support the separation of network input and processing.

Would that be a desired refactoring?

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

No branches or pull requests

2 participants