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

Change loop fixture #7127

Merged
merged 1 commit into from
Nov 14, 2022
Merged

Change loop fixture #7127

merged 1 commit into from
Nov 14, 2022

Conversation

drew2a
Copy link
Contributor

@drew2a drew2a commented Oct 31, 2022

This PR fixes #7126 by it removing the following asyncio-aiohttp compatibility patch from c8c307d:

@drew2a drew2a marked this pull request as ready for review October 31, 2022 13:46
@drew2a drew2a requested review from a team, kozlovsky and xoriole and removed request for a team October 31, 2022 13:46
Copy link
Contributor

@kozlovsky kozlovsky left a comment

Choose a reason for hiding this comment

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

To my understanding, there are three independent changes in this PR:

@pytest.fixture
def loop(event_loop):
    """
    _This_ fixture masks the original "loop" fixture from pytest-asyncio,
    effectively replacing it with the "event_loop" fixture from aiohttp.
    It solves the following problem:
    pytest-asyncio provides "loop" fixture for creating the event loop,
    aiohttp provides "event_loop" fixture for creating the event loop,
    if you use the "@pytest.mark.asyncio" decorator on a test, it will automatically run the "loop"
    fixture, which could result in test failure if the test uses Futures created with the fixtures
    that use the "event_loop" fixture.
    """
    return event_loop

As there is no explanation in PR, it is hard to say why it is including these changes, and do we really need all three changes at once? Maybe we can split changes into three independent PRs and see how each of them affects tests stability?

I'm a bit concerned about removing the asyncio-aiohttp compatibility patch; it may potentially break aiohttp-related tests in a surprising way.

So, can we split this PR into a three smaller ones and check each of them independently?

@drew2a
Copy link
Contributor Author

drew2a commented Nov 2, 2022

@kozlovsky

we can split changes into three independent PRs and see how each of them affects tests stability.

Yes, we can. Normally I'm trying to do as atomic PR as possible. But it is not always necessary.

This PR indeed combined three changes that should improve test stability.

The reason to combine them together is to decrease the total time on stabilizing our test suit. There are a lot of stability issues and only one developer works on fixing them.

@drew2a
Copy link
Contributor Author

drew2a commented Nov 2, 2022

I'm going to split this PR into three today.

Copy link
Contributor

@kozlovsky kozlovsky left a comment

Choose a reason for hiding this comment

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

This change can break aiohttp tests in a subtle way, so we need to be careful and be ready to revert it back if some new instabilities arise after the merge.

@drew2a drew2a merged commit f16fcb0 into Tribler:main Nov 14, 2022
@drew2a drew2a deleted the fix/7126 branch November 14, 2022 10:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

[Tests] ERROR at setup of test_cancel
2 participants