thiezn / iperf3-python

Python wrapper around iperf3
https://iperf3-python.readthedocs.org/
MIT License
110 stars 51 forks source link

Fixing incorrect return of reverse flag #5

Closed fciaccia closed 7 years ago

fciaccia commented 7 years ago

Hi, first, thank you very much for the work you have been doing with this project, it is really useful. I was modifying the TestResult class to correctly handle the case in which the user wants to disable the json_output when I found out this: https://github.com/thiezn/iperf3-python/blob/master/iperf3/iperf3.py#L615

Here we are returning the opposite value of the reverse field respect to the JSON passed from the client. The semantic usage of the -R flag in iperf3 cli is (from man):

-R, --reverse
              run in reverse mode (server sends, client receives)

which means that reverse is not a relative value (i.e. it does not depends on the result being returned by the client or the server) and its default value should be False. BTW, also the test is checking for a True value when we are actually not setting the reverse flag, line: https://github.com/thiezn/iperf3-python/blob/master/tests/test_iperf3.py#L154 . Checking the response from the client actually shows:

[...]
"test_start":   {
            "protocol": "TCP",
            "num_streams":  1,
            "blksize":  131072,
            "omit": 0,
            "duration": 3,
            "bytes":    0,
            "blocks":   0,
            "reverse":  0
        }
[...]

Thanks for any feedback provided, Francesco

PS. my editor automatically removed some whitespaces, just noticed.

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 98.739% when pulling 9595e5274a0577f014310c01258bd449809e6945 on fciaccia:hotfix/reverse-flag into 2abc8347cb7f7520c8f042b9452b2873870e38b4 on thiezn:master.

thiezn commented 7 years ago

Thanks @fciaccia, You are right, I was parsing the reverse field the other way around.

thiezn commented 7 years ago

I'll push the change out to PyPi later today so you can pip install

fciaccia commented 7 years ago

Thanks I actually have implemented a couple of features more in my branch, I'll make you the pull request so you can check them before pushing to pip. thanks again!