Skip to content

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

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

astrofrog wants to merge 2 commits into
astropy:masterfrom
astrofrog:ignore-import-errors

Conversation

@astrofrog

Copy link
Copy Markdown
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.

@pllim pllim left a comment •

Copy link
Copy Markdown
Member

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

bsipocz commented Apr 30, 2020

Copy link
Copy Markdown
Member

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

pllim commented Apr 30, 2020

Copy link
Copy Markdown
Member

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

saimn commented Apr 30, 2020

Copy link
Copy Markdown
Contributor

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

@bsipocz

bsipocz commented Apr 30, 2020

Copy link
Copy Markdown
Member

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
Copy Markdown
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