Skip to content

feat: move tool tests#2642

Open
paul-nechifor wants to merge 1 commit into
mainfrom
paul/feat/move-tool-tests
Open

feat: move tool tests#2642
paul-nechifor wants to merge 1 commit into
mainfrom
paul/feat/move-tool-tests

Conversation

@paul-nechifor

Copy link
Copy Markdown
Contributor

Problem

tool tests are not real tests but they interfere with pytest. You always have to specify -m 'not tool' in one place or another.

Closes DIM-1055

Solution

  • Rename test_ files which contain tool tests to tool_. That way they re not picked up by pytest.
  • You can still run them manually. E.g.: uv run pytest -s dimos/memory2/tool_module.py

@greptile-apps

greptile-apps Bot commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR removes the @pytest.mark.tool pattern in favour of renaming "dev-only pseudo-test" files from test_*.py to tool_*.py. Because pytest only collects test_*.py by default, tool files are no longer picked up during normal runs, eliminating the need for explicit -m 'not tool' filtering everywhere.

  • All @pytest.mark.tool-decorated functions are extracted from test_*.py files into sibling tool_*.py files, and the tool marker is removed from the codebase (conftest, pyproject.toml addopts, CI YAML, scripts, and docs).
  • The mypy exclusion pattern is extended to cover tool_*.py so type-checking stays consistent with the existing test_*.py exclusion.
  • Documentation and internal scripts (bin/pytest-coverage, bin/pytest-slow, bin/test-speed-leaderboard) are updated to drop the now-unnecessary -m 'not tool' flags.

Confidence Score: 5/5

Safe to merge — this is a pure structural reorganisation with no logic changes; the tool_*.py naming convention is consistently applied across all affected files.

Every @pytest.mark.tool usage is removed, all marker filter expressions updated, the mypy exclusion extended, and CI/docs/scripts aligned. No test logic is altered — code is only relocated. The two -m '' concerns in bin/pytest-coverage and docs/development/releasing.md were already captured in prior review rounds.

No files require special attention beyond what was already flagged in earlier review rounds.

Important Files Changed

Filename Overview
pyproject.toml Removes tool from the default addopts marker filter and extends the mypy exclusion regex to cover tool_*.py files — both changes are correct and consistent with the PR intent.
.github/workflows/ci.yml Drops tool from all four -m expressions in the CI test steps; no other logic changed.
dimos/conftest.py Removes the tool marker registration; consistent with the PR since all @pytest.mark.tool usages are also removed.
dimos/memory2/tool_module.py New tool file that re-uses module_cases from the sibling test_module.py; the double _reset_thread_pool() call is preserved from the original code.
dimos/mapping/tool_voxels.py New tool file that duplicates the fixtures from test_voxels.py so the tool is self-contained when run with pytest -s dimos/mapping/tool_voxels.py.
docs/development/testing.md Removes the tool marker section, adds a new 'Tool files' section explaining the tool_*.py convention and how to run them manually.
bin/pytest-coverage Replaces -m 'not tool' with -m ''; the empty expression overrides the addopts marker filter (noted in previous review thread).
docs/development/releasing.md Replaces -m 'not tool' with -m '' in the release validation command; combined with --error-for-skips, the empty marker expression overrides addopts (noted in previous review thread).

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[pytest run] --> B{File name matches\ntest_*.py?}
    B -- Yes --> C[Collected & run\nin normal suite]
    B -- No\ntool_*.py --> D[Not collected automatically]
    D --> E[Developer runs manually\npytest -s dimos/.../tool_file.py]
    C --> F{Marker filter\nnot self_hosted/mujoco/\nself_hosted_large}
    F -- Excluded --> G[Skipped in default run]
    F -- Included --> H[Runs in CI & locally]
    E --> I[Runs on demand\nno marker filtering needed]
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[pytest run] --> B{File name matches\ntest_*.py?}
    B -- Yes --> C[Collected & run\nin normal suite]
    B -- No\ntool_*.py --> D[Not collected automatically]
    D --> E[Developer runs manually\npytest -s dimos/.../tool_file.py]
    C --> F{Marker filter\nnot self_hosted/mujoco/\nself_hosted_large}
    F -- Excluded --> G[Skipped in default run]
    F -- Included --> H[Runs in CI & locally]
    E --> I[Runs on demand\nno marker filtering needed]
