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 tests #3419

Merged
merged 11 commits into from Dec 2, 2018
Merged

Optimize tests #3419

merged 11 commits into from Dec 2, 2018

Conversation

asvetlov
Copy link
Member

@asvetlov asvetlov commented Dec 1, 2018

Currently, travis fails on about 10-15% runs.
This is because pytest-xdist plugin executes tests in several threads to speed up the time.
The intention is good, the benefit is obvious but some aiohttp tests require the main thread context to setup Unix signal handlers.

The PR refactors tests running strategy.

  1. pytest-xdist is removed
  2. Tests with uvloop are dropped. Before the change, every async tests was executed twice: with asyncio loop and uvloop, doubling the execution time. The last and only problem with uvloop was sendfile usage, found and fixed about 3 or 4 years ago.
  3. PYTHONASYNCIODEBUG run was removed, several tests for explicit debug loop was added instead.
  4. AIOHTTP_NO_EXTENSIONS run was removed from every job run but added an explicit job for making sure that aiohttp works without C extensions.

As the result:

  1. A regular job execution was reduced about 2.5 times.
  2. The total run time was shrinked from 33 to 18 secs.
  3. Tests are stable.
  4. Coverage is not changed (increased a little after adding new tests actually)
  5. Output log always fit into Travis requirements, it is browsable again.

@asvetlov asvetlov merged commit 15857de into master Dec 2, 2018
@asvetlov asvetlov deleted the optimize-tests branch December 2, 2018 21:50
@webknjaz
Copy link
Member

webknjaz commented Dec 3, 2018

Its too radical. I'm working on improvements and I think lots of envs are acceptable but only on when run cron.
xdist: I still want it. You could disable it but leave ability to enable it by individuals.

@asvetlov
Copy link
Member Author

asvetlov commented Dec 3, 2018

  1. The PR fixes the real flakiness, so it is useful.
  2. It reduces execution time even more than xdist appliance.
  3. A set of tested environments is always a compromise between tests execution time and amount of scenarios to test. Personally, I have a feeling that testing on uvloop is redundant.

That's why I'd like to keep the PR merged.
When you''ll find a time for improvements -- let's discuss it.

@webknjaz
Copy link
Member

webknjaz commented Dec 3, 2018

Sure, I'm not implying unmerging it. It's just the way it's solved is a bit conflicting with my ideas. We still need tox-based envs in place when I get to it...

@asvetlov
Copy link
Member Author

asvetlov commented Dec 3, 2018

I feel uncomfortable with tox when working on daily tasks:

  1. I don't need to reactivate environment between test runs, the current activated virtual env works pretty fine.
  2. No need to run multiple test passes locally and wait for the long run. Passing tests in the current venv is a good sign for pushing a PR on github, CI will do the rest checks.

@asvetlov
Copy link
Member Author

asvetlov commented Dec 5, 2018

@webknjaz pytest-xdist problem can be solved by introducing xdist_run_in_main_thread marker probably, but I have no time to contribute into the plugin, sorry.
I hope you understand my position.
Now the feature doesn't exist.

@webknjaz
Copy link
Member

webknjaz commented Dec 5, 2018

@asvetlov

  1. It looks like you miss the point: I suspect you're using it wrong. You don't have to reactivate the env and maybe you just need to get familiar with how it works. Could you please make an https://asciinema.org/ recording of what's going wrong?

  2. This is not my default setup either: in my configurations tox always uses currently available Python. So your assumption is wrong.

  3. The idea about the marker is interesting, I'll need to think about it. But again, I want to keep the plugin even turned off so that it won't affect anyone.

  4. I'm trying to get rid of that spaghetti mess currently present in Makefile: it's harmful and not CI-agnostic. I want to have a place with envs described independently from CIs or any other external (local/dev) envs so that it'd be clean.

@lock
Copy link

lock bot commented Dec 5, 2019

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Dec 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Dec 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants