Why disable the scope entirely in the test client rather than making an option to disable it?

[pythonweb2]

Related: #2408

I understand that the intended behavior was to disable this, but pushing a breaking change in a minor version doesn't seem like a good idea. We have a whole bunch of code that depended on the client.host to be populated, and now I have to refactor it all if I update to 0.35.0.

My proposal is to revert the change and make an option to set the client to None when you instantiate the TestClient. Something like this:

t = TestClient(client=None)

If you use a fixture, that is a one line change. If not, you could make subclass it and override the client property.

@Kludex

[pythonweb2]

I created a PR to revert the change, and add an option for users. Could also not revert the change, but also add the option.

#2430

[Kludex]

but pushing a breaking change in a minor version doesn't seem like a good idea.

We are on ZeroVer.

[pythonweb2]

ZeroVer is satire, please do not use it.

I'm not sure what to make of this.

[Kludex]

Pardon me?

[pythonweb2]

I read that in the about section of the website you posted.

[Kludex]

@aminalaee what do u think?

[pythonweb2]

I would actually make a case that the spec should be changed in this instance. It just seems odd that implementing the spec makes some things more difficult to test.

For example, say I made some sort of logging framework and I needed the IP/port of the client to make a logging call. And if it didn't exist, I have some middleware that throws a 4xx/5xx error. The only way I can think to test this is to have a flag inside of production code that changes the scope["client"] based on whether or not a test is running, which is "usually a bad idea".

We have a situation similar to this, and what I'll probably end up doing is making a custom middleware that adds the IP/port and only add that middleware if a test is running, if anyone comes to this discussion and has a similar issue.

[pythonweb2]

Although as I read through the original issue, from what I can see there wasn't a case made that the spec requires the client["scope"] to be None in a test environment, just that the client IP should be a valid IP. So my change of allowing the user to set this in a test case wouldn't be in violation of the ASGI spec.

[pythonweb2]

@Kludex @aminalaee Not sure if you didn't see my last message. Thinking about this for a bit and it still doesn't sit right with me. I wrote the code to add a new parameter to TestClient (it was like a three line change). It still complies with the ASGI spec, since from the original discussion the only reason for the change was that "testclient" was not a proper IP address.

The PR was closed, and essentially now you are saying the code needs to be refactored in a way that is not recommended by pytest, filling up the production code with unsightly flags to check whether or not the test suite is currently running.

Could there be a specific reason given to reject my proposed change? ( #2430 )

[aminalaee]

I would actually make a case that the spec should be changed in this instance

I think you can take it to the asgi-ref for that. But for this change:

• first the hard-coded value was wrong, so it should've been removed
• according to the ASGI spec this value can be None and checking other ASGI frameworks I also saw that they were complying the spec for this
• I think for adding TestClient(scope_client=...) you can start a separate discussion, but I think there could be a lot of arguments to be added like this, so it's probably easier if you just override the TestClient and customise it rather than add every argument in Starlette since it's a very specific use-case

[pythonweb2]

@aminalaee @Kludex in my other messages I mentioned that that my proposed change wouldn't require an ASGI spec change.

Regarding overriding the TestClient, if it was something that isn't being worked on very often, I would be fine with it, or if there was a simple way to override this value (maybe if it was set as a public attribute on the class), then I would be perfectly fine with that. But as of right now, it is being changed quite frequently from what I can see in the history, and having to pull the new updates into a class that overrides the base class would be tedious.

What if instead of a parameter, I just added a public attribute set to None, which wouldn't clutter up the call signature. Then if a user wants to override the value, they can change it after instantiating the class?