-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
✨ Allowing pytest cli arguments for host and port #8
✨ Allowing pytest cli arguments for host and port #8
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8 +/- ##
==========================================
+ Coverage 95.07% 95.45% +0.38%
==========================================
Files 5 5
Lines 142 154 +12
==========================================
+ Hits 135 147 +12
Misses 7 7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Dear Mario, thank you so much for submitting this patch, and apologies for missing it up until now. Your contribution is well received, and will be part of the next release of pytest-mqtt. With kind regards, |
pyproject.toml
Outdated
[project.entry-points.pytest11] | ||
mqttcliargs = "pytest_mqtt.mqttcliargs" | ||
capmqtt = "pytest_mqtt.capmqtt" | ||
mosquitto = "pytest_mqtt.mosquitto" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't think that new mqttcliargs
fixture needs to be exported for public consumption. Now that the new {host,port} information is also available as attributes on MqttClientAdapter
, it can easily be accessed through the capmqtt
fixture, right?
def test_foo(capmqtt):
assert capmqtt.mqtt_client.host == "localhost"
assert capmqtt.mqtt_client.port == 1883
For improved convenience, they could be mirrored / also accessed by additional properties on MqttCaptureFixture
, like:
class MqttCaptureFixture:
@property
def host(self):
return self.mqtt_client.host
@property
def port(self):
return self.mqtt_client.port
def test_foo(capmqtt):
assert capmqtt.host == "localhost"
assert capmqtt.port == 1883
testing/conftest.py
Outdated
parser.addoption("--mqtt_host", action="store", default="localhost", help="mqtt host to be connected through") | ||
parser.addoption("--mqtt_port", action="store", default=1883, help="mqtt port to be connected through") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a suggestion about "naming things".
parser.addoption("--mqtt_host", action="store", default="localhost", help="mqtt host to be connected through") | |
parser.addoption("--mqtt_port", action="store", default=1883, help="mqtt port to be connected through") | |
parser.addoption("--mqtt-host", action="store", default="localhost", help="mqtt host to be connected through") | |
parser.addoption("--mqtt-port", action="store", default=1883, help="mqtt port to be connected through") |
testing/conftest.py
Outdated
@@ -0,0 +1,3 @@ | |||
def pytest_addoption(parser) -> None: | |||
parser.addoption("--mqtt_host", action="store", default="localhost", help="mqtt host to be connected through") | |||
parser.addoption("--mqtt_port", action="store", default=1883, help="mqtt port to be connected through") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about conveying type=int
?
-- https://docs.python.org/3/library/argparse.html#type
parser.addoption("--mqtt_port", action="store", default=1883, help="mqtt port to be connected through") | |
parser.addoption("--mqtt_port", action="store", type=int, default=1883, help="mqtt port to be connected through") |
pytest_mqtt/mqttcliargs.py
Outdated
@pytest.fixture(scope="session") | ||
def mqttcliargs(request) -> t.Tuple[str, int]: | ||
host = request.config.getoption("--mqtt_host", "localhost") | ||
port = int(request.config.getoption("--mqtt_port", 1883)) | ||
yield host, port |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about conveying
type=int
?
We think the mqttcliargs
shortcut fixture, for carrying around host/port information like this, should be dissolved completely.
Instead, we suggest to use pytestconfig
, for acquiring configuration options. Because default values are already defined in pytest_addoption
, there is no need to define them once again redundantly.
pytest_mqtt/capmqtt.py
Outdated
def capmqtt(request): | ||
def capmqtt(request, mqttcliargs): | ||
"""Access and control MQTT messages.""" | ||
|
||
# Configure `capmqtt` fixture, obtaining the `capmqtt_decode_utf8` setting from | ||
# either a global or module-wide setting, or from a test case marker. | ||
# https://docs.pytest.org/en/7.1.x/how-to/fixtures.html#fixtures-can-introspect-the-requesting-test-context | ||
# https://docs.pytest.org/en/7.1.x/how-to/fixtures.html#using-markers-to-pass-data-to-fixtures | ||
|
||
host, port = mqttcliargs | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead, you may already be able to use the built-in pytestconfig
fixture, in order to retrieve configuration options.
def capmqtt(request, pytestconfig):
host = pytestconfig.getoption("--host")
port = int(pytestconfig.getoption("--port"))
-- https://docs.pytest.org/en/8.0.x/reference/reference.html#std-fixture-pytestconfig
capmqtt_decode_utf8 = ( | ||
getattr(request.config.option, "capmqtt_decode_utf8", False) | ||
or getattr(request.module, "capmqtt_decode_utf8", False) | ||
or request.node.get_closest_marker("capmqtt_decode_utf8") is not None | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When acquiring configuration options like this, they can be provided through three different means, see usage of capmqtt_decode_utf8
.
Acquiring capmqtt_host
and capmqtt_port
could be implemented in the very same way, so they are not too special?
... in order to connect to an MQTT broker on a different endpoint than `localhost:1883`.
Dear Mario, we push a few adjustments to your branch, along the lines of our suggestions, and renamed the CLI parameters to With kind regards, |
Hi again, pytest-mqtt 0.4.0 has been released, including corresponding improvements from your pen, slightly adjusted. Thank you very much! With kind regards, |
TL;DR
Adding two optional pytest command line arguments describing a MQTT broker:
--mqtt_host
(defaults tolocalhost
)--mqtt_port
(defaults to1883
)Hi there,
First of all, thank you for the project. It's been super helpful while developing MQTT related features.
However, while using the library I found it didn't quite match my current needs, so I tried to find a way to solve my issue. I hope you find this PR suitable for your package. Since it's my first time contributing to your project, I'm not 100% sure I'm covering all the requirements. Feel free to suggest any changes you may find to match your criteria, please.
I needed to test against a remote MQTT broker, not running on localhost. All connections to the MQTT broker were hardcoded to
localhost:1883
though. I added the chance to make both values configurable, based on pytest cli arguments, using therequest
fixture.Example of use
Given the following docker-compose.yml file
and starting the MQTT server on port
11883
running pytest against the new port works
and the logs on the mqtt broker running on docker: