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

Asyncio servers/clients, large redesign of plugin #15

Closed
wants to merge 29 commits into from

Conversation

nils-werner
Copy link

@nils-werner nils-werner commented Sep 2, 2020

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:

  • asyncio servers and clients
  • multiple servicers per server
  • simple channel credential overloading (i.e. SSL)
  • more examples, and examples are working unittests

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 original server fixtures are almost completely unchanged. The only change: grpc_servicer was made more similar to grpc_add_to_server and grpc_stub_cls and returns the servicer class, instead of an instance.

  • 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.

  • The example files were changed to be actual unit tests. This means the examples are not an afterthought, but are immediately runnable and are effectively testing this plugin using the pytest command.

  • The server/channel credentials were put in special fixtures, so that SSL-tests are much simpler to set up.

  • Probably the biggest change: the fake classes were removed entirely. I found that they were not API-compatible to the gRPC server and channel classes, and therefore weren't really a transparent drop-in-replacement to the actual server and channels. I think this limits their usefulness for unit testing, as you'd introduce behaviour, side-effects and bugs from your fake classes into your unit tests, and you're not testing the gRPC servicer in isolation, but its interaction with the fake classes...

    On the other side, the gRPC team is developing a gRPC testing channel and server classes, too. Right now they do not seem feature-complete and do not seem to offer drop-in-replacement either, however still I would recommend for server/channel-mocking, this plugin should wait until the official test mocks are ready, and use them then.

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
Copy link

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

@NikolaBorisov
Copy link

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

@kataev
Copy link
Owner

kataev commented Mar 3, 2023

Hola Hola, wait a moment

@kataev
Copy link
Owner

kataev commented Mar 3, 2023

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
Copy link

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 pytest-dev/pytest#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
Copy link
Author

nils-werner commented Apr 7, 2023

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
Copy link
Owner

kataev commented Apr 7, 2023

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.

@kataev kataev closed this Apr 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants