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

#863 verify_response_with fix #864

Merged

Conversation

octathorp
Copy link
Contributor

This fixes issue #863 as stated in the comments.

Comment on lines 134 to 136
mqtt_responses = response_block.get("mqtt_responses")
for mqtt_response in mqtt_responses:
check_ext_functions(mqtt_response.get("verify_response_with", None))
Copy link
Member

Choose a reason for hiding this comment

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

This won't work because the response block can still be just a single mqtt response, or a rest response, or (soon) a gRPC response.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I tried with mqtt is working, with single or multiple responses, as even single responses reach this method in the json array form (with only one item, of course). You are totally right about rest response, simply I'm not using rest so I did not think about that.
No idea about future gRPC implementation, so no words about that.

I can try to update pull request with a proper rest response handling if that is enough, but I have no way to make a gRPC-future proof implementation. I was simply trying to fix the bug I reported some days ago.

Copy link
Member

Choose a reason for hiding this comment

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

You don't need to worry about the grpc thing, I'll handle that in the appropriate PR, I mainly just meant it still needs to work for rest responses. Something like

if mqtt_responses := response_block.get("mqtt_responses"):
  for mqtt_response in mqtt_responses:
    check_ext_functions(mqtt_response.get("verify_response_with", None))
else:
  check_ext_functions(response_block.get("verify_response_with", None))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated PR with your code suggestion, thank you!

@michaelboulton michaelboulton merged commit 7464cab into taverntesting:master Nov 18, 2023
7 checks passed
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.

2 participants