Make rest_api fixture async

[drew2a]

This PR fixes rest_api by making it async.

Previously the common pattern was:

@pytest.fixture
def rest_api(event_loop, aiohttp_client, knowledge_endpoint):
    app = Application()
    app.add_subapp('/knowledge', knowledge_endpoint.app)
    yield event_loop.run_until_complete(aiohttp_client(app))
    app.shutdown()

which is incorrect as app.shutdown() is async call.

Additionally, this PR includes some minor refactoring.

Related to #7495

[drew2a]

I'll squash commits at the end, after review.

[kozlovsky]

I appreciate the effort in making the rest_api fixture async, as it streamlines the code used in async tests. However, there are some aspects of the PR that need attention and possible revision:

1. The PR description mentions fixing the app.shutdown() call by adding await, but this change has already been addressed in PR Fix calling await app.shutdown() in REST API tests #7502. Please update the PR description to reflect only the changes made in this PR.

2. The removal of the web_app fixture has led to duplication of the app = Application(...) / ... / await app.shutdown() sequence in 14 different places. The original fixture centralized this logic. Can you elaborate on the benefit of removing it?

3. It's great that this PR addresses the calling of the shutdown methods of specific endpoints used in tests. However, this isn't mentioned in the PR description. Please update the description to include this critical fix. Also, it would be ideal if, in the future, app.shutdown() could automatically handle the shutdown of endpoints, though this should be approached cautiously.

4. The removal of the event_loop fixture needs justification. It was initially introduced in PR Replace the loop fixture with the event_loop fixture. #7168 to resolve the "Asyncio Event Loop is Closed" issue on Windows by employing WindowsSelectorEventLoopPolicy and new_event_loop(). Please either provide a rationale for its removal or consider reinstating it, as it apparently fixed important issues on Windows.

Thank you for your contributions. Please address these points to ensure that the PR accurately reflects its changes and maintains code quality.

[xoriole]

Other than the event_loop fixture removal, the rest is OK to me.

[xoriole]

Based on the Slack discussion, I'm okay with the approach to see if event_loop is required based on future test results, in which case, it can be reverted if the event loop issue reoccurs. For now, I approve the PR.

[drew2a]

@kozlovsky I appreciate your appreciation in effort in making the rest_api fixture async.

The PR's title and description are sufficiently descriptive since the main change is converting the rest_api fixture to asynchronous, ensuring all calls made from this fixture are correct. As a result, the inner logic of the rest_api fixture has undergone modifications, which are clear from both the description and implementation (the latter is straightforward enough to not necessitate a more detailed explanation).

Does the following change truly require a further detailed description due to its complexity?

@pytest.fixture
def rest_api(event_loop, aiohttp_client, knowledge_endpoint):
    app = Application()
    app.add_subapp('/knowledge', knowledge_endpoint.app)
    yield event_loop.run_until_complete(aiohttp_client(app))
    app.shutdown()

@pytest.fixture
async def rest_api(aiohttp_client, knowledge_endpoint):
    app = Application()
    app.add_subapp('/knowledge', knowledge_endpoint.app)
    yield await aiohttp_client(app)
    await app.shutdown()

The intention behind introducing web_app and the modifications in this PR are the same, but the current PR goes further by fixing the rest_api fixture as well as some endpoints. As the changes with web_app were made to the release branch, it led to merge conflicts (which could have been avoided had it been merged into the main branch, as it's not part of release fixes and shouldn't go into the release branch). Given that this PR supersedes the changes with web_app, I applied it regardless, thereby avoiding the merge conflicts.

As for the removal of event_loop, I've already provided the rationale.

[drew2a]

I plan to address the duplication of rest_api in the next PR, as it's less crucial than the changes presented in this one.

[kozlovsky]

Given that this PR supersedes the changes with web_app, I applied it regardless, thereby avoiding the merge conflicts.

I'd say overwriting the code that is already (at that moment) in the main branch is not the same as "avoiding the merge conflicts."

If the repo already has the merge conflict, and one PR is already in the repo while the other PR is going through the review, solving the merge conflict usually does not mean that you completely override the code of the existing PR with your code.