Skip to content

CI(pytest): Refactor common pytest arguments to files#5544

Merged
echoix merged 3 commits intoOSGeo:mainfrom
echoix:refactor-pytest-args
Apr 17, 2025
Merged

CI(pytest): Refactor common pytest arguments to files#5544
echoix merged 3 commits intoOSGeo:mainfrom
echoix:refactor-pytest-args

Conversation

@echoix
Copy link
Member

@echoix echoix commented Apr 13, 2025

Since pytest 8.2, pytest allows using a file to send an argument list (https://docs.pytest.org/en/stable/how-to/usage.html#args-from-file), using native argparse functionality (https://docs.python.org/3/library/argparse.html#fromfile-prefix-chars).

Since the different invocations of pytest in CI are repeated a couple times, and quoting rules is different in between shells (like the cmd vs powershell on Windows vs the other platforms), using argument files makes sense.
It allows to factor out duplicated arguments that needs to get updated at each place.

It is possible to use an argument file multiple times, and it is also possible to use an argument file inside an argument file, but I decided against, as for another use case, it was easier to have independent "building blocks" of common arguments when needing to not have one of them, but keep the other parts.

I had to update the pytest version on Windows, as the one provided by OSGeo4W is too old (8.1.1, it's been a little while).

TLDR: Changing pytest arguments in CI can be done once, and applied to all our files using arguments defined in separate files

@github-actions github-actions bot added windows Microsoft Windows specific macOS macOS specific CI Continuous integration labels Apr 13, 2025
@echoix echoix requested a review from wenzeslaus April 14, 2025 01:01
@echoix
Copy link
Member Author

echoix commented Apr 17, 2025

If my PRs from last week could be looked at (and merged) before the weekend, I would be ready (after rebasing), to have a small subset of gunittest-based tests that would be run by pytest.

I was expecting that the small issues fixed in separate, independent PRs, would be easy to review

Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

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

Unusual, but it makes sense. It generally meant for long lists by both argparse and pytest, but pytest highlights that syntax and we do have similar long list issues in GRASS (file parameter), compensating for the unusual nature of this.

@echoix
Copy link
Member Author

echoix commented Apr 17, 2025

My implementation of running working tests, and handling the discovery/exclusion for gunittest tests heavily uses this, and was not reasonable copying into each workflow.

@echoix echoix merged commit 85ea9eb into OSGeo:main Apr 17, 2025
30 checks passed
@github-actions github-actions bot added this to the 8.5.0 milestone Apr 17, 2025
@echoix
Copy link
Member Author

echoix commented Apr 18, 2025

If you could take a look at any of my open PRs, especially in these I'd appreciate it :)

The full picture of what I want to submit after that is: echoix#404, + clean + rebase and a last fix for the difference between local osgeo4w install vs CI, or unselecting that failing test on windows

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI Continuous integration macOS macOS specific windows Microsoft Windows specific

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants