test_client should accept application object as parameter

[mayurmahajan]

Long story short

This is not an issue, but a feature improvement request. Per this documentation http://aiohttp.readthedocs.io/en/stable/testing.html test_client accepts a create_app function in it's constructor. It will be cool if test_client accepts application object instead.

This feature improvement is not critical. application object is used by any wsgi server to run a service. similarly this test_client should use an application object to create server/client. It's just more convenient to think and use.

Expected behaviour

instead of

@pytest.fixture
def cli(loop, test_client):
return loop.run_until_complete(test_client(create_app))

if test_client accepts app then

@pytest.fixture
def cli(loop, test_client):
return loop.run_until_complete(test_client(app))

Actual behaviour

Behavior wise nothing changes, it's just the implementation change.

Steps to reproduce

Your environment

[asvetlov]

Good idea.
Would you provide a Pull Request?
Old functionality should be supported too, we cannot make backward incompatible change without very strong reason.

[popravich]

This can be easily achieved without changes to library:

def test_something(test_client):
    client = await test_client(lambda loop: app)

However allowing to pass application object instead of factory can lead to unexpected and
unpredictable errors -- application (or middlewares) may store some state between tests and so
using same input data in different test may lead to unexpected errors or application can be simply
attached to closed loop.
I'm a bit paranoid about it cause I've already seen such bugs.
So I think we can simply add some lines to docs saying how to pass single app instance to test_client.

[asvetlov]

@popravich but people will either create app in test method or use a fixture for it.
I pretty sure they will never store an app in global var.
Well, they may do it but we all are adult guys.
Passing app instead of factory is more convenient way of thinking.
At least current design was my first spike when I've tried to use the fixture myself.
I've just missed this point on merge/review during PyCon sprints in Portland.

[popravich]

I pretty sure they will never store an app in global var.

Should I remind you several discussions with word "global" (remember aiopg global pool, recent aiohttp "how to get global app", etc)?
Besides in examples above I didn't find app to be a fixture )

Anyway, having this feature must require big red warning block in docs saying not to use global app
with test_client.

[asvetlov]

I can live with a big red warning but really need the functionality.
Just compare:

async def test_1(test_client):
    async def handler(request):
         ...
    def create_app(loop, path, handler):
        app = web.Application(loop=loop)
        app.router.add_get(path, handler)
        return app
    client = test_client(create_app, '/', handler)

with

async def test_2(test_client):
    async def handler(request):
         ...
    app = web.Application(loop=loop)
    app.router.add_get('/', handler)
    client = test_client(app)

I've found the second snipped is more straightforward. Moving create_app for local function to first-class one makes code even worse.

[popravich]

...and with

async def test_2(test_client):  # missing loop fixture
    async def handler(request):
         ...
    app = web.Application(loop=loop)
    app.router.add_get('/', handler)
    client = test_client(lambda loop: app)

Just 13 chars more and nothing to change in aiohttp

Ok, if you are sure it will work.

[asvetlov]

Jut a simple thought: if test_client(lambda loop: app) is the recommended and the most practical way to use the fixture -- why we need typing lambda loop: everywhere?

[popravich]

It at least verbose that application factory will receive loop.
Fine, lets make it clear -- test_client can accept application instance,
but it must check that it bound to proper loop.

[asvetlov]

Perfect! I love this.

[asvetlov]

Fixed by #1083

[lock]

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.