Loading

Reviews (2): Last reviewed commit: "feat: move tool tests" | Re-trigger Greptile

Comment thread bin/pytest-coverage
Comment thread docs/development/releasing.md
@codecov

codecov Bot commented Jun 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

@@            Coverage Diff             @@
##             main    #2642      +/-   ##
==========================================
+ Coverage   71.10%   71.60%   +0.50%     
==========================================
  Files         897      884      -13     
  Lines       80290    79199    -1091     
  Branches     7183     7124      -59     
==========================================
- Hits        57089    56711     -378     
+ Misses      21319    20605     -714     
- Partials     1882     1883       +1     
Flag Coverage Δ
OS-ubuntu-24.04-arm 63.96% <100.00%> (+0.44%) ⬆️
OS-ubuntu-latest 66.69% <100.00%> (+0.45%) ⬆️
Py-3.10 66.69% <100.00%> (+0.46%) ⬆️
Py-3.11 66.69% <100.00%> (+0.46%) ⬆️
Py-3.12 66.68% <100.00%> (+0.45%) ⬆️
Py-3.13 66.68% <100.00%> (+0.46%) ⬆️
Py-3.14 66.70% <100.00%> (+0.46%) ⬆️
Py-3.14t 66.69% <100.00%> (+0.46%) ⬆️
SelfHosted-Large 30.04% <100.00%> (-0.06%) ⬇️
SelfHosted-Linux 37.52% <100.00%> (+0.03%) ⬆️
SelfHosted-macOS 36.39% <100.00%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
dimos/conftest.py 79.64% <ø> (-0.18%) ⬇️
dimos/mapping/pointclouds/test_occupancy_speed.py 100.00% <100.00%> (+26.47%) ⬆️
dimos/mapping/test_voxels.py 96.10% <ø> (+20.14%) ⬆️
dimos/memory2/test_module.py 92.50% <ø> (+24.80%) ⬆️
dimos/memory2/test_voxel_map.py 100.00% <ø> (+21.42%) ⬆️
dimos/models/vl/test_base.py 100.00% <100.00%> (+45.83%) ⬆️
dimos/models/vl/test_vlm.py 21.87% <ø> (+2.37%) ⬆️
dimos/msgs/sensor_msgs/test_image.py 95.77% <ø> (+9.18%) ⬆️
dimos/protocol/pubsub/impl/rospubsub.py 82.39% <ø> (ø)

... and 15 files with indirect coverage changes

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Dreamsorcerer

Copy link
Copy Markdown
Collaborator

Yes please. I feel like most of these are used to create something, like a file? Seems like they shouldn't be written as tests at all to me. Would make more sense to add a simple CLI interface into each file under a if __name__ == "__main__": gate.

@paul-nechifor

Copy link
Copy Markdown
Contributor Author

Yes please. I feel like most of these are used to create something, like a file? Seems like they shouldn't be written as tests at all to me. Would make more sense to add a simple CLI interface into each file under a if __name__ == "__main__": gate.

I think for some pytest is required because they use fixtures. That's why I just renamed them.

@paul-nechifor paul-nechifor force-pushed the paul/feat/move-tool-tests branch from b8ec6e9 to 67a9c80 Compare June 29, 2026 15:57
@Dreamsorcerer

Copy link
Copy Markdown
Collaborator

Yes please. I feel like most of these are used to create something, like a file? Seems like they shouldn't be written as tests at all to me. Would make more sense to add a simple CLI interface into each file under a if __name__ == "__main__": gate.

I think for some pytest is required because they use fixtures. That's why I just renamed them.

Yeah, but the fixtures I saw looked like they were just for that one function, in which case it can just be converted to a regular function call. Bigger refactor though, I'm not saying to do it in this PR.

@github-actions github-actions Bot added the ready-to-merge Required CI checks have passed on this PR label Jun 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-merge Required CI checks have passed on this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants