Skip to content

De-tox and uv-ify#845

Merged
pdgendt merged 11 commits intozephyrproject-rtos:mainfrom
pdgendt:detox
Oct 7, 2025
Merged

De-tox and uv-ify#845
pdgendt merged 11 commits intozephyrproject-rtos:mainfrom
pdgendt:detox

Conversation

@pdgendt
Copy link
Collaborator

@pdgendt pdgendt commented Sep 19, 2025

Remove the tox dependency, which allows to run the testing directly or with other tasks runners (here using poethepoet).

In CI (can be used locally too) we use uv to install and manage dependencies. A uv.lock file has pinned dependencies.

Some benefits of using uv:

  • Fast environment setup
  • Clean pyproject.toml management
  • Reproducible builds with lockfiles
  • Easy Python version switching

Check individual commits for details.

import pytest
from conftest import cmd

assert 'TOXTEMPDIR' in os.environ, "you must run these tests using tox"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ha, I'm finally understanding the "detox" pun :-)

This would be great: because of too many layers, so far my only solution to run a test in a debugger was PUDB_TTY. It worked... only most of the time!

Don't forget to update b7e091d

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unless... the uv replacement merely substitutes another layer of indirection, still getting in the way and still making it hard to run a test in a debugger directly?

Sorry for thinking out loud. Hey it's just a draft PR ;-)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I want to reduce the layering too, it should be possible to run pytest directly IMO. uv is an optional utility, but I do want to make it the default for running in CI.

@pdgendt pdgendt force-pushed the detox branch 3 times, most recently from 2fc8686 to c88bc8c Compare September 24, 2025 19:33
@codecov
Copy link

codecov bot commented Sep 24, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.52%. Comparing base (e6437d3) to head (e1f4f7f).
⚠️ Report is 11 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #845   +/-   ##
=======================================
  Coverage   84.52%   84.52%           
=======================================
  Files          11       11           
  Lines        3366     3366           
=======================================
  Hits         2845     2845           
  Misses        521      521           

see 2 files with indirect coverage changes

@pdgendt pdgendt force-pushed the detox branch 3 times, most recently from e799cb8 to d25a17e Compare September 28, 2025 19:00
@pdgendt pdgendt changed the title detox: wip Detox Sep 28, 2025
@pdgendt pdgendt marked this pull request as ready for review September 28, 2025 19:10
@pdgendt pdgendt self-assigned this Sep 29, 2025
@marc-hb marc-hb changed the title Detox De-tox and uv-ify Sep 29, 2025
@pdgendt pdgendt force-pushed the detox branch 3 times, most recently from 3e0c3f2 to 57bbf11 Compare September 30, 2025 07:13
@pdgendt
Copy link
Collaborator Author

pdgendt commented Sep 30, 2025

@thorsten-klein, as you're actively developing for west, would you mind sharing your opinion (if you have any) on this PR?

@thorsten-klein
Copy link
Contributor

@pdgendt Nice! Looking forward towards easier debugging of west tests. It seems that with this change pytest can be directly run 👍

Copy link
Contributor

@mbolivar mbolivar left a comment

Choose a reason for hiding this comment

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

Didn't test, but LGTM

Allow running tests without tox, so remove the check for a tox
environment.

Signed-off-by: Pieter De Gendt <[email protected]>
Specify compatible versions for the dependencies, allowing minor and
patch updates, while preventing major updates.

Signed-off-by: Pieter De Gendt <[email protected]>
Add dependencies for different development groups. The groups are
- lint: Linter tools
- test: Pytest and coverage tools
- types: Type checking tools

Signed-off-by: Pieter De Gendt <[email protected]>
Copy mypy options from tox.ini into pyproject.toml.

Signed-off-by: Pieter De Gendt <[email protected]>
Add poethepoet as a development dependency and add tasks for testing.

Signed-off-by: Pieter De Gendt <[email protected]>
Generate a pinned version of the project dependencies.

Signed-off-by: Pieter De Gendt <[email protected]>
Add generated files to omit in the coverage report.

Signed-off-by: Pieter De Gendt <[email protected]>
Use uv to install all pinned dependencies and poethepoet as task runner.

