Skip to content
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

mqtt: fix unexpected behaviour #823

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

stintel
Copy link

@stintel stintel commented Nov 18, 2022

Currently mqtt_response with the unexpected key set to true fails if no message is received. This is strange, was we don't expect a message, and should only fail if a message is received.

Formatted stage:
  mqtt_publish:
    payload: 10.10.10.10
    topic: inet6/add
  mqtt_response:
    payload: !anything ''
    timeout: 5
    topic: vallumd/will
    unexpected: true

Errors:
E   tavern.util.exceptions.TestFailError: Test 'add IPv4 IP to IPv6 ipset' failed:
    - Expected '<Tavern YAML sentinel for anything>' on topic 'vallumd/will' but no such message received

Fix this by suppressing the error MQTTResponse._await_response() and returning an empty dict if the unexpected key is set.

Signed-off-by: Stijn Tintel [email protected]

Comment on lines 184 to 189
if self.expected.get("unexpected"):
return {}
else:
self._adderr(
"Expected '%s' on topic '%s' but no such message received",
expected_payload,
topic,
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if self.expected.get("unexpected"):
return {}
else:
self._adderr(
"Expected '%s' on topic '%s' but no such message received",
expected_payload,
topic,
)
if not self.expected.get("unexpected"):
self._adderr(
"Expected '%s' on topic '%s' but no such message received",
expected_payload,
topic,
)

Returning early will break the saving of variables from responses, this should do what is required

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe I tried that at first and hit other errors. Let me retry.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed: tavern.txt

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My initial submission works, your suggestion including my extra attempts to fix it, doesn't. Any other ideas/suggestions?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I've been looking at the 'multiple mqtt responses' feature in Tavern 2.0 and wasn't thinking about there only being one mqtt response.

I think that what needs to be done is that my suggestion does need to be applied, but then also just after it checks if there were any errors (

if self.errors:
if warnings:
self._adderr("\n".join(warnings))
raise exceptions.TestFailError(
"Test '{:s}' failed:\n{:s}".format(self.name, self._str_errors()),
failures=self.errors,
)
) it should do

if not msg:
    return {}

because there won't be any variables to save.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made a slight alteration to your suggestion, seems to work like that. Let me know if that's OK for you?

@stintel stintel force-pushed the mqtt_fix_unexpected branch 2 times, most recently from 3eba49b to 91828dd Compare November 22, 2022 22:49
@stintel stintel force-pushed the mqtt_fix_unexpected branch from 91828dd to 44b1252 Compare December 9, 2022 08:49
Currently mqtt_response with the unexpected key set to true fails if no
message is received. This is strange, was we don't expect a message, and
should only fail if a message is received.

Formatted stage:
  mqtt_publish:
    payload: 10.10.10.10
    topic: inet6/add
  mqtt_response:
    payload: !anything ''
    timeout: 5
    topic: vallumd/will
    unexpected: true

Errors:
E   tavern.util.exceptions.TestFailError: Test 'add IPv4 IP to IPv6 ipset' failed:
    - Expected '<Tavern YAML sentinel for anything>' on topic 'vallumd/will' but no such message received

Fix this by adding an extra check for the expected key when we received
no message.

Signed-off-by: Stijn Tintel <[email protected]>
@stintel stintel force-pushed the mqtt_fix_unexpected branch from ed92a00 to 3c6d68f Compare December 9, 2022 11:25
@michaelboulton
Copy link
Member

I think the CI has randomly broken again, will try to fix it

@stintel
Copy link
Author

stintel commented Dec 10, 2022

I think the CI has randomly broken again, will try to fix it

Thanks, no rush, I can pip install tavern from my branch which allows me to continue working on the CI I'm using it in!

@felixbohnus
Copy link

I also stumbled over this bug and tested the proposed solution with the local installation. I'm not sure about the processes: should I create a new pull request or should this pull request be updated?

felixbohnus pushed a commit to felixbohnus/tavern that referenced this pull request Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants