Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test - Refactor gdalhttp.py to remove dependency on test order #4484

Merged
merged 2 commits into from
Sep 14, 2021

Conversation

DFEvans
Copy link
Contributor

@DFEvans DFEvans commented Sep 14, 2021

Refactor gdalhttp.py to remove dependency on test order (#4407 )

Also refactor code to reduce duplication, and ensure all tests have a
timeout. I was finding that test_http_4, known to be flakey, was hanging
for me due to a lack of any timeout.

Also refactor code to reduce duplication, and ensure all tests have a
timeout. I was finding that test_http_4, known to be flakey, was hanging
for me due to a lack of any timeout.

return ret
skip_if_unreachable(url)
pytest.fail()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests 1 and 2 previously ended with return ret, but I've not been able to find any evidence that pytest would parse the result of ret. I've changed them to explicitly fail, in line with the other tests.

@DFEvans
Copy link
Contributor Author

DFEvans commented Sep 14, 2021

Another oddity is Test 5, with the odd flow being:

url = 'https://raw.githubusercontent.com/OSGeo/gdal/release/3.1/autotest/gdrivers/data/s4103.blx'
ds = gdal.Open(url)
filename = ds.GetDescription()
ds = None
with pytest.raises(OSError):
    os.stat(filename)
    pytest.fail('file %s should have been removed' % filename)

My impression is that this is meant to be checking that some local temporary file is deleted. However, ds.Description() returns the original URL, not a local filepath, so the os.stat() will never detect a temporary file that hasn't been deleted. I'd guess that ds.GetDescription() used to return a temporary file path; is there a way to get that file path for this test?

@rouault
Copy link
Member

rouault commented Sep 14, 2021

I'd guess that ds.GetDescription() used to return a temporary file path;

yes, before 850dc88

is there a way to get that file path for this test?

hum, when dowloading http://example.com/some.thing, on Unix a /tmp/smoe.thing file is created (see logic in frmts/wcs/httpdriver.cpp). But only for drivers that don't support the VSI virtual file system API. At the time the test was written, this was the case of the BLX driver, but since then it has been ported to VSI VFS, so the test is no longer relevant. I'd say just drop this test case

@DFEvans DFEvans force-pushed the autotest_http_order_dependency branch from 2027689 to 9f6c1d6 Compare September 14, 2021 13:38
@DFEvans DFEvans force-pushed the autotest_http_order_dependency branch from 9f6c1d6 to 68b5df1 Compare September 14, 2021 13:47
@rouault rouault merged commit acab6d8 into OSGeo:master Sep 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants