tavern icon indicating copy to clipboard operation
tavern copied to clipboard

Bind all MQTT Client callbacks

Open RFRIEDM-Trimble opened this issue 3 years ago • 12 comments

Fix for #753 where I bound all the available MQTT callbacks that would be useful to a developer facing connection issues. There are still a few callbacks that could be bound, but I haven't ever needed them.

Ran the code formatter and smoke tests on the code. Unit tests seem to have unrelated failures, and there is only calls to the logger in this. If there is a good way you wanted to unit test the new callbacks, let me know.

RFRIEDM-Trimble avatar Jan 10 '22 02:01 RFRIEDM-Trimble

Seems the failures in the test report are related to two things:

  1. Using f-strings in logger calls is not optimal performance-wise; will switch those.
  2. Accessing the protected _client_id in the paho client is frowned upon. I want to give the client ID that paho is using in the logger calls, but there isn't a public getter, so I'll remove that debugging; the client ID is already printed in debug should you need it earlier in the logs.

RFRIEDM-Trimble avatar Jan 29 '22 17:01 RFRIEDM-Trimble

It's fine to access the private client ID, I think I already do it in a few other places. If they don't offer an API to get that data then it's the only option :woman_shrugging:

michaelboulton avatar Jan 29 '22 17:01 michaelboulton

Ok, got it. Given that, is that actually what's causing the errors? I'm not super familiar with pylint.

Your code has been rated at 9.97/10

ERROR: InvocationError for command /home/runner/work/tavern/tavern/.tox/py39lint/bin/pylint tavern/ -j 4 (exited with code 4)
___________________________________ summary ____________________________________
ERROR:   py39lint: commands failed
Error: Process completed with exit code 1.

RFRIEDM-Trimble avatar Jan 29 '22 17:01 RFRIEDM-Trimble

It's just more warnings about logging and access to a protected member


tavern/_plugins/mqtt/client.py:209:8: W1203: Use lazy % formatting in logging functions (logging-fstring-interpolation)
tavern/_plugins/mqtt/client.py:210:23: W0212: Access to a protected member _client_id of a client class (protected-access)
tavern/_plugins/mqtt/client.py:217:12: W1203: Use lazy % formatting in logging functions (logging-fstring-interpolation)
tavern/_plugins/mqtt/client.py:218:27: W0212: Access to a protected member _client_id of a client class (protected-access)
tavern/_plugins/mqtt/client.py:221:12: W1203: Use lazy % formatting in logging functions (logging-fstring-interpolation)
tavern/_plugins/mqtt/client.py:222:27: W0212: Access to a protected member _client_id of a client class (protected-access)
tavern/_plugins/mqtt/client.py:228:8: W1203: Use lazy % formatting in logging functions (logging-fstring-interpolation)
tavern/_plugins/mqtt/client.py:229:41: W0212: Access to a protected member _client_id of a client class (protected-access)
tavern/_plugins/mqtt/client.py:209:8: W1203: Use lazy % formatting in logging functions (logging-fstring-interpolation)
tavern/_plugins/mqtt/client.py:210:23: W0212: Access to a protected member _client_id of a client class (protected-access)
tavern/_plugins/mqtt/client.py:217:12: W1203: Use lazy % formatting in logging functions (logging-fstring-interpolation)
tavern/_plugins/mqtt/client.py:218:27: W0212: Access to a protected member _client_id of a client class (protected-access)
tavern/_plugins/mqtt/client.py:221:12: W1203: Use lazy % formatting in logging functions (logging-fstring-interpolation)
tavern/_plugins/mqtt/client.py:222:27: W0212: Access to a protected member _client_id of a client class (protected-access)
tavern/_plugins/mqtt/client.py:228:8: W1203: Use lazy % formatting in logging functions (logging-fstring-interpolation)
tavern/_plugins/mqtt/client.py:229:41: W0212: Access to a protected member _client_id of a client class (protected-access)

michaelboulton avatar Jan 30 '22 10:01 michaelboulton

If there is nothing my code caused in terms of tests, can this be merged?

RFRIEDM-Trimble avatar Feb 25 '22 21:02 RFRIEDM-Trimble

