mindflayer / python-mocket

a socket mock framework - for all kinds of socket animals, web-clients included
BSD 3-Clause "New" or "Revised" License
284 stars 45 forks source link

Refactor split socket and ssl socket #265

Closed betaboon closed 1 week ago

betaboon commented 1 week ago

continuation of #261 #262 #263 #264

This PR includes the following changes:

Some notes:

socket.read and socket.write seem to have been considered part of the socket-api. They are infact part of SSLSocket and have thus been incorrectly used. a test and some smaller internal methods had to be changed accordingly.

betaboon commented 1 week ago

interesting. i tested locally with python3.8, which passes. with >=3.11 i can reproduce the tests failing. will look at that tomorrow

mindflayer commented 1 week ago

Hi there, could you please explain why you think this is a good idea?

extend FakeSSLContext by replacing all dummy-methods with full signatures

Mocket is supporting different versions of Python 3. In the past it has even supported Python 2 and 3 at the same time (that's where the compat module with types comes from). Libraries can change from a version to another, we don't want to implement full signatures, and that's the reason for having a dummy method used as catch-all. For getter/setter, what is the reason to implement them one by one, if what we do is still return 0 and pass everywhere? Trying to keep Mocket code-base as lean as possible positively impacts its maintainability. And believe me, almost every time there is a new Python version around, there are things to do to make it work.

betaboon commented 1 week ago

hey. in my eyes it contributes to readability.

but again, preferences. if you prefer the less-verbose way, i will revert :)

mindflayer commented 1 week ago

I don't think that having x lines of copy/pasted functions is more readable than having a single catch-all method, especially in the case of Mocket where their code does literally nothing meaningful.

mindflayer commented 1 week ago

Also, having a single function helps with debugging.

mindflayer commented 1 week ago

socket.read and socket.write seem to have been considered part of the socket-api. They are infact part of SSLSocket and have thus been incorrectly used. a test and some smaller internal methods had to be changed accordingly.

Weren't you the one who split the logic into two separated classes: MocketSocket AND MocketSSLSocket?

betaboon commented 1 week ago

Weren't you the one who split the logic into two separated classes: MocketSocket AND MocketSSLSocket?

yep. exactly for the reason of cleanly separate the apis.

eg this in the tests is just wrong:

       sock = socket.socket()
        sock.connect(address[-1])
        sock.write(f"{method} {path} HTTP/1.0\r\n")

socket.socket does not have a write-method.

sonarcloud[bot] commented 1 week ago

Quality Gate Passed Quality Gate passed

Issues
19 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

coveralls commented 1 week ago

Coverage Status

coverage: 99.255% (-0.07%) from 99.324% when pulling 636951f2f9ea47139539b346c0e3bbc9067e86f0 on betaboon:refactor-split-socket-and-ssl-socket into 89055e8a54b41a0ff8d21dcd2d1774e1e95f8667 on mindflayer:main.

coveralls commented 1 week ago

Coverage Status

coverage: 99.255% (-0.07%) from 99.324% when pulling 636951f2f9ea47139539b346c0e3bbc9067e86f0 on betaboon:refactor-split-socket-and-ssl-socket into 89055e8a54b41a0ff8d21dcd2d1774e1e95f8667 on mindflayer:main.

coveralls commented 1 week ago

Coverage Status

coverage: 99.255% (-0.07%) from 99.324% when pulling 636951f2f9ea47139539b346c0e3bbc9067e86f0 on betaboon:refactor-split-socket-and-ssl-socket into 89055e8a54b41a0ff8d21dcd2d1774e1e95f8667 on mindflayer:main.

coveralls commented 1 week ago

Coverage Status

coverage: 99.255% (-0.07%) from 99.324% when pulling 636951f2f9ea47139539b346c0e3bbc9067e86f0 on betaboon:refactor-split-socket-and-ssl-socket into 89055e8a54b41a0ff8d21dcd2d1774e1e95f8667 on mindflayer:main.

mindflayer commented 1 week ago

yep. exactly for the reason of cleanly separate the apis.

eg this in the tests is just wrong:

Yes, the test was using the wrong method. It was added by a contributor and I did not spot the fact that it was using write(). Good catch!

betaboon commented 1 week ago

from my perspective this is ready.

mindflayer commented 1 week ago

from my perspective this is ready.

I am working atm, I'll review it as soon as possible.

betaboon commented 1 week ago

take your time. I'm back at work too :)