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

Report lineno from doctest #2610

Merged
merged 4 commits into from Jul 24, 2017

Conversation

Projects
None yet
6 participants
@hongquan
Contributor

hongquan commented Jul 24, 2017

This is to fix pytest-sugar#122 issue.

Report lineno from doctest
This is to fix pytest-sugar#122 issue.
@coveralls

This comment has been minimized.

coveralls commented Jul 24, 2017

Coverage Status

Coverage remained the same at 92.093% when pulling af2c153 on AgriConnect:doctest-lineno into 1cf8266 on pytest-dev:master.

@RonnyPfannschmidt

This comment has been minimized.

Member

RonnyPfannschmidt commented Jul 24, 2017

not all supported python versions have this availiable

@The-Compiler

This comment has been minimized.

Member

The-Compiler commented Jul 24, 2017

@RonnyPfannschmidt Even Python 2.6 has, so what doesn't?

@RonnyPfannschmidt

This comment has been minimized.

Member

RonnyPfannschmidt commented Jul 24, 2017

@The-Compiler please excuse my mistake, the python version issues coming from travis upgrading images got me confused, i recalled a different doctest issue not supported on all python versions

@nicoddemus

Thanks @hongquan !

  • Please add a functional test for this so we can avoid a possible regression in the future.

  • Add a CHANGELOG entry in changelog, I suggest changelog/2610.bugfix with something like:

    doctests line numbers are now reported correctly, fixing `pytest-sugar#122 <https://github.com/Frozenball/pytest-sugar/issues/122>`_.
    
  • Rebase on the latest master, I just pushed a fix for the Travis issue.

@hongquan

This comment has been minimized.

Contributor

hongquan commented Jul 24, 2017

Hi, I rebased my PR and added changelog entry.

However, I have difficulty writing test case. I tried to follow examples in testing/test_doctest.py, but I don't know how to "assert" the output.

I wanted to use result.stdout.fnmatch_lines, like other examples, but what pytest prints out to the screen doesn't contain the info from DoctestItem.reportinfo(), which my PR aims to. I cannot do "fnmatch_lines" on it.

What should I do?

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Jul 24, 2017

I suggest to use testdir.inline_genitems() which will return the collected items without running the tests, then you can call item.reportinfo() directly and assert its return value.

@coveralls

This comment has been minimized.

coveralls commented Jul 24, 2017

Coverage Status

Coverage remained the same at 92.092% when pulling dea671f on AgriConnect:doctest-lineno into 81ad185 on pytest-dev:master.

@RonnyPfannschmidt

This comment has been minimized.

Member

RonnyPfannschmidt commented Jul 24, 2017

@hongquan a merge is not a rebase :/

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Jul 24, 2017

a merge is not a rebase :/

Well no biggie. 😁

@RonnyPfannschmidt

This comment has been minimized.

Member

RonnyPfannschmidt commented Jul 24, 2017

important question - is that lineno the line number in the file, or on a particular doctest element

@hongquan

This comment has been minimized.

Contributor

hongquan commented Jul 24, 2017

Hi.

I added test case. I did "merge" instead of "rebase" because rebase will overwrite Git history and make this PR invalid. I will have to close this PR and make a new PR just for the same bug fix. "Merge" and "rebase" are for the same purpose: "bring the latest update to my branch", but "merge" produces less garbage.

important question - is that lineno the line number in the file, or on a particular doctest element

According to doctest doc:

The line number within filename where this DocTest begins, or None if the line number is unavailable. This line number is zero-based with respect to the beginning of the file.

The lineno reported here may not be useful, but it is the only "line number" I can deduce from pytest's DoctestItem. At least it doesn't return None and cause other plugin crash.

@The-Compiler

This comment has been minimized.

Member

The-Compiler commented Jul 24, 2017

I did "merge" instead of "rebase" because rebase will overwrite Git history and make this PR invalid. I will have to close this PR and make a new PR just for the same bug fix.

After a rebase, you can git push --force to your branch and the PR will update. That way, it produces less "noise" because you don't have the merge commit. I agree it doesn't really matter though 😉

@The-Compiler

This comment has been minimized.

Member

The-Compiler commented Jul 24, 2017

oh, by the way:

The lineno reported here may not be useful, but it is the only "line number" I can deduce from pytest's DoctestItem. At least it doesn't return None and cause other plugin crash.

Note the "or None if the line number is unavailable" part. This should still be fixed properly in pytest-sugar.

@hongquan

This comment has been minimized.

Contributor

hongquan commented Jul 24, 2017

Note the "or None if the line number is unavailable" part. This should still be fixed properly in pytest-sugar.

That is if doctest.DocTest returns None. But here doctest.DocTest actually returns a number (1)!
If doctest.DocTest returns None, pytest-sugar has to handle None. But here doctest.DocTest doesn't return None, pytest's DoctestItem should not return None.

After a rebase, you can git push --force to your branch and the PR will update.

The "Commit" tab in this page then will contain a lot of "outdate diff".
You can get rid of "merge commit" by choosing "squash" method when merging this PR. All my commits will be "compressed" to 1.

@coveralls

This comment has been minimized.

coveralls commented Jul 24, 2017

Coverage Status

Coverage decreased (-0.04%) to 92.056% when pulling d40d774 on AgriConnect:doctest-lineno into 81ad185 on pytest-dev:master.

@The-Compiler

This comment has been minimized.

Member

The-Compiler commented Jul 24, 2017

That is if doctest.DocTest returns None. But here doctest.DocTest actually returns a number (1)!
If doctest.DocTest returns None, pytest-sugar has to handle None. But here doctest.DocTest doesn't return None, pytest's DoctestItem should not return None.

Yes, but if doctest.DocTest.lineno is None (which can happen as per the docs), pytest-sugar is still going to break. I think this change makes sense, but it still needs to fixed properly in pytest-sugar to work in all cases.

@hongquan

This comment has been minimized.

Contributor

hongquan commented Jul 24, 2017

I think this change makes sense, but it still needs to fixed properly in pytest-sugar to work in all cases.

I agree with you.

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Jul 24, 2017

Thanks @hongquan!

@nicoddemus nicoddemus merged commit 70d9f86 into pytest-dev:master Jul 24, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bsipocz

This comment has been minimized.

bsipocz commented Aug 2, 2017

FYI this breaks our doctesting in astropy and affiliated packages where we use a customized DoctestModule https://circleci.com/gh/astropy/astropy/2583

@hongquan

This comment has been minimized.

Contributor

hongquan commented Aug 2, 2017

@bsipocz
Do you think pytest should be fixed something?

@bsipocz bsipocz referenced this pull request Aug 2, 2017

Closed

Handling case when dtest is the default None #2651

2 of 4 tasks complete
@bsipocz

This comment has been minimized.

bsipocz commented Aug 2, 2017

Yes, I'm happy that this fixed pytest-sugar, but broke all the others that had used the default None. See my PR for the fix, probably a test will be needed, but I wasn't sure where to add it.

@hongquan

This comment has been minimized.

Contributor

hongquan commented Aug 2, 2017

Thanks.

Sorry, I thought that self.dtest would always be set.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment