test: review skipped tests (networking timeouts) #1027
Conversation
tests/test_0001_source_class.py
Outdated
| @pytest.mark.network | ||
| @pytest.mark.xrootd | ||
| @pytest.mark.parametrize("use_threads", [True, False], indirect=True) | ||
| def test_xrootd_deadlock(use_threads): | ||
| def test_xrootd_deadlock(): |
There was a problem hiding this comment.
Not really sure of what I did here, don't really understand this test. Please review @nsmith- in case I broke something important.
There was a problem hiding this comment.
I think this test is obsolete now that XRootD upstream has truly fixed the issues discussed in #59 and our CI is not testing older xrootd that needs the patch of
Lines 105 to 109 in 6b6fa94
In principle we could in https://github.com/scikit-hep/uproot5/blob/6b6fa9458b4ae46894e053dee25c56f678567f9d/.github/workflows/build-test.yml#L62C11-L62C11 specify older xrootd to test but it seems a bit moot.
There was a problem hiding this comment.
Perhaps @chrisburr has input
lobis
changed the title
There was a problem hiding this comment.
That's awesome! I've been meaning to get back to tests that were excluded for "flakiness" (also in Awkward Array; some tests were turned off and labeled with "FIXMEs").
I've checked all of the fixes, and it all looks good to me. I'm going to leave this to @nsmith- because you have a question for him inline.
tests/test_0001_source_class.py
Outdated
| @pytest.mark.network | ||
| @pytest.mark.xrootd | ||
| @pytest.mark.parametrize("use_threads", [True, False], indirect=True) | ||
| def test_xrootd_deadlock(use_threads): | ||
| def test_xrootd_deadlock(): |
There was a problem hiding this comment.
I think this test is obsolete now that XRootD upstream has truly fixed the issues discussed in #59 and our CI is not testing older xrootd that needs the patch of
Lines 105 to 109 in 6b6fa94
In principle we could in https://github.com/scikit-hep/uproot5/blob/6b6fa9458b4ae46894e053dee25c56f678567f9d/.github/workflows/build-test.yml#L62C11-L62C11 specify older xrootd to test but it seems a bit moot.
* return a http.client.HTTPException instead of OSError * rerun tests on http.client.HTTPException * remove test skip for xrootd * rename test * rename test * do not capitalize variables * correctly load default options * timeout error * update test (TODO: review) * add TimeoutError to retry exceptions * correctly initialized num_bytes * correctly access resource * rerun on timeout * update retry regex * remove outdated test
In this PR I review the skipped tests due to "flakiness". They are all xrootd tests.
After removing the skip some of them failed for various reasons, I made changes so tests pass. It's not the cleanest fix but this xrootd source is going to be replaced by fsspec (atleast for the default behaviour).
Ocasionaly some test will fail due to server issues, we should retry those. I think raising a specific error when this happens and configuring pytest to retry on this error is the way to go (this is what we done at the moment, but it doesn't work since we don't see
requestserrors). I updated thehttpsource to throw ahttp.client.HTTPExceptionand xrootd to throw aTimeoutError. This currently works for the ocasional xrootd and s3 fails.Probably these errors can be improved (not sure if it's correct to throw a
TimeoutError). Before this changes we were throwing anOSErrorwhich imho is too generic to do retries on.