TestClient.app can be None

[Dreamsorcerer]

A minor nuisance, but TestClient.app is Application | None.
When type checking in mypy this results in a bunch of errors unless extra assert calls are made.

Maybe we can make it generic or similar, so when passing an Application to the aiohttp_client fixture, for example, it will know that the attribute will not be None.

[1ort]

Please provide more information about the problem, The warning and error logs from mypy are ideal

[Dreamsorcerer]

Let me know If you'd like to work on the issue, and I'll provide further guidance. This was partly just a note for me to come back to later.

But, basically, as indicated above, it is typed as Application | None. But, in reality, if you instantiate it with Application, it will always return an Application, if you instantiate it with BaseServer, it will always return None. So, this could be made Generic to represent that accurately.

[1ort]

I'd like to work with that, but I don't know how to approach it.
We have BaseTestServer, which we could make a generic type, but to do that we would have to use a class attribute like I wrote below and loop this whole chain of generics all the way down to TestClient

OptionalApplication = TypeVar('OptionalApplication', None, Application)

class BaseTestServer(ABC, Generic[OptionalApplication]):
    app: OptionalApplication = None
    ...

class TestServer(BaseTestServer[Application]):
    ...

class RawTestServer(BaseTestServer[None]):
    ....

class TestClient(Generic[OptionalApplication]):
    server: BaseTestServer[OptionalApplication]
    
    @property
    def app(self) -> OptionalApplication:
        return self._server.app

    ....

I'm not sure if this is the right approach, I'd be glad if you could point me in the right direction

[Dreamsorcerer]

OK, it looks like it's just TestServer which includes the app:

aiohttp/aiohttp/test_utils.py

Line 209 in 3ff81dc

class TestServer(BaseTestServer):

So, following the current implementation, we could just make TestClient generic. If it's passed an instance of TestServer, then it would be set to Application, any other instance of BaseTestServer would be None.

It might be nice to make BaseTestServer generic as well and add that information in, but current code is just cast()ing.

[Dreamsorcerer]

So, yeah. If you want to do what you described above, as long as it doesn't cause a big headache in the tests, I think that's a good way to go.

[1ort]

I returned to this issue again. We can just always return Application from TestClient.app and raise an error instead of None
Something like this

    @property
    def app(self) -> Application:
        if hasattr(self._server, "app"):
            return self._server.app
        raise AttributeError("BaseTestServer has no attribute app")     

However, this changes the behavior of the tests, this can be problematic

[Dreamsorcerer]

Hmm, that could work. But, is there a problem with the previous proposal?

[1ort]

I'm just trying to pick an approach that affects as few lines as possible and doesn't affect anything else.

[Dreamsorcerer]

My thoughts were that these test servers are unlikely to be used directly by users often, so if making them generic improves accuracy/robustness of the code, then it seems like a good change. Let me know if you think I'm missing something though.