Skip to content

Make sure test collection doesn't crash if there are import errors#10242

Closed
astrofrog wants to merge 2 commits intoastropy:masterfrom
astrofrog:ignore-import-errors
Closed

Make sure test collection doesn't crash if there are import errors#10242
astrofrog wants to merge 2 commits intoastropy:masterfrom
astrofrog:ignore-import-errors

Conversation

@astrofrog
Copy link
Member

It looks like we now need to specify the --doctest-ignore-import-errors flag to avoid issues when collecting tests - this is due to a change in https://github.com/astropy/pytest-doctestplus/pull/103/files#diff-ebe8cdb0141ee1407c94011e34737c5dR171-R174 which doesn't add the flag as such (it's a pytest flag) but makes pytest-doctestplus properly take it into account.

@saimn - is this intentional, or should the default still somehow be to skip on import errors? If the fix here is needed, we could use batchpr to automatically update packages that have doctest_plus = enabled.

Copy link
Member

@pllim pllim left a comment

Choose a reason for hiding this comment

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

I think this is much less work than to add that flag individually to other repos.

Oh ops... I thought this is at the pytest-doctestplus repo. I need more coffee!

@bsipocz
Copy link
Member

bsipocz commented Apr 30, 2020

Could we rather have this as a default in pytest-doctestplus and rather opt in into import errors?

@pllim pllim added the testing label Apr 30, 2020
@pllim pllim added this to the v4.1 milestone Apr 30, 2020
@pllim pllim added the 🔥 Critical label Apr 30, 2020
@pllim
Copy link
Member

pllim commented Apr 30, 2020

Tests ran. There is a new error about existing file but at least collection is fine.

040     >>> hdu.writeto('test_dask.fits')
UNEXPECTED EXCEPTION: OSError("File 'test_dask.fits' already exists.")

@saimn
Copy link
Contributor

saimn commented Apr 30, 2020

I saw that too, and pushed a change that should fix it (at least for doctextplus 0.6).

@bsipocz
Copy link
Member

bsipocz commented Apr 30, 2020

I think, now I'm more in favour of this (scientific-python/pytest-doctestplus#108 (comment)). After all #10246 seems to be a legit collection of bugs.

Then we can hope that at some point we'll be able to remove the --doctest-ignore-import-errors flag.
Also, I suspect not all doctestplus user needs this as a batchpr, the two others I checked, photutils and astroquery are fine.

@bsipocz bsipocz modified the milestones: v4.1, v4.0.2 Apr 30, 2020
@astrofrog
Copy link
Member Author

Closing in favor of #10358

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants