support running and debugging pytest for local tree#859
support running and debugging pytest for local tree#859pdgendt merged 3 commits intozephyrproject-rtos:mainfrom
pytest for local tree#859Conversation
cb8f5da to
95b5937
Compare
|
I'm not sure I like where this is going. Testing $ python -m venv .venv
$ source .venv/bin/activate
$ pip install -e . |
95b5937 to
131b1c3
Compare
|
Note: A few other tests invoke the hardcoded For consistency and robustness, these tests should be refactored so that pytest passes even when |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #859 +/- ##
==========================================
- Coverage 84.41% 84.40% -0.02%
==========================================
Files 11 11
Lines 3382 3385 +3
==========================================
+ Hits 2855 2857 +2
- Misses 527 528 +1
|
pdgendt
left a comment
There was a problem hiding this comment.
I recommend you check https://docs.pytest.org/en/7.1.x/explanation/goodpractices.html#tests-outside-application-code too.
Installation in editable mode does not work for me: Am I doing something wrong?
In this documentation it says: So this is exactly what I want to do. I want to run In many other Python open-source projects, it’s possible to run
If the installed west version matches, then the tests should test against this installed package. The main problem seems to lay in current tests that call Running
|
So what I do to test (requires an environment!): $ python3.10 -m venv .venv
$ source .venv/bin/activate
$ pip install -e .
I get that, but I also want to make sure developers are running the version they're expecting to run. Maybe we should check
Right, but I'd like to avoid having to mess with the system path if possible. There should be some recommended practices for this, no? There are so many packages out there, we can't be the only with this problem. I guess that's why the myriad of task runners exist. |
Wouldn't it make sense to get rid of this subprocess completely and instead use
With this, pytest can completely ensure which modules are tested, because there are no other processes that have different PYTHONPATH. The benefit of this would also be that the tests become fully debuggable. |
Yes please, but I guess that this is even more work than the current proposal?
Getting rid of the |
|
I will give it a try 👍 |
Just FYI that's technically an abstraction violation; nothing in |
But that doesn't mean we can't rely on it for testing, right? From a practical point of view it would solve some of the issues we have with testing/coverage. |
|
No, it's just white-box testing |
|
I use |
16981ae to
fe1ec68
Compare
4d49829 to
9d9462c
Compare
Shorter commands are always nicer but it's IMHO not enough to justify a PR that big. Is there some bigger problem that this PR solves for you? I remember I used to struggle to use |
I scanned the comments and found a couple problem descriptions that should have been in commit messages. And then some more:
Prototyping and experimenting is great, but once a PR leaves the draft status it should be clear 1. what is the problem solved 2. why and how that solution was chosen. For earlier stage discussions please use drafts, issues, discord,... Some good inspiration: |
13a8e62 to
abba370
Compare
c886314 to
9745f8b
Compare
@thorsten-klein see #862 |
pytest for local treepytest for local tree
313f0ef to
3d819af
Compare
There was a problem hiding this comment.
Pull Request Overview
Enable running and debugging pytest directly against the local source tree without relying on subprocess-based invocations, improving IDE integration and consistency of environment handling.
- Replace subprocess-based test helpers with in-process CLI invocation (west.app.main), adding a subprocess variant only where needed
- Update tests to expect SystemExit, capture stderr explicitly, and add coverage for module execution and forall env vars
- Adjust main entrypoint to prepend src to sys.path when run as a script/module; update coverage paths and README instructions
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_project.py | Replace CalledProcessError with SystemExit, switch to cmd_subprocess where necessary, add test_forall_env_vars, and separate stderr assertions. |
| tests/test_main.py | Add tests for version output via in-process and subprocess calls; add module-run test verifying sys.path injection and exit code. |
| tests/test_help.py | Simplify help output checks by removing platform newline normalization (now captured consistently). |
| tests/test_config.py | Migrate exception expectations from CalledProcessError to SystemExit and update cmd_raises usage. |
| tests/test_alias.py | Update exception expectations, but assertions inspect exit code instead of error output. |
| tests/conftest.py | Introduce _cmd, cmd, cmd_raises, cmd_subprocess helpers; capture stdout/stderr; run west.app.main directly; optional subprocess mode. |
| src/west/app/main.py | Prepend src to sys.path when executed as main to ensure local modules are used. |
| pyproject.toml | Fix coverage omit globs to match paths nested under any directory. |
| README.rst | Document how to run pytest against installed package vs local source using pythonpath=src. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
3d819af to
e24ae8e
Compare
|
Gentle ping :-) I have adopted the PR description to explain the "Why". Is there anything else you’d like me to change on this branch? |
|
Needs a rebase |
e24ae8e to
c2fcc62
Compare
Done 👍🏻 |
carlescufi
left a comment
There was a problem hiding this comment.
This looks reasonable to me altogether. I don't love the addition to main.py, but I have read the comments and it seems the better way of handling this.
There was a problem hiding this comment.
I have no problem with the general idea but I have a few requests, see below.
Also, as the original author, I think we really need @mbolivar to approve the general idea. Not to review every line but at least approve the conceptual changes.
Finally, I didn't have time to check every code change yet, sorry.
tests/conftest.py
Outdated
| # subprocess output invoked by `main` are captured together in a single | ||
| # string and also, that program-level setup and teardown in `main` happen | ||
| # fresh for each call. | ||
| cmd = cmd.split() if isinstance(cmd, str) else cmd |
There was a problem hiding this comment.
| cmd = cmd.split() if isinstance(cmd, str) else cmd | |
| cmd = cmd if isinstance(cmd, list) else cmd.split() |
2 reasons:
- more obvious that
listis the preferred option - if
cmdis neither list or str by mistake, fail faster.
tests/conftest.py
Outdated
| # Python subprocess. This ensures that both Python-level stdout and any | ||
| # subprocess output invoked by `main` are captured together in a single | ||
| # string and also, that program-level setup and teardown in `main` happen | ||
| # fresh for each call. |
There was a problem hiding this comment.
Please mention briefly that this is best avoided because it makes it impossible or harder to use a debugger, etc. That's the one of the main purposes of this PR!
| # fresh for each call. | ||
| cmd = cmd.split() if isinstance(cmd, str) else cmd | ||
| cmd = [sys.executable, main.__file__] + cmd | ||
| print('running (subprocess):', cmd) |
There was a problem hiding this comment.
Naive question sorry: isn't there some logging function around here?
There was a problem hiding this comment.
I don't think so. There are print statements at other places as well in conftest.py.
tests/conftest.py
Outdated
| # This function is similar to `cmd()`, but runs the command in a separate | ||
| # Python subprocess. This ensures that both Python-level stdout and any | ||
| # subprocess output invoked by `main` are captured together in a single | ||
| # string and also, that program-level setup and teardown in `main` happen |
There was a problem hiding this comment.
stderr and stdout always race with each other, so this is a bad idea. Was it already like this? Either way, this deserves an elaboration: explaining that it is generally bad, then explaining why it is not too bad in this particular context.
There was a problem hiding this comment.
It was always this case, and I think the "racing" is not problematic as west does not invoke async subprocesses.
I will adapt the text little bit to point out the rare case when this function shall be used.
tests/test_project.py
Outdated
| stderr = io.StringIO() | ||
| actual = cmd('list zephyr -f {name}', stderr=stderr).splitlines() | ||
| assert actual == ['manifest'] | ||
| assert stderr.getvalue().splitlines() == expected_warns |
There was a problem hiding this comment.
This split between stdout and stderr looks unrelated to this commit. Can you please submit it in a separate PR?
There was a problem hiding this comment.
I have adapter the refactored cmd() function to capture combined stdout and stderr.
| # Prepend the west src directory to sys.path so that running this script | ||
| # always uses the local 'west' modules instead of any installed ones. | ||
| src_dir = Path(__file__).resolve().parents[2] | ||
| sys.path.insert(0, os.fspath(src_dir)) |
There was a problem hiding this comment.
The comment must explain what is the effect of this when people run west from a pip3 pipx/uv/uvx install or from their distro.
sys.path is security-sensitive, all use cases must be carefully considered.
You made this a separate commit. I love smaller commits and smaller PRs [*] but I wonder what happens when I try to use the previous, big refactoring without this commit?
[*] https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html#defining-smaller-prs
There was a problem hiding this comment.
There is no effect when people run west. As the comment says it has only an effect when running this script. I will adapt the comment little bit to point out that it only affects if running this script directly
There was a problem hiding this comment.
This is apparently not enough, see fixup:
There was a problem hiding this comment.
Please correct me: this 3rd commit and test addition is only to test the previous, sys.path commit? In other words, this is test code to test... code required for testing? Test coverage is great but this feels overkill... if the previous commit is really required for testing, then it's already going to be covered regularly, no? If not, then maybe that previous commit is not required that much.
Please correct me.
There was a problem hiding this comment.
Yes, this is just a test for the second commit.
Without the second commit, the cmd_subprocess does not use the correct local modules.
I am not sure if I see this as a test code. With this change also users can invoke west by running the main.py script directly. No need for them to install west or setting PYTHONPATH.
I will add this to the PR description.
The previous test setup used subprocess calls, which did not allow to debug the tests. It also caused a mix of Python modules from the installed West package and the local source tree when running tests directly with `pytest`. The tests have been updated so that `pytest` can now be run directly and fully debugged. `pytest` can be run as follows: - `pytest` — runs tests against the installed package - `pytest -o pythonpath=src` — runs tests against the local source tree Within the tests following methods can be used to run west commands: - cmd(...): call main() with given west command and capture std (and optionally stderr) - cmd_raises(...): call main() with given west command and catch expected exception. The exception and stderr are returned by default. Optionally stdout can be captured. - cmd_subprocess(...): Run west command in a subprocess and capture stdout.
When west/app/main.py is executed directly, also the correct West modules must be imported. Therefore, the Python module search path is configured appropriately before importing any West modules.
Test that the Python module search path is prepended with correct local module path if west/app/main.py is executed directly.
c2fcc62 to
26fd91d
Compare
Ping @mbolivar, we have the necessary approvals, but I agree with @marc-hb, I'd like to know your opinion. |
|
Is #149 not a problem anymore? AFAICT it's still open because of |
You're going to hate this answer... At least where tests are concerned, west.log isn't a problem, because we have no tests for it in west/tests :) |
|
Thanks! Considering it's deprecated, fine by me. It bet its logic was not very complicated and not crying for test coverage either. |
Discovered by chance after uninstall west and trying to run pytest directly. This was hopefully not a problem when running test with "uv"? This issue was probably introduced by zephyrproject-rtos#859. Commit 879bb01 seems relevant too. Signed-off-by: Marc Herbert <[email protected]>
Why?
I'm not sure how tests for the local source tree are currently debugged.
In my setup, I’m unable to run
pytestdirectly on the local tree, and debugging is difficult because it’s not possible to step through sincesubprocessis used in the current test setup.Proposed Changes
This change allows running
pytestdirectly, instead of only being able to run it viauv run poe test.pytestcan test either the installed West package or the local source tree, depending on how it’s invoked.This simplifies test execution and improves compatibility with
pytestintegrations (e.g. in IDEs).With this setup,
pytestis fully in charge for setting up the Python environment, so testing becomes straightforward:Background
During this work, I faced that many tests used
subprocess, which interfered with testing the local copy.Subprocesses do not inherit the full Python environment configured by
pytest, causing a mix between modules from the installed West package and those from the local source tree. Therefore I removed the use of subprocesses in tests wherever possible. Where subprocesses are still used, the python module (west/app/main.py) is called instead ofwestwhereby it is ensured now that its correct modules are used by prepending correct PYTHONPATH. With those changes only one Python environment managed bypytestis used, ensuring consistent behavior and enabling proper debugging.Additional infos
cmd()captures only Python-level stdout and does not include output from any subprocesses launched internally.If the code under test starts one or more subprocesses and the test verifies this combined stdout, the older
cmd_subprocess()behavior must be used, as this function captures all stdout, both from Python itself and from any spawned subprocesses.To simplify invoking west with corresponding modules, the
main.pyscript can now be executed directly.This approach is used in tests by
cmd_subprocess(), but also end users can also invokemain.pydirectly in their local tree if needed.