To fix the lint errors, you need to change all the instances of

# pylint: disable=unused-argument

to

# pylint: disable=unused-argument,protected-access

And all the f-string usage like

 logger.debug(
            f"Client '{client._client_id.decode()}' successfully connected to the broker with result code '{paho.connack_string(rc)}'"
        )

to

 logger.debug(
          "Client '%s' successfully connected to the broker with result code '%s'", client._client_id.decode(), paho.connack_string(rc))
        )

michaelboulton avatar May 02 '22 15:05 michaelboulton

To fix the lint errors, you need to change all the instances of

# pylint: disable=unused-argument

to

# pylint: disable=unused-argument,protected-access

And all the f-string usage like

 logger.debug(
            f"Client '{client._client_id.decode()}' successfully connected to the broker with result code '{paho.connack_string(rc)}'"
        )

to

 logger.debug(
          "Client '%s' successfully connected to the broker with result code '%s'", client._client_id.decode(), paho.connack_string(rc))
        )

Thanks, will do.

RFRIEDM-Trimble avatar May 05 '22 03:05 RFRIEDM-Trimble

I've updated the PR, but Github is showing I have 20 commits and 24 files changed on here, but that's not true. image

ryan@rfriedm-us-dl01:~/Development/RFRIEDM-Trimble/tavern$ git log -n  3
commit 7e516525ee4d3c17245d73d5aa37d3d3b5682de6 (HEAD -> feature/bind-all-mqtt-callbacks, origin/feature/bind-all-mqtt-callbacks)
Author: Ryan Friedman <[email protected]>
Date:   Sat May 7 12:59:59 2022 -0600

    Use lazy f-string formatting and allow private member access

commit a4cc0c03a2167da870cf44e64c8a6cf2348949f2
Author: Ryan Friedman <[email protected]>
Date:   Sun Jan 9 20:44:21 2022 -0600

    Bind all MQTT client callbacks (#753)

commit 6c45900e97b2fcd4aae27f5892535852943da5f3 (origin/master, origin/HEAD, master)
Author: Konstantinos Xynos <[email protected]>
Date:   Mon May 2 17:20:44 2022 +0200

    Updated documentation to show how to enable tavern-merge-ext-function-values  (#767)
    
    * Update basics.md
    
    Updated basics.md to provide an example of how to include tavern-merge-ext-function-values in pytest.ini
    ```

RFRIEDM-Trimble avatar May 07 '22 19:05 RFRIEDM-Trimble

That's probably from branching off master then putting in a PR against the feature-2.0 branch - you are picking commits up from master which aren't on that branch.

michaelboulton avatar May 08 '22 14:05 michaelboulton

That's probably from branching off master then putting in a PR against the feature-2.0 branch - you are picking commits up from master which aren't on that branch.

Fixed, ready for final review

RFRIEDM-Trimble avatar May 09 '22 02:05 RFRIEDM-Trimble

Rebased back on top of feature-2.0, no changes otherwise.

RFRIEDM-Trimble avatar Aug 01 '22 03:08 RFRIEDM-Trimble

After last night's CI run, it appears to be failing on the python3.9 invocation of black.

py39black run-test: commands[0] | black -t py39 --check tavern
would reformat tavern/_plugins/mqtt/client.py

Oh no! 💥 💔 💥
1 file would be reformatted, 41 files would be left unchanged.
ERROR: InvocationError for command /home/runner/work/tavern/tavern/.tox/py39black/bin/black -t py39 --check tavern (exited with code 1)

However, my local workspace I am unable to reproduce the issue. This is affecting all my other PR's.

(.venv) rfriedm@rfriedm-us-ll6:~/Development/RFRIEDM-Trimble/tavern$ black -t py39 --check tavern
All done! ✨ 🍰 ✨
42 files would be left unchanged.

RFRIEDM-Trimble avatar Aug 01 '22 14:08 RFRIEDM-Trimble

After last night's CI run, it appears to be failing on the python3.9 invocation of black.

I'll fix it after the merge, probably a Black update. I was looking at using a pyproject.toml instead to try and lock dependencies a bit better

michaelboulton avatar Aug 29 '22 14:08 michaelboulton