Signed-off-by: Pieter De Gendt <[email protected]>
Remove the tox configuration file. It's replaced with uv to manage
installations of dependencies and the environment.
Together with a simple task runner from poethepoet we can provide
shortcuts for users during development.

Signed-off-by: Pieter De Gendt <[email protected]>
Remove mentions of tox in the README and MAINTAINERS file. Update with
poethepoet and uv alternatives.

Signed-off-by: Pieter De Gendt <[email protected]>
Let dependebot check for Python updates on a monthly basis.

Signed-off-by: Pieter De Gendt <[email protected]>
@thorsten-klein
Copy link
Contributor

I had a try today and I was not able to run pytest directly. It seems that following points need to be tackled:

  1. An entry in pyproject.toml is necessary to extend the pythonpath with src folder.

  2. cmd() is calling 'west' which does not exist in the PATH. It would be nice if this function is reworked so that it can also be debugged.

I will have a closer look tomorrow 👍

Copy link
Collaborator

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

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

Minor comments that can all be addressed later.

I downloaded this and gave it good try and it felt great. Running pytest is especially easier. Thanks @pdgendt, not the least for forcing me to discover uv. It was worth it. Also, now I feel rejuvenated: like I belong to the cool kids!

- name: Run tox
run: tox -c 'check out' -- -W error
- name: Run tasks
run: uv run --frozen --directory "./check out/" poe all
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you not run "all" here to provide finer-grained results? And durations.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is close to what tox did, so I'll leave it for now. I think we should split indeed, but also reduce the matrix a bit. We don't need to run the linter task 15 times. Will follow-up!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

This new version of the README.rst seems to hesitate between embracing the new uv and poe approaches for "advanced" developers and sticking to the old pip ways for more basic stuff... I feel like this could needs more review time but I don't have that now. As long as all the examples were tested and do work, then good enough for now! Can be polished later after merging this. The README.rst tail should not wag the uv dog and delay this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wanted to keep options open for developers.

BTW, I intend to re-vamp the readme together with moving the docs from Zephyr to a West site, and create a dedicated page for testing west itself.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Big doc move WIP in:

Copy link
Collaborator

Choose a reason for hiding this comment

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

I hate mixing generated and non-generated files in the same git repo but who am I to question the wisdom of the new Youvee religion?

- name: Install tox
run: pip3 install tox --break-system-packages
- name: Install uv
run: pip3 install uv --break-system-packages
Copy link
Collaborator

Choose a reason for hiding this comment

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

GH runners are ephemeral so it does really matter but... just for better "role-modelling" maybe?

Suggested change
run: pip3 install uv --break-system-packages
run: pipx install uv

Only if pipx is available by default, otherwise forget it.

@marc-hb
Copy link
Collaborator

marc-hb commented Oct 7, 2025

I had a try today and I was not able to run pytest directly. It seems that following points need to be tackled...

Maybe not directly yet, but you can uv run pytest [--trace] -k something without setting any virtualenv or anything and this works even inside Emacs PDB :-)

@pdgendt
Copy link
Collaborator Author

pdgendt commented Oct 7, 2025

I had a try today and I was not able to run pytest directly. It seems that following points need to be tackled:

  1. An entry in pyproject.toml is necessary to extend the pythonpath with src folder.

It's a bit tricky, as we want to test an installment, rather than the sources directly? Not sure if the difference is even noticeable? We can document an pip install -e ., but that would need to be done for every pytest run, so maybe we need to do your suggestion.

  1. cmd() is calling 'west' which does not exist in the PATH. It would be nice if this function is reworked so that it can also be debugged.

Sure, we can improve on this, I'm going to go ahead with merging as this is a good basis IMO.

@pdgendt pdgendt merged commit ba70229 into zephyrproject-rtos:main Oct 7, 2025
37 checks passed
@pdgendt pdgendt deleted the detox branch October 7, 2025 06:04
@thorsten-klein
Copy link
Contributor

Sure, we can improve on this, I'm going to go ahead with merging as this is a good basis IMO.

Sounds good. I will then open the follow-up PR with my changes to run pytest directly 👍

Copy link
Collaborator

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

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

I will then open the follow-up PR with my changes to run pytest directly

- name: Run tox
run: tox -c 'check out' -- -W error
- name: Run tasks
run: uv run --frozen --directory "./check out/" poe all
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Big doc move WIP in:

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants