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

Move tests to pytest #588

Merged
merged 1 commit into from
Nov 11, 2023
Merged

Move tests to pytest #588

merged 1 commit into from
Nov 11, 2023

Conversation

nitzmahone
Copy link
Member

  • custom pytest collector and item to surface pyyaml data-driven tests as individual pytest unit tests
  • moved some true unit tests to pytest-native tests
  • deprecated setup.py test custom command

@kloczek
Copy link

kloczek commented Jun 16, 2022

${PYTHON} -m pytest pytest should not be used that way because if it is executed python -m <foo> python automatically adds current disrectory to sys.path. This is why it is provided pytest executable stript which should be used instead.

@nitzmahone nitzmahone force-pushed the tests_to_pytest branch 2 times, most recently from ff8b400 to de789c3 Compare November 10, 2023 18:09
@nitzmahone
Copy link
Member Author

@kloczek thanks- the Python PATH mess on Windows to get it to find the right version of the script is a lot harder to untangle (which is why it was using module-style invocation in CI in the first place) that on Linux/MacOS... I did change the Windows CI invocation to use -I though, which will bring its behavior back in line with the script-style invocations (by not adding CWD to sys.path).

@nitzmahone nitzmahone force-pushed the tests_to_pytest branch 2 times, most recently from d091b81 to ed36ccf Compare November 10, 2023 19:15
* custom pytest collector and item to surface pyyaml data-driven tests as individual pytest unit tests
* moved some true unit tests to pytest-native tests
* deprecated `setup.py test` custom command
* updated Makefile to use pytest
* align test matrix with planned 7.x Python support
@ingydotnet ingydotnet self-requested a review November 11, 2023 00:59
@ingydotnet ingydotnet merged commit a98fd60 into yaml:main Nov 11, 2023
13 checks passed
@perlpunk
Copy link
Member

perlpunk commented Nov 11, 2023

When I run the tests with one of the following commands:

pytest
python3 -m pytest

I get the following error:

=========================================================== ERRORS ===========================================================
______________________________________ ERROR collecting tests/legacy_tests/test_yaml.py ______________________________________                                                                                                                              
ImportError while importing test module '/home/tina/develop/github/pyyaml/tests/legacy_tests/test_yaml.py'.                   
Hint: make sure your test modules/packages have valid Python names.              
Traceback:                                                                                                                    
/usr/lib64/python3.11/importlib/__init__.py:126: in import_module                                                                                                                                                                                           
    return _bootstrap._gcd_import(name[level:], package, level)                
tests/legacy_tests/test_yaml.py:2: in <module>                                                                                                                                                                                                              
    from test_reader import *                                                                                                                                                                                                                               
tests/legacy_tests/test_reader.py:2: in <module>                                                                              
    import yaml.reader                                                                                                        
E   ModuleNotFoundError: No module named 'yaml.reader'         

Or if I install pyyaml as a system package, it tests that code instead.

I'm on a fresh openSUSE Tumbeweed container.

What helps:

PYTHONPATH=$PWD/lib python3 -m pytest

or adding a pytest.ini:

[pytest]
pythonpath = lib

I didn't realize that yesterday because I was testing in a container that had pyyaml installed, so the tests just passed and I didn't realize it wasn't testing the code from git.
This needs to be fixed, we can't rely on people having to add lib to the path manually.

@perlpunk
Copy link
Member

perlpunk commented Nov 11, 2023

Also, .github/workflows/manual_artifact_build.yaml has a syntax error in line 355:

  windows_pyyaml:
    needs: windows_libyaml
    name: pyyaml ${{ matrix.platform }} ${{matrix.python_arch}} ${{matrix.spec}}
    runs-on: ${{matrix.platform}}
    strategy:
      matrix:
        include:
          spec: 3.8    # <-----  this line shouldn't be there. The value for `include` should be a sequence
        - platform: windows-2019
          build_arch: x64
          python_arch: x64
          spec: 3.9

See #763

@kloczek
Copy link

kloczek commented Nov 11, 2023

python3 -m pytest

Please do not use pytest that way.
It was MANY times explained by pytest maintainer to use ONLY pytest shell wrapper script because when python interpreter is executed by python -m foo it always adds current directory path to sys.paths and this trashes test suite execution in many cases (look on -m option description in python man page)

@perlpunk
Copy link
Member

perlpunk commented Nov 11, 2023

@kloczek
If you READ my comment, you will see that I wrote:

When I run the tests with one of the following commands:

pytest
python3 -m pytest

So you can see I tried pytest first.
So maybe you can help explain what I am doing wrong with just running pytest instead of telling me what NOT to do?

@kloczek
Copy link

kloczek commented Nov 11, 2023

I know what you've done.
I'm not telling you anything.
I've only added note why python -m pytest should not be used.
The same is about other modules which have wrapper scripts.

@webknjaz
Copy link

help explain what I am doing wrong with just running pytest

