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

Unitests: move environment setup from run_unitests.py to test fixtures #14367

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

dcbaker
Copy link
Member

@dcbaker dcbaker commented Mar 14, 2025

We currently have the run_unttests.py removing certain environment variables (CFLAGS and friends) before starting tests, because some tests do not behave correctly when they are set. The right place to do this is an a test fixture, particularly in setUpClass.

To facilitate this, a new base class has been added that adds this setup through a mock, and several test classes that should have this setup have been extended to use it. Then the environment setup is moved into the fixture.

Apart from being more correct, this allows running tests without the runner to be more useful, namely if one wants to invoke pytest or python -m unittest directly.

@dcbaker dcbaker requested a review from jpakkane as a code owner March 14, 2025 18:16
Copy link
Member

@eli-schwartz eli-schwartz left a comment

Choose a reason for hiding this comment

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

Apart from being more correct, this allows running tests without the runner to be more useful, namely if one wants to invoke pytest or python -m unittest directly.

I have some skepticism of this PR, mostly because I do not understand why you think you should be doing this.

dcbaker added 3 commits March 14, 2025 11:42
This will allow all tests, even those that don't need all of the
helpers provided by the BasePlatformTest to still shared some of the
environment setup from other tests.
While the Cargo and TAP tests do not invoke parts of Meson that need
specific environment setups, several other tests do not get those setups
but need them.
…_unitests.py

These are expected state setup for the test cases, not argument handling
for the run_unittests script, as such they should be handled through
test fixtures not through the run unittests script. This makes a bare
run of "pytest" and "./run_unitests.py" with pytest installed basically
equivalent (though without the automatic addition of some arguments
passed by run_unittests.py.
@dcbaker dcbaker force-pushed the submit/unitests-move-env-setup-to-fixtures branch from 010a8b7 to e0e0aa0 Compare March 14, 2025 18:42
@dcbaker
Copy link
Member Author

dcbaker commented Mar 14, 2025

Apart from being more correct, this allows running tests without the runner to be more useful, namely if one wants to invoke pytest or python -m unittest directly.

I have some skepticism of this PR, mostly because I do not understand why you think you should be doing this

Because I want to use standard tools in the standard way rather than through a project specific wrapper script.

run_unitests.py offers me 0 advantages over running pytest directly (except for this case of test environment setup being done in the wrapper instead of in a test fixture where it belongs, so it papers over a bug), and numerous disadvantages, among them:

  • it automatically turns on -n auto, a behavior I don't want. If I invoke without -n I expect to get a serial test run.
  • it mangles the -k option making it more difficult than it already is to write test filters
  • When I press ctrl-c it gives piles of backtraces instead of just cleanly existing, a thing I am prone to do when every test starts failing.

Rather than trying to make this custom wrapper not terrible for me as a developer, I can just accept that it isn't meant for me to use as a developer, it's meant to make running our unittests in CI work well, a job it is doing just fine at.

@eli-schwartz
Copy link
Member

Can you demonstrate for example how to implement --backend without run_unittests.py?

@dcbaker
Copy link
Member Author

dcbaker commented Mar 14, 2025

MESON_UNIT_TEST_BACKEND=xcode pytest

I'm honestly frustrated with the whole discussion, because it feels like we're blocking what is clearly an improvement in the form "use test fixtures to ensure that the environment matches the test expectations" on what feels to me like animosity toward me not wanting to use run_unittests.py

@eli-schwartz
Copy link
Member

I just think it feels overengineered. :/

#13423 attempted the same thing, but by moving "test harness" setup to unittests/__init__.py and performing it a single time at import, which felt a bit more straightforward. Creating a setupClass that potentially runs setup and teardown on every single test, just for something that is supposed to run at program startup, feels like we're learning the wrong lesson here.

@dcbaker
Copy link
Member Author

dcbaker commented Mar 14, 2025

Well, I really wanted to move to pytest because pytest has global fixtures, not just test and class fixtures.

These are class fixtures, so they're only run once per class and not per test.

@eli-schwartz
Copy link
Member

These are class fixtures, so they're only run once per class and not per test.

They may be run once per test, if tests from different classes are interleaved together. There are some plugins that do this automatically whenever they are installed, so you might not even realize that's what happened.

@jpakkane
Copy link
Member

Well, I really wanted to move to pytest because pytest has global fixtures, not just test and class fixtures.

We can't really "move" to pytest because it is not in the stdlib. Plain unittest will still have to work so any functionality that is not in it has to be hand rolled anyway.

@bonzini
Copy link
Collaborator

bonzini commented Mar 16, 2025

Even if meson cannot require more than the stdlib to run, that would not exclude relying on pytest or other external Python-only wheels for the purpose of running tests, would it?

@eli-schwartz
Copy link
Member

I've found it useful in the past to be able to run the self-tests on systems without pytest installed. In particular, running against additional python interpreter versions, where pytest was only packaged / installed for a single (default) python.

Requiring pytest would make it a hassle for me to do cpython alpha testing.

@dcbaker
Copy link
Member Author

dcbaker commented Mar 17, 2025

I understand the reasons why we're not going to, but it's pretty frustrating because unittest is... not great. As we see here.

@dcbaker
Copy link
Member Author

dcbaker commented Mar 17, 2025

Stack overflow has some suggestions on global setup/teardown for unittest, I'll have to try those in a bit.

@eli-schwartz
Copy link
Member

We really don't see that here. :) A pytest session fixture is often overly complicated as opposed to just running code at startup. The point of fixtures is to tag them for use on specific tests (by passing them as magic test function arguments), which allows you to do things like have a fixture that runs only once per pytest run, but only right before the first function that uses that fixture, and avoid running the fixture if you don't run any tests that use the fixture (only tests that don't use it).

It's trivial to remap the current environment setup into an __init__.py, @lazka already submitted a PR to do so but never responded to my review comment there.

@dcbaker
Copy link
Member Author

dcbaker commented Mar 17, 2025

What I mean is, there's a mechanism unittest provides for a global setup/teardown you put in your __init__.py, but is guaranteed to work the way you expect.

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

Successfully merging this pull request may close these issues.

4 participants