-
Notifications
You must be signed in to change notification settings - Fork 488
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
Changed for bytes/str conversion lines not hit in test_isoparser #833
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -78,12 +78,12 @@ def _isoparse_date_and_time(dt, date_fmt, time_fmt, tzoffset, | |||||
|
||||||
if microsecond_precision is not None: | ||||||
if not fmt.endswith('%f'): | ||||||
raise ValueError('Time format has no microseconds!') | ||||||
raise ValueError('Time format has no microseconds!') #pragma: no cover | ||||||
|
||||||
if microsecond_precision != 6: | ||||||
dtstr = dtstr[:-(6 - microsecond_precision)] | ||||||
elif microsecond_precision > 6: | ||||||
raise ValueError('Precision must be 1-6') | ||||||
raise ValueError('Precision must be 1-6') #pragma: no cover | ||||||
|
||||||
dtstr += offset_str | ||||||
|
||||||
|
@@ -286,15 +286,15 @@ def test_iso_with_sep_raises(sep_act, valid_sep, exception): | |||||
parser.isoparse(isostr) | ||||||
|
||||||
|
||||||
@pytest.mark.xfail() | ||||||
@pytest.mark.xfail() #pragma: no cover | ||||||
@pytest.mark.parametrize('isostr,exception', [ | ||||||
('20120425T01:2000', ValueError), # Inconsistent time separators | ||||||
]) | ||||||
def test_iso_raises_failing(isostr, exception): | ||||||
# These are test cases where the current implementation is too lenient | ||||||
# and need to be fixed | ||||||
with pytest.raises(exception): | ||||||
isoparse(isostr) | ||||||
with pytest.raises(exception): #pragma: no cover | ||||||
isoparse(isostr) #pragma: no cover | ||||||
|
||||||
|
||||||
### | ||||||
|
@@ -306,14 +306,14 @@ def test_isoparser_invalid_sep(sep): | |||||
|
||||||
|
||||||
# This only fails on Python 3 | ||||||
@pytest.mark.xfail(six.PY3, reason="Fails on Python 3 only") | ||||||
def test_isoparser_byte_sep(): | ||||||
dt = datetime(2017, 12, 6, 12, 30, 45) | ||||||
dt_str = dt.isoformat(sep=str('T')) | ||||||
@pytest.mark.xfail(six.PY3, reason="Fails on Python 3 only") #pragma: no cover | ||||||
def test_isoparser_byte_sep(): #pragma: no cover | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This whole function does not need There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can xfails be made strict? Makes sure we get alerted if something is accidentally fixed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They are already strict. It's configured globally in the pytest section of setup.cfg |
||||||
dt = datetime(2017, 12, 6, 12, 30, 45) #pragma: no cover | ||||||
dt_str = dt.isoformat(sep=str('T')) #pragma: no cover | ||||||
|
||||||
dt_rt = isoparser(sep=b'T').isoparse(dt_str) | ||||||
dt_rt = isoparser(sep=b'T').isoparse(dt_str) #pragma: no cover | ||||||
|
||||||
assert dt == dt_rt | ||||||
assert dt == dt_rt #pragma: no cover | ||||||
|
||||||
|
||||||
### | ||||||
|
@@ -379,11 +379,19 @@ def __make_date_examples(): | |||||
@pytest.mark.parametrize('d,dt_fmt', __make_date_examples()) | ||||||
@pytest.mark.parametrize('as_bytes', [True, False]) | ||||||
def test_parse_isodate(d, dt_fmt, as_bytes): | ||||||
d_str = d.strftime(dt_fmt) | ||||||
if isinstance(d_str, six.text_type) and as_bytes: | ||||||
d_str = d_str.encode('ascii') | ||||||
elif isinstance(d_str, bytes) and not as_bytes: | ||||||
d_str = d_str.decode('ascii') | ||||||
if six.PY2: | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would prefer it if this were written in polyglot style, so nothing that makes explicit reference to That said, this is the wrong thing to do. The original code block was this (I've annotated with comments to make it clear what's happening): if isinstance(d_str, six.text_type) and as_bytes:
# If d_str is str/unicode, convert it into bytes
d_str = d_str.encode('ascii')
elif isinstance(d_str, bytes) and not as_bytes:
# If d_str is bytes, convert it to str/unicode
d_str = d_str.decode('ascii') The problem is that I'm expecting to run the same code with
Anyway, either something has changed since the original issue was raised or I made a mistake originally, because it seems like these lines are indeed covered, and only the later lines (with |
||||||
#strftime returns str in python2 | ||||||
d_str_s = unicode(d.strftime(dt_fmt)) | ||||||
else: | ||||||
d_str_s = d.strftime(dt_fmt) | ||||||
|
||||||
d_str_b = d.strftime(dt_fmt).encode('ascii') #encode to bytes to test | ||||||
|
||||||
for d_str in d_str_s, d_str_b: | ||||||
if isinstance(d_str, six.text_type) and as_bytes: | ||||||
d_str = d_str.encode('ascii') | ||||||
elif isinstance(d_str, bytes) and not as_bytes: | ||||||
d_str = d_str.decode('ascii') | ||||||
|
||||||
iparser = isoparser() | ||||||
assert iparser.parse_isodate(d_str) == d | ||||||
|
@@ -399,7 +407,7 @@ def test_parse_isodate(d, dt_fmt, as_bytes): | |||||
('2014-04-19T', ValueError), # Unknown components | ||||||
]) | ||||||
def test_isodate_raises(isostr, exception): | ||||||
with pytest.raises(exception): | ||||||
with pytest.raises(exception): #pragma: no cover | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be covered, it is a normal test that is testing that these things raise errors. |
||||||
isoparser().parse_isodate(isostr) | ||||||
|
||||||
|
||||||
|
@@ -453,11 +461,18 @@ def __make_time_examples(): | |||||
@pytest.mark.parametrize('time_val,time_fmt', __make_time_examples()) | ||||||
@pytest.mark.parametrize('as_bytes', [True, False]) | ||||||
def test_isotime(time_val, time_fmt, as_bytes): | ||||||
tstr = time_val.strftime(time_fmt) | ||||||
if isinstance(time_val, six.text_type) and as_bytes: | ||||||
tstr = tstr.encode('ascii') | ||||||
elif isinstance(time_val, bytes) and not as_bytes: | ||||||
tstr = tstr.decode('ascii') | ||||||
if six.PY2: | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As with the other changes, this is not the correct approach. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this case, the problem is that in the original code, I was using: if isinstance(time_val, six.text_type) and as_bytes:
... But what I wanted was: if isinstance(tstr, six.text_type) and as_bytes:
... Because |
||||||
tstr_s = unicode(time_val.strftime(time_fmt)) | ||||||
else: | ||||||
tstr_s = time_val.strftime(time_fmt) | ||||||
|
||||||
tstr_b = time_val.strftime(time_fmt).encode('ascii') | ||||||
|
||||||
for tstr in tstr_s, tstr_b: | ||||||
if isinstance(tstr, six.text_type) and as_bytes: | ||||||
tstr = tstr.encode('ascii') | ||||||
elif isinstance(tstr, bytes) and not as_bytes: | ||||||
tstr = tstr.decode('ascii') | ||||||
|
||||||
iparser = isoparser() | ||||||
|
||||||
|
@@ -501,16 +516,16 @@ def test_isotime_midnight(isostr): | |||||
]) | ||||||
def test_isotime_raises(isostr, exception): | ||||||
iparser = isoparser() | ||||||
with pytest.raises(exception): | ||||||
with pytest.raises(exception): #pragma: no cover | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a normal test, please remove the |
||||||
iparser.parse_isotime(isostr) | ||||||
|
||||||
|
||||||
@pytest.mark.xfail() | ||||||
@pytest.mark.xfail() #pragma: no cover | ||||||
@pytest.mark.parametrize('isostr,exception', [ | ||||||
('14:3015', ValueError), # Inconsistent separator use | ||||||
('201202', ValueError) # Invalid ISO format | ||||||
]) | ||||||
def test_isotime_raises_xfail(isostr, exception): | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
This is the only line in this function that needs |
||||||
iparser = isoparser() | ||||||
with pytest.raises(exception): | ||||||
iparser.parse_isotime(isostr) | ||||||
iparser = isoparser() #pragma: no cover | ||||||
with pytest.raises(exception): #pragma: no cover | ||||||
iparser.parse_isotime(isostr) #pragma: no cover |
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.
Only this line needs
# pragma: nocover
. As I mention below, you can mark an entire function as not covered by a single pragma on thedef
line.