Skip to content

Show warnings in pytest and turn them into errors#7928

Merged
bsipocz merged 15 commits intoastropy:masterfrom
pllim:test-show-warn
Nov 18, 2019
Merged

Show warnings in pytest and turn them into errors#7928
bsipocz merged 15 commits intoastropy:masterfrom
pllim:test-show-warn

Conversation

@pllim
Copy link
Member

@pllim pllim commented Oct 19, 2018

We are ignoring warnings right now but as @bsipocz stated, they should be cleaned up at some point. This PR show them for the purpose of clean-up campaign.

Update: Over the months, I might have lost track of some PRs that got merge that got rid of the warnings. I apologize if I missed your PR(s). Please update the list above accordingly if you so desired. See the xref PRs in the comments thread below.

Fun fact: #6163

@astropy-bot

This comment has been minimized.

@pllim

This comment has been minimized.

@bsipocz
Copy link
Member

bsipocz commented Oct 19, 2018

Whee, only 2089 warnings...

well, there was good reasons for adding the ignorance to the config 😅

@mhvk
Copy link
Contributor

mhvk commented Oct 19, 2018

Wow. Many seem to be of the type where a test should either be properly written so that it doesn't trigger it, or explicitly look for the warning. Fortunately, there are a lot of duplications (e.g., VOunit test giving warning about a unit being deprecated; ERFA tests warning about bad years, etc.) It does look like we better tackle this module by module...

@pllim pllim added the sprint label Oct 26, 2018
@pllim
Copy link
Member Author

pllim commented Oct 26, 2018

Adding a "sprint" tag in case people feel like sprinting for this one, one subpackage at a time.

@astrofrog
Copy link
Member

The warnings about figures not being closed by Matplotlib can be fixed by making sure that pytest-mpl is installed (even if it's not actually enabled with --mpl)

@astrofrog
Copy link
Member

@pllim - can you install pytest-mpl for the job where you are checking the warnings? I think we might want to just add it as one of the packages that gets installed with pytest-astropy so it's always there by default.

@pllim
Copy link
Member Author

pllim commented Oct 27, 2018

re: pytest-mpl -- Didn't it used to be in pytest-astropy but then taken out due to some problems? 🤔

@pllim
Copy link
Member Author

pllim commented Oct 27, 2018

@astrofrog -- The build with pytest-mpl https://travis-ci.org/astropy/astropy/jobs/447178372

@astrofrog
Copy link
Member

We are now at 2148 warnings. Huh. 😕

@bsipocz
Copy link
Member

bsipocz commented Oct 27, 2018

hmm, it's worse than when we've started?

@pllim
Copy link
Member Author

pllim commented Oct 27, 2018

😱 I hope I didn't accidentally make things worse in my attempts to fix them. 😱

@pllim
Copy link
Member Author

pllim commented Oct 27, 2018

Ah, but before we go into full panic. The earlier number was from Python 3.5 run, while the latter one from Python 3.7. If I go back to that earlier log and pick the Python 3.7 build (https://travis-ci.org/astropy/astropy/jobs/443714273), there were actually 2689 warnings. So now we have 541 less. 😬

@bsipocz
Copy link
Member

bsipocz commented Oct 27, 2018

Ohh, that's a good point about the python version. I assumed we have more warnings due to the new PRs merged in the past few days not because of the cleanup efforts :)

@drdavella
Copy link
Contributor

I need to refresh myself on exactly how this works, but I wonder how many of these warnings come from outside astropy? If you enable warnings using the Python interpreter outside of pytest, you end up getting warnings from all kinds of libraries that we don't really care about.

assert type(t) is iers.IERS_B

@pytest.mark.remote_data
@pytest.mark.xfail(reason='See issue 9600')
Copy link
Member Author

Choose a reason for hiding this comment

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

I marked this whole test as xfail until #9600 is fixed. Also reverted my other test modifications to this file, @astrofrog .

@pllim
Copy link
Member Author

pllim commented Nov 18, 2019

Thanks for the review everyone! I rebased and addressed the latest round of comments. Unless new warnings were introduced in the commits over the weekend, hopefully we're good to go now.

Copy link
Member

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

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

Let's get on with this.

@bsipocz
Copy link
Member

bsipocz commented Nov 18, 2019

Thanks @pllim!

@bsipocz bsipocz merged commit a87d96e into astropy:master Nov 18, 2019
@pllim pllim deleted the test-show-warn branch November 18, 2019 02:51
@pllim
Copy link
Member Author

pllim commented Nov 18, 2019

Whew. That only took 13 months to clear over 2000 warnings on and off! 😉

It was very much a group effort. Thank you, everyone!

@bsipocz
Copy link
Member

bsipocz commented Nov 18, 2019

I'm sure at the end it was more than 2000. Thanks for your persistence, it wouldn't have happen without you.

bsipocz added a commit that referenced this pull request Nov 18, 2019
Show warnings in pytest and turn them into errors
@astrofrog
Copy link
Member

Thank you @pllim for leading the charge!! 👏 👍

@mhvk
Copy link
Contributor

mhvk commented Nov 18, 2019

Thanks indeed, fantastic!!

eerovaher added a commit to eerovaher/astropy that referenced this pull request Sep 8, 2021
The only thing the file `astropy/coordinates/tests/conftest.py` did was
it defined a pytest fixture for converting a deprecation warning into an
error, but since astropy#7928 such a fixture is no longer needed.
@eerovaher eerovaher mentioned this pull request Sep 8, 2021
9 tasks
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.

7 participants