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

autotest: make utilities, pyscript tests run independently #8097

Merged
merged 19 commits into from
Jul 20, 2023

Conversation

dbaston
Copy link
Member

@dbaston dbaston commented Jul 13, 2023

What does this PR do?

This PR makes tests under the utilities and pyscripts folders run independently, using the following types of changes:

  • Pulling out generated data that is used by multiple tests into module-scoped fixtures.

  • Moving cleanup steps from a dummy test into a fixture, or in some cases writing generated files to a location provided by tmp_path to avoid the need for a cleanup fixture. The tmp_path method is more invasive but probably easier for understand, so it might be worth doing this in all cases.

  • Merging tests, occasionally, such as a test that writes a file followed by a test that reads the file.

  • Flagging tests as non-independent using the random_order(disabled=True) marker (44b24c8).
    These will need to be addressed eventually if we want to run the tests in parallel.

While I was in there, I also split and parameterized some tests (2062341)

What are related issues/pull requests?

#4407

Tasklist

  • Review
  • Adjust for comments
  • All CI builds and checks have passed

@hobu
Copy link
Contributor

hobu commented Jul 13, 2023

💪

@dbaston
Copy link
Member Author

dbaston commented Jul 14, 2023

I'm working though a couple Windows issues here:

  • the gdaltest.runexternal* commands use shlex.split, which erases '\ characters in Windows paths. The \ characters arise when converting pathlib paths to strings on Windows. Removing shlex.split appears to break Windows command strings that contain newlines.
  • some tests require a relative path between datasets but this is not possible on Windows when code is checked out to D:\ and the temporary directory created by tmp_path is under C:\

@dbaston dbaston force-pushed the pytest-independent branch 2 times, most recently from 447f387 to 3445972 Compare July 15, 2023 00:42
@rcoup
Copy link
Member

rcoup commented Jul 15, 2023

writing generated files to a location provided by tmp_path to avoid the need for a cleanup fixture. The tmp_path method is more invasive but probably easier for understand, so it might be worth doing this in all cases.

If we’re using tmp_path more, would it be helpful to document where pytest preserves files/how to find them.

pytest-dev/pytest#8141 ha changed retention behaviour to keep only failed test data by default too (configurable via tmp_path_retention_policy)

@rouault
Copy link
Member

rouault commented Jul 15, 2023

for test_gdal_edit_py_3 failure, changing -a_srs '' to -a_srs None can do the job

@dbaston
Copy link
Member Author

dbaston commented Jul 19, 2023

for test_gdal_edit_py_3 failure, changing -a_srs '' to -a_srs None can do the job

I've made this change, though I worry similar issues may pop up in other places and cause confusing platform-specific test failures. I tried to address the root cause by patching pathlib to use slashes instead of backslashes:

@pytest.fixture(autouse=True)
def use_posix_paths(monkeypatch):

    from pathlib import PureWindowsPath

    oldstr = PureWindowsPath.__str__

    monkeypatch.setattr(PureWindowsPath, "__str__", lambda x : oldstr(x).replace('\\', '/'))

But this causes other failures, such as osgeo_utils tests that require the behavior of pathlib to be the same as os.path.

I guess I'm inclined to leave it as-is now, but maybe a better solution will come.

@coveralls
Copy link
Collaborator

coveralls commented Jul 19, 2023

Coverage Status

coverage: 66.931% (+0.001%) from 66.93% when pulling 2afea0a on dbaston:pytest-independent into 517beea on OSGeo:master.

@rouault
Copy link
Member

rouault commented Jul 19, 2023

Regarding the failures on the build-windows-conda target, cf #8123 (once 8123 passes, please rebase on top of it or master if it is merged)

@rouault rouault merged commit bacb9fd into OSGeo:master Jul 20, 2023
28 of 29 checks passed
@rcoup
Copy link
Member

rcoup commented Jul 20, 2023

I tried to address the root cause by patching pathlib to use slashes instead of backslashes:

Patching stdlib to behave opposite to the platform's behaviour seems like a dangerous approach to solve it...

the gdaltest.runexternal* commands use shlex.split, which erases \ characters in Windows paths. The \ characters arise when converting pathlib paths to strings on Windows.

IIUC on Windows subprocess.* just re-joins a list argument anyway, so you might as well just pass the command string and avoid shlex.split(), which will never work right on Windows.

Removing shlex.split appears to break Windows command strings that contain newlines.

Where do we have newlines in command strings? Should/are they CRLF (\r\n) on windows? Seems like this is probably the problem to focus on?

@dbaston
Copy link
Member Author

dbaston commented Jul 20, 2023

Patching stdlib to behave opposite to the platform's behaviour seems like a dangerous approach to solve it...

Agree - it was an option I shared from my search for the least bad solution. In this case it created more problems than it solved.

IIUC on Windows subprocess.* just re-joins a list argument anyway, so you might as well just pass the command string and avoid shlex.split(), which will never work right on Windows.

That's what I've done here, though it caused a handful of Windows-only failures that were difficult for me to understand and reproduce.

Where do we have newlines in command strings?

The only one that executes on CI was addressed in a32e4ff

@rcoup
Copy link
Member

rcoup commented Jul 20, 2023

ah, I had a search in master and found that code, but I hadn't spotted Even had merged this already - hence the confusion 👍

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.

5 participants