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

♻️ Remove overloaded loops in tests #2674

Conversation

sanderegg
Copy link
Member

@sanderegg sanderegg commented Dec 2, 2021

What do these changes do?

In pytest the loop fixture is function scoped. The idea behind is to have a clean Event Loop for each test.

For different reasons, we used to overload that loop with a module scoped, or session scoped loop. This leads to weird behaviours in some tests that became a hurdle and a pain. Sometimes the loop gets replaced and the typical error message of "a future is waiting in a different loop than the current loop" would appear.

Rationale:

  • we are using pytest-aiohttp for pytest
  • this creates a loop fixture that is function scoped
  • when we use async fixtures this gets detected automatically by pytest-aiohttp and then auto-use the loop fixture

--> To simplify and start from a clean slate, all the module/session scoped loops were removed.

Also some of the autouse fixtures are removed. PLEASE DO NOT USE THAT FEATURE, it is not explicit and is painful when debugging.

Hint: with pytest one casn use pytest --setup-show, this shows how the fixtures are started

Related issue/s

How to test

Checklist

@sanderegg sanderegg added a:infra+ops maintenance of infrastructure or operations (discussed in retro) a:webserver issue related to the webserver service a:director-v2 issue related with the director-v2 service a:dask-service Any of the dask services: dask-scheduler/sidecar or worker labels Dec 2, 2021
@sanderegg sanderegg added this to the Meerkat milestone Dec 2, 2021
@sanderegg sanderegg self-assigned this Dec 2, 2021
@codecov
Copy link

codecov bot commented Dec 2, 2021

Codecov Report

Merging #2674 (a059446) into master (670cb22) will decrease coverage by 2.8%.
The diff coverage is 80.0%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #2674     +/-   ##
========================================
- Coverage    78.0%   75.1%   -2.9%     
========================================
  Files         636     636             
  Lines       26064   26073      +9     
  Branches     2524    2525      +1     
========================================
- Hits        20342   19602    -740     
- Misses       5053    5847    +794     
+ Partials      669     624     -45     
Flag Coverage Δ
integrationtests 82.0% <60.0%> (+15.3%) ⬆️
unittests 73.6% <80.0%> (-0.1%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...rector_v2/modules/comp_scheduler/dask_scheduler.py 91.8% <ø> (ø)
...-sidecar/src/simcore_service_dask_sidecar/tasks.py 81.2% <70.0%> (-3.0%) ⬇️
...ctor_v2/models/schemas/dynamic_services/service.py 100.0% <100.0%> (ø)
...r-v2/src/simcore_service_director_v2/utils/dask.py 93.5% <100.0%> (+<0.1%) ⬆️
...simcore_service_webserver/computation_subscribe.py 27.2% <0.0%> (-65.7%) ⬇️
...imcore_service_webserver/exporter/export_import.py 34.1% <0.0%> (-61.0%) ⬇️
...re-sdk/src/simcore_sdk/node_ports/serialization.py 25.0% <0.0%> (-57.4%) ⬇️
...vice_webserver/exporter/formatters/formatter_v2.py 31.9% <0.0%> (-56.4%) ⬇️
...vice_webserver/exporter/formatters/formatter_v1.py 24.5% <0.0%> (-54.2%) ⬇️
...c/simcore_service_webserver/users_to_groups_api.py 46.1% <0.0%> (-53.9%) ⬇️
... and 43 more

Copy link
Contributor

@GitHK GitHK left a comment

Choose a reason for hiding this comment

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

Nice cleanup!

@@ -135,7 +135,7 @@ def services_endpoint(


@pytest.fixture(scope="module")
async def simcore_services_ready(
def simcore_services_ready(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this changed to non async?

Copy link
Member

Choose a reason for hiding this comment

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

@GitHK because, as stated in the PR, we can only use async fixtures with a function scope, otherwise (i.e. module or session) they must be synchronous. This is a "reasonable" limitation provided the complex situations we found when mixing loops at different scopes :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

read the PR description ;)
async def fixtures are auto-detected by pytest-aiohttp

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, I've missed that part. I got it, no more module or session scoped async fixtures, only function wise async.

services/director-v2/tests/conftest.py Show resolved Hide resolved
services/director-v2/tests/unit/conftest.py Show resolved Hide resolved
@sanderegg sanderegg force-pushed the maintenance/remove_overloaded_loops_in_tests branch from 77f6b59 to 20ed9b7 Compare December 2, 2021 09:36
Copy link
Member

@pcrespov pcrespov left a comment

Choose a reason for hiding this comment

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

thx! greatly appreciated the changes in this PR

@@ -135,7 +135,7 @@ def services_endpoint(


@pytest.fixture(scope="module")
async def simcore_services_ready(
def simcore_services_ready(
Copy link
Member

Choose a reason for hiding this comment

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

@GitHK because, as stated in the PR, we can only use async fixtures with a function scope, otherwise (i.e. module or session) they must be synchronous. This is a "reasonable" limitation provided the complex situations we found when mixing loops at different scopes :-)

services/dask-sidecar/requirements/_test.in Show resolved Hide resolved
services/director-v2/requirements/_test.in Show resolved Hide resolved
services/director-v2/tests/conftest.py Show resolved Hide resolved
services/director-v2/tests/conftest.py Outdated Show resolved Hide resolved
@sanderegg sanderegg force-pushed the maintenance/remove_overloaded_loops_in_tests branch from 8cc58e3 to a059446 Compare December 2, 2021 21:07
@sanderegg sanderegg merged commit ae56a82 into ITISFoundation:master Dec 2, 2021
@sanderegg sanderegg deleted the maintenance/remove_overloaded_loops_in_tests branch December 2, 2021 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:dask-service Any of the dask services: dask-scheduler/sidecar or worker a:director-v2 issue related with the director-v2 service a:infra+ops maintenance of infrastructure or operations (discussed in retro) a:webserver issue related to the webserver service
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants