TST: fix warnings and stop ignoring them #2520

bsipocz · 2022-09-11T08:18:24Z

Some of these warnings were masking over real issues, e.g. missing test dependencies or ignored pytest markers. Not everything is fixed yet, but locally I'm down to <20, and now pushing a draft PR to check whether anything comes back up when running the tests in different setup scenarios.

This works towards #2057 fixes #2053

pep8speaks · 2022-09-12T00:01:51Z

Hello @bsipocz! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-09-12 07:14:20 UTC

codecov · 2022-09-12T00:16:32Z

Codecov Report

Merging #2520 (c19f188) into main (59e6f84) will increase coverage by 0.02%.
The diff coverage is 96.42%.

@@            Coverage Diff             @@
##             main    #2520      +/-   ##
==========================================
+ Coverage   63.61%   63.63%   +0.02%     
==========================================
  Files         132      132              
  Lines       17074    17092      +18     
==========================================
+ Hits        10861    10876      +15     
- Misses       6213     6216       +3

Impacted Files	Coverage Δ
astroquery/sdss/core.py	`91.50% <88.88%> (-0.11%)`	⬇️
...oplanet_orbit_database/exoplanet_orbit_database.py	`93.61% <100.00%> (+0.59%)`	⬆️
astroquery/ipac/irsa/ibe/core.py	`78.12% <100.00%> (ø)`
...roquery/ipac/nexsci/nasa_exoplanet_archive/core.py	`57.55% <100.00%> (+0.70%)`	⬆️
astroquery/mpc/core.py	`88.39% <100.00%> (+0.06%)`	⬆️
astroquery/vamdc/core.py	`45.61% <100.00%> (ø)`
astroquery/mast/discovery_portal.py	`66.51% <0.00%> (-0.92%)`	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

astroquery/cadc/tests/test_cadctap.py

bsipocz · 2022-09-28T06:52:02Z

@pllim and/or @ceb8 - would you mind to give this a quick review? As you see from the cross-links there have been quite a few things discovered, I fixed some of them already, worked around a few, and currently produced more remote-tests failures than before this PR.
That being said, I would like to get this in nowish rather than dragging on forever and growing it to a size that's unmanageable. Besides 1) the ignores are way more specific now, which results in a bit more robust test setup 2) the additional remote failures are still note affecting the users more than the current main, and we can aim to fix them gradually.

astroquery/alfalfa/tests/test_alfalfa.py

pllim

Great clean-up! Just minor comments from drive-by review. Feel free to ignore if they are no applicable.

pllim · 2022-09-28T12:48:06Z

astroquery/besancon/tests/test_besancon.py

        else:
            file_obj = filename
-        yield file_obj
+            yield file_obj


Can we just yield filename here and not bother with the extra file_obj = filename? 🤔

pllim · 2022-09-28T12:51:33Z

astroquery/cadc/tests/test_cadctap_remote.py

+                       output_file=output_file)#'my_observations.xml')
+#        assert output_file.exists()


Remove commented code. Otherwise, please leave a comment on why they are commented.

Suggested change

output_file=output_file)#'my_observations.xml')

# assert output_file.exists()

output_file=output_file)

pllim · 2022-09-28T12:52:13Z

astroquery/cadc/tests/test_cadctap_remote.py

+        results = cadc.exec_sync(
+            "SELECT o.observationID, intent FROM caom2.Observation o JOIN "
+            "tap_upload.test_upload tu ON o.observationID=tu.observationID",
+            uploads={'test_upload': output_file})#'my_observations.xml'})


Remove commented code. Otherwise, please leave a comment on why they are commented. (Won't let me insert suggestion here, not sure why.)

Thanks for spotting, these are debug remains indeed

pllim · 2022-09-28T12:54:00Z

astroquery/cadc/tests/test_cadctap.py

@@ -406,7 +407,7 @@ def test_get_images():
       Mock(side_effect=lambda x, y, z: ['https://some.url']))
 def test_get_images_async():
    with patch('astroquery.utils.commons.get_readable_fileobj', autospec=True) as readable_fobj_mock:
-        readable_fobj_mock.return_value = open(data_path('query_images.fits'), 'rb')
+        readable_fobj_mock.return_value = Path(data_path('query_images.fits'))


I am not familiar with the mock stuff here nor know Path very well. Is Path object interchangeable as an open file handler?

Apparently it is. And this mocking really bothers me, this change was necessary as a test in the remote file was failing without this, so there is some fundamental problems with the approach. I might have opened an issue for it and we'll try to remember to check when I get in the office.

hmm, not exactly interchangeable as it turns out here: #2541, but I think it's close enough to be OK. Something is fundamentally wrong with these in-place patches for cadc, but I won't have time to investigate.

Worth a bug ticket?

There have been a few genuine bugs smoked out for the module, and I'm not sure it's worth adding the noise, this only affects the testing and the only effect I saw is the appearance of unclosed file warnings in the skipped remote tests.
I would expect that if there is a real issue with the code, we would be able to catch it with e.g. switching to use ptytest-randomly.

(in short: the only effect I saw is the open file warnings, which is annoying but I doubt would actually affecting users)

pllim · 2022-09-28T12:55:49Z

astroquery/cadc/tests/test_cadctap_remote.py

@@ -293,3 +293,20 @@ def test_list_jobs(self):
        if len(jobs) > 5:
            jobs_subset = cadc.list_async_jobs(last=5)
            assert len(jobs_subset) == 5
+
+    @pytest.mark.xfail(reason='https://github.com/astropy/astroquery/issues/2538')


Why not use @pytest.mark.filterwarnings here like the other test?

Eventually, I want these to be cleaned up, and we won't notice if it's a filterwarning.

pllim · 2022-09-28T13:00:46Z

astroquery/ipac/ned/tests/test_ned.py

+        with open(data_path(DATA_FILES['image']), 'rb') as infile:
+            yield infile


I guess it is okay to have a context manager inside a context manager, though such is not used in the official example usage at https://docs.python.org/3/library/contextlib.html#contextlib.contextmanager . 🤷

Suggested change

with open(data_path(DATA_FILES['image']), 'rb') as infile:

yield infile

infile = open(data_path(DATA_FILES['image']), 'rb')

try:

yield infile

finally:

infile.close()

I recall I've seen it somewhere, maybe in the pytest or mock docs or a very similar SO problem. I would rather keep it than switch to try/finally and manual close.

pllim · 2022-09-28T13:06:18Z

astroquery/sdss/core.py

+                warnings.filterwarnings("ignore", category=AstropyWarning,
+                                        message=r'OverflowError converting to IntType in column.*')
+            arr = Table.read(response.text, format='ascii.csv', comment="#")
+            for id_column in ['objid', 'specobjid']:


Nitpicky nitpick.

Suggested change

for id_column in ['objid', 'specobjid']:

for id_column in ('objid', 'specobjid'):

ceb8

LGTM

…me remote cases

bsipocz · 2022-10-03T18:50:52Z

Hmm, so this smoked out a lot of issues on windows, we have 127 remote-test failures there. Some are trivial, but I expect to find a few actual bugs, too.

bsipocz · 2022-10-04T04:36:19Z

Log is here, in case anyone cares to dive in: https://github.com/astropy/astroquery/actions/runs/3171737942/jobs/5165493118#step:5:26037

pllim · 2022-10-04T12:27:50Z

I think it is from astropy-helpers .

bsipocz · 2022-10-04T16:28:17Z

I didn't mean to post a line link, but all the 127 failures, most of which are due to warnings. Updated the link about to point to a better line.

pllim · 2022-10-04T16:35:58Z

So some definitely are bugs because the code failed to account for Windows path using \\ instead of /.

Things like ResourceWarning can probably be ignored for now.

... And this one is really weird:

requests.exceptions.HTTPError: 500 Server Error: Internal Server Error for url:
http://hst.esac.esa.int/ehst-sl-server/servlet/data-action?OBSERVATION_ID=J6FL25S4Q&USERNAME=ehst-astroquery&CALIBRATION_LEVEL=RAW
D:\a\astroquery\astroquery\docs\esa\hubble\hubble.rst:41: UnexpectedException

When I copy-pasted http://hst.esac.esa.int/ehst-sl-server/servlet/data-action?OBSERVATION_ID=J6FL25S4Q&USERNAME=ehst-astroquery&CALIBRATION_LEVEL=RAW on my Chrome browser on Windows, a tab popped up but then immediately closed. Might be a server-side problem.

pllim · 2022-10-04T16:37:49Z

And there is also this:

TimeoutError: [WinError 10060] A connection attempt failed because the connected party
did not properly respond after a period of time, or established connection failed because
connected host has failed to respond

Are some servers purposely blocking requests from Windows? Cert issue?

pllim · 2022-10-04T16:40:05Z

This one might be worth investigating in case it is io.ascii bug on Windows:

>               result_s = eso.query_surveys(survey, coord1=266.41681662,
                                             coord2=-29.00782497,
                                             box='01 00 00',
                                             cache=False)
...
>                   warnings.warn(
                        "OverflowError converting to {} in column {}, reverting to String."
                        .format(converter_type.__name__, col.name), AstropyWarning)
E                   astropy.utils.exceptions.AstropyWarning: OverflowError converting to IntType in column Object, reverting to String.

...\astropy\io\ascii\core.py:1098: AstropyWarning

bsipocz · 2022-10-04T16:40:11Z

A lot of the ESA 500 got fixed upstream yesterday, so those are red herrings.

But the sdss ones e.g. real, and is due to the usual large numbers get casted to str on 32bit. Some of it got worked around to manually convert bach, but apparently I missed a few.

pllim · 2022-10-04T16:42:03Z

