-
-
Notifications
You must be signed in to change notification settings - Fork 313
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
pytest: Allow default test file naming patterns inside tests folders #4784
base: main
Are you sure you want to change the base?
Conversation
Oh, there are two tests that fail, that weren't collected before due to the expected behaviour not being the one implemented. |
I found the reason why the test added in #3582, doesn't work: grass/python/grass/script/tests/test_script_task.py Lines 1 to 11 in 856a514
It works when pytest is run inside a grass shell, where GISBASE, GISRC are set, and the modules, like "r.mapcalc.simple" are in path (the bin and scripts subfolders of the install location are in path). So, I used a fixture that creates a temporary session like used in other pytest tests, but it wasn't enough, as grassTask doesn't take the "env" argument. For this, I used monkeypatch to set the env vars and undo once done: https://docs.pytest.org/en/latest/how-to/monkeypatch.html#monkeypatching-environment-variables That makes it working again (for the first time in CI), as it wasn't run. To ensure that modules in both the script and bin folders are usable, I added two other tests. The d.vect one is taken from one of the few usages of grassTask in the source code, in the gui. |
3ddb862
to
4e18165
Compare
4e18165
to
7f8a8eb
Compare
Only the |
@Shreshth-Malik Take a look at the workaround I used here. Maybe it could be useful for you too if you got functions that don't work with the env from a |
@echoix Thank you for this. I will definitely take a look and try to put it in my tests |
Can someone take a quick look at this? The original intent of the PR was to have pytest run on tests for (new) users following the pytest docs. I have asked someone else to do the change, that I would have approved immediately, but there were some new tests failures that needed to be solved. So I did them myself, so I need to be approved by someone else. I would've approved this PR if it was somebody else that did it. To help out keep the trace of what was done, I document above in the comment the thoughts and the solution. That's what is important. There is one person that is more knowledgeable on this subject precise subject, but we cannot rely on only one person to be always available, effectively having a bus factor of 1 on reviewing testing infrastructure. With the documentation above, I still recommend merging this even if not scrutated as much, so PRs of other new users can go through, and let them work on new tests. It fixes a real overlook of previous PRs that were merged without having their tests actually run, thus not failing. (4 new test files are running now as shown with code coverage, one of which was failing and has been added with tests of what I observed was failing when debugging) |
Co-authored-by: Markus Neteler <[email protected]>
To help out #4755 and also myself, that I worked-around in #4700 by renaming the file (p.s.: still waiting for feedback on the expected behaviour there)
This PR allows to use file names like shown in pytest docs here
https://docs.pytest.org/en/latest/reference/reference.html#confval-python_files, as long as it is inside a tests folder instead of anywhere. The only expected file name pattern currently is not as natural to me, requiring to end with
_test.py
.