-
Notifications
You must be signed in to change notification settings - Fork 58
Adding web app tests #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
Conversation
Signed-off-by: Denis Makogon <denys.makogon@oracle.com>
20adb02
to
0972b50
Compare
@evankanderson please review. |
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.
A few comments, but looks good overall.
@@ -159,5 +158,4 @@ def MarshalBinary( | |||
headers["ce-{0}".format(key)] = value | |||
|
|||
data, _ = self.Get("data") | |||
return headers, io.BytesIO( | |||
str(data_marshaller(data)).encode("utf-8")) | |||
return headers, data_marshaller(data) |
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.
This changes the output to be the result of the data_marshaller, rather than enforcing that the data is utf-8 encoded.
Not sure if this is intentional or not.
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.
my original idea doesn't really work well, so, I think it would be simpler to let people decide what they want to do with the event data.
for instance, if you'd like to get an io.Bytes
from marshaller you can do the following:
m.ToRequest(event, lambda x: io.BytesIO(str(x).encode("utf-8")))
but forcing them to do things they don't want is a bad idea, though.
cloudevents/sdk/exceptions.py
Outdated
super().__init__("Invalid CloudEvent class: " | ||
"'{0}'".format(event_class)) | ||
super().__init__( | ||
"Invalid CloudEvent class: " "'{0}'".format(event_class) |
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.
This is strange -- I'd collapse the " "
in this case.
@@ -35,14 +35,16 @@ def __init__(self, converters: typing.List[base.Converter]): | |||
:param converters: a list of HTTP-to-CloudEvent-to-HTTP constructors | |||
:type converters: typing.List[base.Converter] | |||
""" | |||
self.__converters = (c for c in converters) | |||
self.__converters = [c for c in converters] |
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.
D'oh -- I was too clever!
Thanks!
return response.text(body, headers=hs) | ||
|
||
|
||
def test_reusable_marshaller(): |
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.
This is great, I suspect that just calling .FromRequest
5 times would have caught my bug.
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.
well, maybe, but 10 is 2 times greater than 5, so we have double-insurance here =)
Signed-off-by: Denis Makogon <denys.makogon@oracle.com>
Looks Good To Me (LGTM). |
thanks, merging. |
Closes: #12