@perlpunk there's no fundamental problem with running just pytest. But when you run python -m, the interpreter itself pre-injects your CWD to $PYTHONPATH, which typically causes the test modules to import the tested package from a Git checkout. The change in this PR adds the -I CLI flag to counteract that effect.
Though, when a project uses a so-called “src-layout” (which PyYAML does, through the lib/ subdir), said side effect won't have noticeable influence on how the tests are executed.

So, there are 2 mechanisms implemented to prevent testing a non-installed Git checkout directly. It is recommended to always test “as installed”. This means, making sure that the project modules aren't imported from a Git checkout, because that's not what the end-users would get. Sometimes, projects forget to include some files into PyPI-published package distributions, causing the CI to be green while the pip installed copies might be unusable. So in general, it's recommended to pip install what you test and run the tests against that. It could be pip install -e ., which is sometimes useful locally, for development, but I would avoid in CI.

Using tools like tox or nox helps manage your virtualenv and is a good way to make sure that the tests are executed the same way locally and in CI, without having things leak from the outer envs like system site-packages.

tox can also produce a “detached” dev env for you to do your experiments, from one of its managed toxenvs.

If, for some reason, you don't want to use tox, it is imperative that you make use of virtualenvs manually. For example, by using python -m venv .venv. Then, install the project and the test deps (like pytest) into that virtualenv, with something like .venv/bin/python -m pip install . pytest (you could do -e . instead of ., if need be, for editable/develop mode). After that, running pytest from that env will be able to pick up pyyaml from .venv/bin/**/site-packages/* (which will have a copy of an installed package or a link to the Git source in case of editable mode). You can do this using either .venv/bin/pytest or .venv/bin/python -m pytest. Or, you can activate it via . .venv/bin/activate once per shell session and drop that relative path prefix .venv/bin/ from the commands.

But again, I encourage you to use tox, since it's a well-known tool that is familiar to many contributors, and it makes sure that everybody runs the same stuff locally and in CI, reducing the burden of troubleshooting weird corner case.

Hope that helps!

@perlpunk
Copy link
Member

My question is still the same: Should it be possible to just run pytest (plus any flags) to get successful tests? If yes, what am I doing wrong?
I know tox and venv, but so far I thought it would always be possible in a project to also run pytest directly, if you have every dependency installed.

@nitzmahone
Copy link
Member Author

Should it be possible to just run pytest (plus any flags) to get successful tests?

Sort of; ultimately pytest has to be able to import the code-under-test, and you don't usually want to bake assumptions about how it should do that into the tests themselves- I'm guessing that's the part you're having trouble with. With pure-Python code that's directly runnable from the source, it's usually sufficient to just have the dir containing the project's packages on PYTHONPATH (for current PyYAML, that's basically PYTHONPATH=$your_pyyaml_git_checkout_path/lib/) , but it gets gnarlier when there are non-Python extensions or other magic required to make the source function as it does when truly "installed". You can just pip install . from the project root to do that, but that's not very friendly to iterative development/testing, since it's a full installation that copies all the code into your Python env's site-packages dir, so anytime you want to change the code, you have to reinstall it before anything else can see it. That's what pip install -e . is about: it's a well-defined mechanism for doing "editable installs" that still runs extension builds, and makes it look like the package has been installed, but uses $magic_unimportant_right_now to tell Python how to load the code directly from your source. So at that point, the only time you need to re-run that is if you've changed code that affects the built extension.

The other consideration with all this stuff is that it should usually be done in some kind of isolated Python environment (eg not /usr/bin/python3). pip install can trash and/or create really gnarly conflicts with Python packages that were installed by OS package managers; they've been adding warnings over the years when it detects that, and there's actually a new standard (PEP668) that allows OS-managed Python environments to completely block pip et al from installing/munging their Python packages. With PyYAML being a very common OS-packaged Python library, this becomes super important. Long story short: most PyYAML dev/test activities should occur in a dedicated Python venv- as more OSs start implementing PEP668 marking of their OS-managed Python envs, there won't be any choice.

This is also one of many reasons why the root Makefile stuff is a big problem- it's effectively impossible to make generic assumptions about a random dev's Python environment where this kind of generic workflow can actually do anything useful. At best, it just doesn't work (or it doesn't test what you think it does), and at worst, it destroys the environment. Which leads me to:

I've only added note why python -m pytest should not be used

Directly invoking the tools as modules from a specific interpreter path ensures that everything is always happening in the same environment; using script wrappers for tools from PATH exposes you to all sorts of different kinds of breakage that's arguably much worse than what running the tool as a module might; this problem is especially common on MacOS + homebrew + Xcode and Windows, but it's technically a problem everywhere. python -I -m pytest is much easier to make work reliably across arbitrary environments than anything involving script wrappers and ensure that you're running the tools and code you think you are, and the -I isn't strictly necessary with PyYAML's current source layout either (though it's a decent practice anyway for interactive dev, at least until -P is usable everywhere with 3.11+).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

5 participants