-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Fixes #5054: Enhance Should Contains for bytes #5097
base: master
Are you sure you want to change the base?
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.
Looks very good. See the inline comments for things that could be still enhanced. At least the documentation change needs to be done, other's are just "nice to have".
@@ -1042,7 +1042,7 @@ def should_contain(self, container, item, msg=None, values=True, | |||
ignore_case=False, strip_spaces=False, collapse_spaces=False): | |||
"""Fails if ``container`` does not contain ``item`` one or more times. | |||
|
|||
Works with strings, lists, and anything that supports Python's ``in`` | |||
Works with strings, lists, bytes, and anything that supports Python's ``in`` |
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 should be explained somewhere in the docs that the item
is converted from sting to bytes automatically f the container is bytes. There should also be a note that this functionality is new in RF 7.1.
[Template] Should Contain | ||
${BYTES} ${1000} | ||
${BYTES} ${UNICODE_NON_LATIN} | ||
${BYTES} \xA0 |
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 doesn't really matter, but it would be kind of better to have passing examples first.
- It would probably be a good idea to add a passing example where the item that's search for is an integer like ${1}.
- An example where the item itself is bytes would be good too. Could use the inline Python evaluation syntax like
${{b'\x02\xa0'}}
. - An example that fails so that there's no conversion error would be good too.
- Not sure does it make sense to have all these examples in a single test. There could, for example, be one test where the item is bytes, another where it's an integer, and one where it's string. You can decide.
try: | ||
item = item.encode('latin-1') | ||
except UnicodeEncodeError: | ||
raise ValueError(f"{item} can not be encoded into bytes.") |
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.
You added a test where the item is 1000
and it fails for a ValueError. Should we actually validate that, if the item is an integer, it is in range 0-255 and provide a nicer error than the current byte must be in range(0, 256)
that Python gives us? That would also avoid the issue with the error possibly changing in some future Python version.
Implemented as described in the issue.
Fixes #5054.