-
Notifications
You must be signed in to change notification settings - Fork 726
WIP: importor skip #2650
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
base: main
Are you sure you want to change the base?
WIP: importor skip #2650
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would recommend against this. This will make it more difficult for downstream to test dimos. Instead, I suggest paying more attention to codecov (the CI checks should all be in place now). For release, we added the error-on-skips to ensure we do test everything before a release (should be an automated step soon), so any damage caused by an importorskip is short term. As prior art, we use this in several places in aiohttp for similar reasons: I also suspect that people will see the importskip being banned in future, and then just use the less preferred try/except pattern to achieve the same result.. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,97 @@ | ||
| # Copyright 2026 Dimensional Inc. | ||
| # | ||
| # Licensed under the Apache License, Version 2.0 (the "License"); | ||
| # you may not use this file except in compliance with the License. | ||
| # You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, software | ||
| # distributed under the License is distributed on an "AS IS" BASIS, | ||
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| import os | ||
|
|
||
| from dimos.constants import DIMOS_PROJECT_ROOT | ||
|
|
||
| PATTERN = "importorskip" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
||
|
|
||
| IGNORED_DIRS = { | ||
| ".venv", | ||
| "venv", | ||
| "__pycache__", | ||
| "node_modules", | ||
| ".git", | ||
| "dist", | ||
| "build", | ||
| ".egg-info", | ||
| ".tox", | ||
| } | ||
|
|
||
|
|
||
| def _is_ignored_dir(dirpath: str) -> bool: | ||
| parts = dirpath.split(os.sep) | ||
| return bool(IGNORED_DIRS.intersection(parts)) | ||
|
|
||
|
|
||
| def find_importorskip_usages() -> list[tuple[str, int, str]]: | ||
| """Return a list of (rel_path, line_number, line_text) for every violation.""" | ||
| dimos_dir = DIMOS_PROJECT_ROOT / "dimos" | ||
| violations: list[tuple[str, int, str]] = [] | ||
| # Skip this test file: it necessarily contains the forbidden pattern. | ||
| self_path = os.path.realpath(__file__) | ||
|
|
||
| for dirpath, dirnames, filenames in os.walk(dimos_dir): | ||
| dirnames[:] = [d for d in dirnames if d not in IGNORED_DIRS] | ||
|
|
||
| if _is_ignored_dir(dirpath): | ||
| continue | ||
|
|
||
| for fname in filenames: | ||
| if not fname.endswith(".py"): | ||
|
Comment on lines
+49
to
+53
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
||
| continue | ||
|
|
||
| full_path = os.path.join(dirpath, fname) | ||
| if os.path.realpath(full_path) == self_path: | ||
| continue | ||
| rel_path = os.path.relpath(full_path, DIMOS_PROJECT_ROOT) | ||
|
|
||
| try: | ||
| with open(full_path, encoding="utf-8", errors="replace") as f: | ||
| for lineno, line in enumerate(f, start=1): | ||
| stripped = line.rstrip("\n") | ||
| if PATTERN not in stripped: | ||
| continue | ||
| violations.append((rel_path, lineno, stripped)) | ||
| except (OSError, UnicodeDecodeError): | ||
| continue | ||
|
|
||
| return violations | ||
|
|
||
|
|
||
| def test_no_importorskip(): | ||
| """ | ||
| Fail if any file uses `pytest.importorskip`. | ||
|
|
||
| `importorskip` silently skips a test (or a whole module) when an optional | ||
| dependency is missing. That means a broken test, or a dependency missing | ||
| from the dev/CI environment, goes unnoticed because the test never runs. | ||
| Install all dependencies locally so the tests actually execute. If a | ||
| dependency genuinely cannot be installed in some environment, exclude those | ||
| tests explicitly (e.g. a CI `--ignore`, or a platform-conditioned | ||
| `collect_ignore` in a conftest) rather than silently skipping per import. | ||
| """ | ||
| violations = find_importorskip_usages() | ||
| if violations: | ||
| report_lines = [ | ||
| f"Found {len(violations)} forbidden use(s) of `pytest.importorskip`. " | ||
| "Don't silently skip tests when a dependency is missing -- install all " | ||
| "dependencies so the tests run, or exclude the module explicitly " | ||
| "(CI `--ignore` / a platform `collect_ignore`):", | ||
| "", | ||
| ] | ||
| for path, lineno, text in violations: | ||
| report_lines.append(f" {path}:{lineno}: {text.strip()}") | ||
| raise AssertionError("\n".join(report_lines)) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| # Copyright 2026 Dimensional Inc. | ||
| # | ||
| # Licensed under the Apache License, Version 2.0 (the "License"); | ||
| # you may not use this file except in compliance with the License. | ||
| # You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, software | ||
| # distributed under the License is distributed on an "AS IS" BASIS, | ||
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| import platform | ||
|
|
||
| # viser[urdf] has no installable wheel set on linux-aarch64 (see the `tests` | ||
| # dependency group marker in pyproject.toml), so these tests cannot be collected | ||
| # there: their module-level imports pull in viser. Mirror the skipif_aarch64 | ||
| # convention (dimos/conftest.py) at collection time. test_state.py is | ||
| # intentionally NOT listed -- it imports only viser.state (no viser dependency) | ||
| # and runs on every platform. | ||
| collect_ignore = ( | ||
| [ | ||
| "test_gui_status.py", | ||
| "test_operation_worker.py", | ||
| "test_viser_visualization.py", | ||
| "test_visualizer_lifecycle.py", | ||
| ] | ||
| if platform.machine() == "aarch64" | ||
| else [] | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--ignoreapplied toself_hosted_largemay permanently suppress coverageself_hosted_largeis the high-memory runner intended for heavier workloads. Ifdimos_mls_plannerordimos_voxel_ray_tracingare — or ever become — available on that runner (e.g. as a compiled native artifact cached in the runner image), adding the same--ignoreflags here means those test directories will never execute there either. A broken change to the ray-tracing or MLS-planner code would not be caught by any CI job. Consider whether theself_hosted_largerunner should build and run those suites instead of ignoring them.