Changed for bytes/str conversion lines not hit in test_isoparser

[jramacha]

Summary of changes

@pganssle mentioned some branches were not hit in test_isoparser.py. Ran the test and found that the test weren't hitting the is instance(param, six.text_type) for python2.7 , because the string value returned from strftime was not a unicode string, so converted that to unicode and made sure the test were hitting both the branches. And test weren't hitting the isinstance(param, bytes) for python3+ because the string value returned from strftime always failed that check as it was unicode string. So added a line to encode that string to ascii and made sure the test hit the other branch.

Also added #pragma: no cover to lines not covered.

Closes #776

[pganssle]

Thank you for your attempt to fix this issue, but as it stands this PR currently adds many more issues and doesn't solve the original problems.

To get it into shape, please address my inline comments. The big picture items are:

• Revert your changes to test_parse_isodate
• Revert your changes to test_isotime and add the correct fix (changing time_val to tstr in the isinstance checks)
• Remove the spurious # pragma: nocover statements on already-covered tests
• For unconditional xfail tests (not the one that only fails on Python 3), change it so that only the def line has # pragma: nocover.

As a guide, take a look at the current coverage report. Do not add # pragma: nocover to anything where the line is already green.

Thanks!

[pganssle]

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.

[pganssle]

This should be covered, it is a normal test that is testing that these things raise errors.

[pganssle]

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

[pganssle]

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

[pganssle]

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

[pganssle]

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.

[pganssle]

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

[jbrockmendel]

Can xfails be made strict? Makes sure we get alerted if something is accidentally fixed.

[pganssle]

They are already strict. It's configured globally in the pytest section of setup.cfg

[pganssle]

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.