Compatibility with Pytest 5.4#103
Conversation
| """ | ||
| ) | ||
| testdir.inline_run(p, '--doctest-plus', '--doctest-rst').assertoutcome(passed=1) | ||
| testdir.inline_run(p, '--doctest-plus', '--doctest-rst').assertoutcome(skipped=1) |
There was a problem hiding this comment.
This is testing that the test is skipped because of a missing import, I guess the outcome should be "skipped".
There was a problem hiding this comment.
how did all of these tests work before?
| ) | ||
| # passed because 'pytest<1.0' was not satisfied and 'assert 0' was not evaluated | ||
| testdir.inline_run(p, '--doctest-plus', '--doctest-rst').assertoutcome(passed=1) | ||
| testdir.inline_run(p, '--doctest-plus', '--doctest-rst').assertoutcome(skipped=1) |
| '--doctest-glob', '*.tex', | ||
| '--doctest-glob', '*.txt' | ||
| ).assertoutcome(passed=3) | ||
| ).assertoutcome(passed=0) |
There was a problem hiding this comment.
This is testing that the tests are commented, so they are not run, they should not "pass". Same below.
| str(self.fspath), module_relative=False, | ||
| optionflags=options, parser=DocTestParserPlus(), | ||
| extraglobs=dict(getfixture=fixture_request.getfixturevalue), | ||
| raise_on_error=True, verbose=False, encoding='utf-8') |
There was a problem hiding this comment.
Ok so the tests differences are due to this removal of doctest.testfile, which is because I basically copied what pytest's doctest plugin is doing, to remove the multiple inheritance, and use collect instead of runtest. I don't know why the old code was doing it another way so ... 🤷♂️
|
There are still issues with pytest><4 versions. Those are however 1.5 years old, so we may even consider dropping them if the fix is more complicated than necessary. |
|
Oh sorry, I thought I read somewhere that pytest's minversion was 5.0, but I must have mistaken it for another package. Anyway, for tests dependencies I think it is easier to require recent versions, so indeed I'd be happy if we can drop old versions. |
|
ok, let's wait for @astrofrog's opinion. And nominally @eteq is also listed as testing maintainer, in case he wants to chime in... |
astrofrog
left a comment
There was a problem hiding this comment.
I'd be fine with dropping support for pytest<4 - I'm not super familiar with this code, but if (a) the test suite here passes and (b) if the astropy core test suite passes with this, then 👍 from me 😄
|
@saimn - Shall I open a separate PR for the travis updates, or push directly to your branch? There are other unrelated issues with the config, so would ideally all need to be cleaned up before this is merged (to be sure this works). |
|
@astrofrog - yes it works fine with astropy. The test cases I had to changes were edge cases, and I mostly copied code from pytest's doctest plugin, so hopefully it should be fine. @bsipocz - sure, push here if you want. The travis config indeed needs some updates. |
b466e88 to
1e4152b
Compare
|
This now drops support for python <3.6 as well as for pytest <4. |
|
I've also added a job for pytest dev version, and switched on the weekly cron. Now this is very much ready for merging. |
|
No vetoes here in the past week, so I'm merging and will go ahead with a release fairly soon. Thank you @saimn for picking this up and fixing it. |
| if self.config.getvalue("doctest_ignore_import_errors"): | ||
| pytest.skip("unable to import module %r" % self.fspath) | ||
| else: | ||
| raise |
There was a problem hiding this comment.
This changes dramatically the behavior of the plugin. Now if any module in the repo using this plugin has import failures, it will cause a collection error. In the past for this plugin, this was ignored. I see that using --doctest-ignore-import-errors can turn this off, but it would be good to document this in the README and the release notes.
There was a problem hiding this comment.
See astropy/astropy#10242 for a possible fix in astropy btw, we should discuss there whether that is the correct fix.
There was a problem hiding this comment.
Hmm, sorry for the breakage. The goal was just to reduce the number of difference with pytest's doctest plugin, because of lot of the code here is copied from there, and reducing the differences should make it easier to maintain...
But yeah as previously this was ignored by default it is probably easier to fix this here.
There was a problem hiding this comment.
No worries. I think it is easy to add --doctest-ignore-import-errors to addopts in setup.cfg, it was just surprising. And it took some time to debug. Hence the desire of making this clear in the documentation.
There was a problem hiding this comment.
Also, it was nice finding out we had tumbleweed code laying about that had imports that didn't work.
I agree that having its behavior similar to pytest's doctest is good. The more things that are the same, not only easier to maintain, but the less unexpected behavior for people using the plugin as an extension or replacement of the pytest one.
There was a problem hiding this comment.
see a few open issues about moving these upstream and consolidate the differences between doctest and doctestplus. The blocking factor is time, and now I also think it's better to wait for the situation at pytest to resolve.
There was a problem hiding this comment.
also, it's my bad that I didn't pick this up during the review, I suppose I put too much trust in tests and was just too happy when this PR finally had a green status.
|
Has this made its way into a release yet? |
|
Thanks! |
This is quite similar to #95, but I started from scratch and porting some code from pytest's doctest plugin which probably changed a lot since the first version of doctestplus.
With these changes I had a few tests failing, and I actually don't understand why the tests were not failing before so I updated the tests. Please review, not sure who knows this code best ?