kataev / pytest-grpc

Allow test gRPC with pytest
MIT License
126 stars 20 forks source link

Asyncio servers/clients, large redesign of plugin #15

Closed nils-werner closed 1 year ago

nils-werner commented 4 years ago

I realize that the diff for this PR seems like a huge, backwards incompatible change to the versions before. While a lot of things moved under the hood, the API for the end user stayed mosty the same (see further down). However still, please understand it as a conversation starter, not as a finalized PR.

Also, please don't read the diff (which is a mess), but the actual source code of the branch (which is clean).

This branch started as an attempt to implement the new gRPC Asyncio API. The reason for this was not just for fun or cosmetic, but because certain gRPC features, like interceptors, must be implemented and tested separately for for sync- and async-servers.

With this change, a user can simply request a grpc_stub to run synchronous tests

def test_some(grpc_stub):
    request = EchoRequest()
    response = grpc_stub.handler(request)
    assert response.name == f'test-{request.name}'

or an asio_grpc_stub to run asynchronous tests.

@pytest.mark.asyncio
async def test_some_async(aio_grpc_stub):
    request = EchoRequest()
    response = await aio_grpc_stub.handler(request)
    assert response.name == f'test-{request.name}'

In the background, the appropriate gRPC server is then automatically started up (mixing async with sync client and server is not possible atm). The remainder of the configuration and fixtures stay the same.

Features included in the PR:

However, while developing this, I realized that there are too many places in pytest-grpc that would not really allow the implementation of async-fixtures, without changing those parts of the plugin first. The parts that I changed are:

The way forward: As I said, I am aware this seems like a huge changeset, which makes it a very unattractive merge. However, I believe this design is a true improvement over the design before, and should provide good foundation for future changes.

For most users, I recon, there is only a tiny change they need to make to be compatible to this:

  1. Change the fixture scopes, i.e. @pytest.fixture(scope='module') to @pytest.fixture
  2. Change

    @pytest.fixture
    def grpc_servicer():
        return Servicer()

    to

    @pytest.fixture
    def grpc_servicer():
        return Servicer

The remainder of changes only affect users who are making very advanced use of this, using fake channels (which were only a run-time optional parameter), or SSL (which is now much easier to use).

I believe these changes are small enough that a major-version release is enough to differentiate between the old API and architecture, and the proposed, new API and architecture.

I am looking forward to hearing your thoughts on this.

NikolaBorisov commented 2 years ago

@kataev Any chance we can merge this PR. I can help or @nils-werner maintain the project also.

NikolaBorisov commented 1 year ago

@kataev Any hope here. If you want you can make me maintainer and I can help merge the @nils-werner PR.

kataev commented 1 year ago

Hola Hola, wait a moment

kataev commented 1 year ago

Thanks for the suggested changes.

But why did you decide to rewrite the entire library in asynchronous code and run it under the same name without maintaining backward compatibility? It seems that your code can be published under a different name and everything will be fine, for example pytest-grpc-async.

curtiscook commented 1 year ago

The fixture scope was changed to function. There is no reason to use a larger scope than this, and larger scopes really mess with per-function parameterization or simultaneously offering sync and async servers.

Creating a server per test will be slow and seems irrelevant to enabling async? You wouldn't create a new database for every single test function. If you want to test sync and async servers, you could just .. create a second test file? If you want to change this, it should at least be a setting (until https://github.com/pytest-dev/pytest/issues/1681 is implemented )

That said, there's no real lib for testing async grpc right now. Why not make this pr backward-compatible so it can get merged and people can use the async functionality?

nils-werner commented 1 year ago

That said, there's no real lib for testing async grpc right now. Why not make this pr backward-compatible so it can get merged and people can use the async functionality?

Because development of this lib is dead/stale anyways. A new major release or a new name would have the same effect.

That being said, this PR is 2.5 years old, and I have moved on to other projects. I won't be able to work on this or take part in discussions about this anymore.

kataev commented 1 year ago

Thank you for your contribution to pytest-grps. I appreciate the time and effort you've put into creating a pull request. After careful review, I have decided that this particular pull request may not align with the goals or direction of the project at this time. However, I wanted to provide some feedback and suggestions for your code.

I believe that your code has potential and could be valuable in a different context. I recommend considering creating a fork repository to showcase your work. This way, your contribution can still be recognized and used by others who may find it useful for their async use cases.

Best regards, Denis.