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.

Sign up for GitHub

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

Jump to bottom

Changed for bytes/str conversion lines not hit in test_isoparser #833

Closed
wants to merge 6 commits into from
Closed

Changed for bytes/str conversion lines not hit in test_isoparser #833

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a16ac21
debug
Oct 29, 2018
157ab6f
changes to bytes and str test branches
Oct 30, 2018
7eef087
change comparision to the tstr not time_val
Oct 30, 2018
3533333
added #pragma: no cover
Oct 30, 2018
ca17d26
dt_rt
Oct 30, 2018
0375d30
Merge branch 'master' into master
jramacha Oct 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
69 changes: 42 additions & 27 deletions dateutil/test/test_isoparser.py
View file
Open in desktop
Expand Up @@ -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

Expand Down Expand Up @@ -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):
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
def test_iso_raises_failing(isostr, exception):
def test_iso_raises_failing(isostr, exception): # pragma: nocover

Only this line needs # pragma: nocover. As I mention below, you can mark an entire function as not covered by a single pragma on the def line.

# 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


###
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

This whole function does not need # pragma: nocover because it is indeed already covered by the Python 2 tests.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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


###
Expand Down Expand Up @@ -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:
Copy link
Member

Choose a reason for hiding this comment

The 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 six.PY2.

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 as_bytes=True and as_bytes=False, which means that whatever d_str is, it should have to be converted to the other thing half the time. In Python 2 I expect it to be bytes and in Python 3 I expect it to be str, so both of these branches should already be hit.

unicode(b'example') is the wrong way to convert from bytes to string, the correct way is b'example'.encode(), which is what I'm already doing.

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 time_val) are having the coverage problem, so these changes can be removed entirely.

#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
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

The 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)


Expand Down Expand Up @@ -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:
Copy link
Member

Choose a reason for hiding this comment

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

As with the other changes, this is not the correct approach.

Copy link
Member

Choose a reason for hiding this comment

The 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 time_val is the time input, not the output of time_val.strftime (which will either be bytes or str depending on the Python version).

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()

Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

This is a normal test, please remove the #pragma: nocover

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):
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
def test_isotime_raises_xfail(isostr, exception):
def test_isotime_raises_xfail(isostr, exception): # pragma: nocover

This is the only line in this function that needs # pragma; nocover. The pragma goes after the function signature line on each of the xfail tests, and not in the body or on the decorator.

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