Raise warnings as exceptions #317
Conversation
|
OK, so for the most recent commits locally all tests and docs build pass, without remote-data turned on. |
Codecov Report
@@ Coverage Diff @@
## main #317 +/- ##
==========================================
+ Coverage 75.46% 76.68% +1.22%
==========================================
Files 44 44
Lines 5123 5121 -2
==========================================
+ Hits 3866 3927 +61
+ Misses 1257 1194 -63
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
bsipocz
force-pushed
the
tests_raise_warnings
branch
2 times, most recently
from
fe447df
to
bc2e70a
Compare
|
The doctests in the narrative docs will require enabling remote-data testing in CI, I would rather do that separately as a follow-up, but if you prefer, I can add the changes to this one. |
bsipocz
force-pushed
the
tests_raise_warnings
branch
3 times, most recently
from
9ab2c84
to
b134048
Compare
docs/dal/index.rst
Outdated
|
|
||
| >>> url = row.getdataurl() | ||
| >>> fileobj = row.getdataset() | ||
| >>> obj = row.getdataobj() |
There was a problem hiding this comment.
This last one is failing for me. Could point to a bug.
There was a problem hiding this comment.
opened #324 for it, and commented out the line for now.
|
On Mon, May 09, 2022 at 02:57:38PM -0700, Adrian wrote:
I've just merged the mock fix. Does it work without `remote_data` now?
I'm lost the context a bit, and so I'm not even sure this a question
for me, but if the question is: "Would you, Markus, expect the
datalink bysemantics tests to run without network connectivity?",
then I'd make that a clear and resounding Yes (in the sense of: If
that's not true, it's a bug on me).
|
bsipocz
force-pushed
the
tests_raise_warnings
branch
from
eee5022
to
017a862
Compare
conftest.py
Outdated
| # Note that we don't need to change the environment variables back or remove | ||
| # them after testing, because they are only changed for the duration of the | ||
| # Python process, and this configuration only matters if running pytest | ||
| # directly, not from e.g. an IPython session. | ||
|
|
||
| from astropy.utils.iers import conf as iers_conf | ||
|
|
There was a problem hiding this comment.
I'm not familiar with iers and I don't know why this is necessary. Can it be documented if someone asks?
There was a problem hiding this comment.
iers is a huge pile of pain.
This is a workaround to make it avoid trying to download the table when running the tests locally (it would error out with remote data stuff).
So, yes, I can dig up some explanation about this, I'm pretty sure I inherited the workaround from other places.
There was a problem hiding this comment.
Would it be enough to move the comment from the line below to before the import?
There was a problem hiding this comment.
and here is the convo: https://astropy.slack.com/archives/C79U78JKT/p1651188906521579
There was a problem hiding this comment.
I don't think we need a long explanation. Something that will help us remember what is all about and possibly trace it, so we, or other maintainers after us can tell when it's safe to remove or change it.
There was a problem hiding this comment.
This is my last comment. I could merge it afterwards unless you want Tom's feedback as well.
pyvo/dal/tests/test_tap.py
Outdated
| assert len(service.get_job_list(phases=['EXECUTING'], last=3, | ||
| after=datetime.datetime.now())) == 6 | ||
|
|
||
| with pytest.warns(UnknownElementWarning): |
There was a problem hiding this comment.
This should not happen. I'm currently working to find a fix and might remove it in the future.
There was a problem hiding this comment.
See #331. It's a race now... :-)
There was a problem hiding this comment.
thanks for the fix. I'll remove this workaround and rebase this PR once that other one is merged.
There was a problem hiding this comment.
You don't have to do that. If this is ready we can merge it first and I'll deal with this in the other PR. That's why I said it's a race... :-)
There was a problem hiding this comment.
With #332 now merged, could you please remove this exception? Thanks
There was a problem hiding this comment.
yeap, I already did -1min of your comment :) (as I was just chatting with Tom about this)
No, we're raising all exceptions, except the ones explicitly listed in |
… match them instead
bsipocz
force-pushed
the
tests_raise_warnings
branch
from
90e07dc
to
3e6c93e
Compare
This is to make the testing more robust in pyvo itself.
First commit just fixes the heading in setup.cfg which uncovers a lot of issues on its own already, will enable the warning raising once these are all fixed.
to close #307
The fix to the config file also fixes #306 (though most doctests will also need remote-data enabled to run)