-
Notifications
You must be signed in to change notification settings - Fork 13
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
feat: add mqtt internal url in tests #239
feat: add mqtt internal url in tests #239
Conversation
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.
That is exactly what I mean with the MQTT subscriptions. More general, in the Issue #238 we would also need an internal URL for other FIWARE components such as the OCB, for example in
def test_input_endpoints(self) -> None: |
tests/clients/test_ngsi_v2_cb.py
Outdated
@@ -533,6 +534,7 @@ def on_disconnect(client, userdata, reasonCode, properties=None): | |||
cb_url=settings.CB_URL) | |||
def test_notification(self): | |||
mqtt_sub_url = settings.MQTT_BROKER_URL |
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.
I think I would rename mqtt_sub_url
to mqtt_url
because only the internal URL is the subscription URL.
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.
It has been changed now
@djs0109 If I checked everything correctly, the only missing thing is an internal cb_url for the timeseries test function which I mentioned on the top:
|
@RCX112 I would rather not implement it, because this API has been deprecated in QL1.0.0 (see here orchestracities/ngsi-timeseries-api#493). I will open up another issue to test the capability with QL 1.0.0 and remove this test |
@RCX112 Could you check again, whether it is good to merge? |
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.
@djs0109 Looks good, we can merge :)
close #238