Some of the errors might be due to Windows endline vs *NIX endline (since I don't see any path problem in the traceback) but I am not 100% sure.

bsipocz · 2022-10-04T17:42:38Z

Some of the errors might be due to Windows endline vs *NIX endline

Oh, that's interesting. If that's the case that's an upstream bug for WHITESPACE_NORMALIZE?

pllim · 2022-10-04T17:54:42Z

Re: #2520 (comment)

Not sure. Will need someone to investigate. I was thinking more like maybe the query is malformed in the backend... 💭

bsipocz added testing infrastructure no-changelog-entry-needed labels Sep 11, 2022

bsipocz added this to the v0.4.7 milestone Sep 11, 2022

bsipocz force-pushed the TST_fix_warnings branch from a87296b to 4bbe3c2 Compare September 11, 2022 08:21

bsipocz mentioned this pull request Sep 11, 2022

Fix or catch all warnings raised by the tests #2057

Closed

11 tasks

bsipocz force-pushed the TST_fix_warnings branch from 4bbe3c2 to 6eb4d9d Compare September 12, 2022 00:01

bsipocz force-pushed the TST_fix_warnings branch from 6eb4d9d to 2849403 Compare September 12, 2022 00:12

bsipocz force-pushed the TST_fix_warnings branch from 2849403 to 05e9f4c Compare September 12, 2022 02:01

bsipocz marked this pull request as ready for review September 12, 2022 02:01

bsipocz force-pushed the TST_fix_warnings branch 2 times, most recently from e8cb103 to d42bdb7 Compare September 12, 2022 07:14

eerovaher reviewed Sep 12, 2022

View reviewed changes

astroquery/cadc/tests/test_cadctap.py Outdated Show resolved Hide resolved

bsipocz force-pushed the TST_fix_warnings branch from d42bdb7 to 15a9cf5 Compare September 16, 2022 23:10

bsipocz force-pushed the TST_fix_warnings branch 2 times, most recently from 814c433 to b8ae02d Compare September 27, 2022 21:01

This was referenced Sep 28, 2022

BUG: cadc uploads doesn't close file #2538

Open

BUG: eso.query_instrument help leaves unclosed sockets #2539

Open

BUG: cadc cuts down filepath when non string output_file is passed on #2540

Closed

bsipocz force-pushed the TST_fix_warnings branch 3 times, most recently from 29e18aa to 31ff9a4 Compare September 28, 2022 05:53

bsipocz mentioned this pull request Sep 28, 2022

BUG: multiple timeout kwargs for query.BaseQuery._download_file #2542

Closed

bsipocz force-pushed the TST_fix_warnings branch 3 times, most recently from 71f9df4 to ee3ef4d Compare September 28, 2022 06:33

eerovaher reviewed Sep 28, 2022

View reviewed changes

astroquery/alfalfa/tests/test_alfalfa.py Show resolved Hide resolved

pllim approved these changes Sep 28, 2022

View reviewed changes

ceb8 approved these changes Sep 29, 2022

View reviewed changes

bsipocz added 5 commits October 1, 2022 16:56

Adding test dependency and registering pytest marker

b0b1a8a

Fixing more warnings and removing them from the config

50ea662

MAINT: fixing warning handling for remaining non-remote tests, and so…

3cba46f

…me remote cases

BUG: ignore column int overflow warning for windows

2823283

MAINT: addressing minor review comments

c19f188

bsipocz force-pushed the TST_fix_warnings branch from ee3ef4d to c19f188 Compare October 1, 2022 23:56

bsipocz merged commit 4b4af55 into astropy:main Oct 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TST: fix warnings and stop ignoring them #2520

TST: fix warnings and stop ignoring them #2520

bsipocz commented Sep 11, 2022 •

edited

Loading

pep8speaks commented Sep 12, 2022 •

edited

Loading

codecov bot commented Sep 12, 2022 •

edited

Loading

bsipocz commented Sep 28, 2022

pllim left a comment

pllim Sep 28, 2022

pllim Sep 28, 2022

pllim Sep 28, 2022

bsipocz Sep 28, 2022

pllim Sep 28, 2022

bsipocz Sep 28, 2022

bsipocz Sep 29, 2022

ceb8 Sep 29, 2022

bsipocz Sep 29, 2022

bsipocz Sep 29, 2022

pllim Sep 28, 2022

bsipocz Sep 28, 2022

pllim Sep 28, 2022

bsipocz Sep 28, 2022

pllim Sep 28, 2022

ceb8 left a comment

bsipocz commented Oct 3, 2022

bsipocz commented Oct 4, 2022 •

edited

Loading

pllim commented Oct 4, 2022

bsipocz commented Oct 4, 2022

pllim commented Oct 4, 2022

pllim commented Oct 4, 2022

pllim commented Oct 4, 2022

bsipocz commented Oct 4, 2022

pllim commented Oct 4, 2022

bsipocz commented Oct 4, 2022

pllim commented Oct 4, 2022

		output_file=output_file)#'my_observations.xml')
		# assert output_file.exists()

	output_file=output_file)#'my_observations.xml')
	# assert output_file.exists()
	output_file=output_file)

		with open(data_path(DATA_FILES['image']), 'rb') as infile:
		yield infile

	for id_column in ['objid', 'specobjid']:
	for id_column in ('objid', 'specobjid'):

TST: fix warnings and stop ignoring them #2520

TST: fix warnings and stop ignoring them #2520

Conversation

bsipocz commented Sep 11, 2022 • edited Loading

pep8speaks commented Sep 12, 2022 • edited Loading

Comment last updated at 2022-09-12 07:14:20 UTC

codecov bot commented Sep 12, 2022 • edited Loading

Codecov Report

bsipocz commented Sep 28, 2022

pllim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ceb8 left a comment

Choose a reason for hiding this comment

bsipocz commented Oct 3, 2022

bsipocz commented Oct 4, 2022 • edited Loading

pllim commented Oct 4, 2022

bsipocz commented Oct 4, 2022

pllim commented Oct 4, 2022

pllim commented Oct 4, 2022

pllim commented Oct 4, 2022

bsipocz commented Oct 4, 2022

pllim commented Oct 4, 2022

bsipocz commented Oct 4, 2022

pllim commented Oct 4, 2022

bsipocz commented Sep 11, 2022 •

edited

Loading

pep8speaks commented Sep 12, 2022 •

edited

Loading

codecov bot commented Sep 12, 2022 •

edited

Loading

bsipocz commented Oct 4, 2022 •

edited

Loading