Don't override users test selection with `spin test` by lagru · Pull Request #7889 · scikit-image/scikit-image

lagru · 2025-09-02T13:38:38Z

Description

The changes in #7864 would include --pyargs skimage even if in cases such as

spin test -- tests/skimage/...

which don't indicate that doctests should be run too. This fix makes sure to only add --pyargs if the user doesn't give any indication which tests are to be run in the form of positional arguments.

Checklist

A descriptive but concise pull request title
Docstrings for all functions
Unit tests
A gallery example in ./doc/examples for new features
Contribution guide is followed

Release note

For maintainers and optionally contributors, please refer to the instructions on how to document this PR for the release notes.

...

lagru · 2025-09-04T13:50:11Z

Head's up, I'll merge this maybe by the end of the week in case nobody objects. Should get rid of a few annoyances to the dev workflow. But it would be good to know if it works on other machines too. 😉

.spin/cmds.py

significant changes

stefanv

This works for me; perhaps we can get away with just adding src to the ignore list?

Also, can you help me think through whether any of these changes need to be upstreamed?

.spin/cmds.py

stefanv · 2025-12-03T08:27:40Z

.spin/cmds.py

+        # Pytest doesn't expect the doctest's sources and installation to be
+        # different. Avoid this - even if user specifies it by ignoring `src/`
+        # explicitly if not using an editable install
+        pytest_args = pytest_args + ('--ignore=./src/',)


Maybe should catch this and report the suppression, otherwise could be quite surprising.

Any reason to do the editable install if-clause in the first place?

I also think we need to document somewhere that doctests can only be speecified using skimage.module.name syntax.

Any reason to do the editable install if-clause in the first place?

Good thought. The user would have to manually specify the src/ directory. I'll see if we can simplify this.

I also think we need to document somewhere that doctests can only be speecified using skimage.module.name syntax.

That's not true for editable installs.

spin test src/skimage/morphology

will run doctests in skimage.morphology.

But

spin test -- skimage.morphology

works with both installation methods. So would be useful to document that as the default option.

After investigating, I opted to remove the implicit --ignore option. See f03cff1.

lagru · 2025-12-10T15:23:09Z

@stefanv, I've updated the PR and am reasonably happy with it right now. Since we are recommending an editable install in our documentation I updated the testing section in our contribution guide. That section needed updating anyway because it missed the changes to our source layout.

Have a look if you are happy with this.

matthew-brett

A quick pass.

Are we testing the editable and not-editable ways of calling spin test?

matthew-brett · 2025-12-31T13:09:34Z

.spin/cmds.py

+    is_out_of_tree_build = not _is_editable_install_of_same_source("scikit-image")
+    if is_out_of_tree_build and "src" in str(pytest_args):
+        click.secho(
+            "WARN: Found 'src' in test arguments and using out-of-tree build. "


Do you think it would be worth being more specific here? Such as looking for src/skimage in Path(arg) for arg in pytest_args or similar?

That might be detrimental. For example if we look for the more specific "src/skimage" we might miss someone using "spin test -- src" and fail to warn.

Yes - that's easily fixed with something like a check for /src in Path(arg).resolve() - but I guess, with the current code, you'd run into trouble with spin test -- tests/skimage/test_src.py or any other Pytest argument with src somewhere in the text. Can sketch up some code to do that if helpful.

Yes, I even think I had a more complex logic and more detailed warning message here. But at the same time this whole warning is a nicety that I don't anticipate to be triggered often. So I didn't want to over-engineer it. I'm happy to take suggestions though, and I may revisit this for a solution that is perhaps more inline with what you are getting at.

I would want an example. e.g.:

For example, don't use `spin test -- src/skimage/registration/_watershed.py`, use ...

And what should they use...?

Done in 69939e4.

However, I have to say that the more and more I investigate this house of cards, the more confused I get.

Without scikit-image being installed

spin test -- src/skimage/io # builds & runs doctests successfully spin test -- src/skimage/measure # builds & fails with ImportError spin test -- skimage.measure # builds & runs doctests successfully

The error in question (traceback)
``` _______________________ ERROR collecting src/skimage/measure/_find_contours.py ________________________ src/skimage/measure/_find_contours.py:3: in from ._find_contours_cy import _get_contour_segments E ModuleNotFoundError: No module named 'skimage.measure._find_contours_cy' ___________________________ ERROR collecting src/skimage/measure/_label.py ____________________________ src/skimage/measure/_label.py:2: in from ._ccomp import label_cython as clabel E ModuleNotFoundError: No module named 'skimage.measure._ccomp' ___________________ ERROR collecting src/skimage/measure/_marching_cubes_lewiner.py ___________________ src/skimage/measure/_marching_cubes_lewiner.py:6: in from . import _marching_cubes_lewiner_cy E ImportError: cannot import name '_marching_cubes_lewiner_cy' from 'skimage.measure' (/home/lg/Res/scikit-image/src/skimage/measure/__init__.py) __________________________ ERROR collecting src/skimage/measure/_moments.py ___________________________ src/skimage/measure/_moments.py:6: in from . import _moments_cy E ImportError: cannot import name '_moments_cy' from 'skimage.measure' (/home/lg/Res/scikit-image/src/skimage/measure/__init__.py) ________________________ ERROR collecting src/skimage/measure/_regionprops.py _________________________ src/skimage/measure/_regionprops.py:13: in from ._find_contours import find_contours E ImportError: cannot import name 'find_contours' from 'skimage.measure._find_contours' (/home/lg/Res/scikit-image/src/skimage/measure/_find_contours.py) ```

I'm suspecting that this is because compiled modules are involved in skimage/measure which isn't the case for skimage/io.

Apparently, there are now three (!) ways setups to compile and run scikit-image with spin:

spin build # out-of-tree build, spin required everywhere spin install # defaults to editable install, implicit re-build on import spin install --no-editable # conventional install, manual rebuild necessary by running again

And our tooling potentially has deal with all of those (at least fail gracefully).

pyproject.toml

matthew-brett · 2025-12-31T13:12:18Z

pyproject.toml

+    "--strict-markers",
+    "--maxfail=5",
+    # Necessary for doctest collection with for editable installations
+    "--import-mode=importlib",


The comments above seem to import that we should only use this mode for doctests - can you clarify?

If I remember correctly, this works fine for editable and out-of-tree scenarios. The limitation is that test modules can't import each other. But that isn't an issue we have right now.

Worth pinning down? Is there any disadvantage to having it on all the time?

I don't follow. Currently it's pinned and on all the time. I consider this an advantage because it forces the tests to be compatible with that import strategy for all building modes.

lagru · 2025-12-31T15:35:03Z

Are we testing the editable and not-editable ways of calling spin test?

Both should work. Not-editable installs (or out of tree builds) is what @stefanv uses. I'm using editable ones.

I think in practice this file is changed far to often for unrelated reasons. Thus resulting in a significant waste of resources (for example #7889 or #7976). We are periodically building and testing wheels for our nightlies. So we should still catch stuff early. And if necessary there's always the option to use a `maintenance/*` branch.

coderabbitai · 2026-01-28T20:35:36Z

Caution

Review failed

The pull request is closed.

📝 Walkthrough

Walkthrough

Refactors test command handling and pytest discovery: simplifies doctest option, detects out-of-tree builds to warn when src is in pytest args, removes automatic pytest arg defaults/--pyargs insertion, updates pytest paths/options, adjusts testing docs, and tweaks a test backend mock to use __name__.

Changes

Cohort / File(s)	Summary
Test command implementation `.spin/cmds.py`	Replaced complex doctest option block with `--doctest/--no-doctest`; imported `_is_editable_install_of_same_source`; detect out-of-tree builds and emit `_SRC_IN_TEST_ARGS_WARNING_MESSAGE` when `src` appears in `pytest_args`; removed default `pytest_args` and automatic `--pyargs`/import-mode handling; still appends `--doctest-plus` when doctest enabled and forwards `pytest_args`.
Pytest configuration `pyproject.toml`	Expanded `addopts` into a multi-line block adding `--import-mode=importlib` and `--pyargs`; broadened `testpaths` from `["tests"]` to `["skimage","skimage2","./tests/"]`.
Testing documentation `CONTRIBUTING.rst`	Rewrote Testing section from a detailed pytest command list to example-driven guidance for `spin test`, showing common invocation patterns and note about forwarding options after `--`.
Test utility `tests/skimage/util/test_backends.py`	Updated mock backend `info` to use `BackendInformation([f"{__name__}:foo"])` instead of hard-coded `"test_backends:foo"`, so the identifier reflects the module name at runtime.

Sequence Diagram(s)

sequenceDiagram
  participant User as User
  participant Spin as spin/cmds.py
  participant Meson as spin.cmds.meson._is_editable_install_of_same_source
  participant Pytest as pytest

  User->>Spin: run `spin test` with pytest_args
  Spin->>Meson: call _is_editable_install_of_same_source("scikit-image")
  Meson-->>Spin: editable_install_of_same_source? (bool)
  alt out-of-tree build AND "src" in pytest_args
    Spin->>User: emit `_SRC_IN_TEST_ARGS_WARNING_MESSAGE`
  end
  Spin->>Pytest: invoke pytest with final pytest_args (append --doctest-plus if enabled)
  Pytest-->>User: test results

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested labels

:adhesive_bandage: type: Bug fix

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and accurately summarizes the main change: preventing `spin test` from automatically adding test arguments that override user-specified test selection.
Description check	✅ Passed	The description is directly related to the changeset, explaining the problem addressed (unconditional `--pyargs` addition), the solution (only add it when user doesn't specify tests), and mentions documentation updates.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (1)

.spin/cmds.py (1)

87-105: Make the out-of-tree “src” warning path-aware to avoid false positives.

"src" in str(pytest_args) can warn for non-path usage (e.g., -k src). Consider checking path segments instead.

♻️ Suggested refinement

-    if is_out_of_tree_build and "src" in str(pytest_args):
+    def _has_src_path(args):
+        return any(
+            isinstance(arg, str)
+            and (arg == "src" or arg.startswith(("src/", f"src{os.sep}")))
+            for arg in args
+        )
+
+    if is_out_of_tree_build and _has_src_path(pytest_args):
         click.secho(_SRC_IN_TEST_ARGS_WARNING_MESSAGE, fg="yellow", bold=True)

CONTRIBUTING.rst

This fix is picked from scikit-imagegh-7889.

stefanv · 2026-01-31T23:44:23Z

CONTRIBUTING.rst

+    # Tests inside directory(s)
+    spin test -- tests/skimage/morphology
+    spin test -- src/skimage/morphology tests/skimage/morphology


This category example is not correct for out-of-tree builds.

stefanv

Let's give it a whirl.

Previously, something like `spin test -- tests/skimage/...` would still run all doctests because `--pyargs skimage` was added. This fix makes sure to only add `--pyargs` if the user doesn't give any indication which tests are to be run.

This solution hopefully avoids the most unexpected failure modes for both editable and out-of-tree installations. Should also work for the CI. I'm not entirely sure if always using `--import-mode=importlib` has unforeseen consequences. But our local dev setup forces use to work with the constraints [1] imposed by `--import-mode=importlib` anyway. [1] https://docs.pytest.org/en/stable/explanation/pythonpath.html

Both pytests --import-mode and the location of the test file itself change it's `__name__` which is relied on implicitly in tests such as `test_notification_raised`. So instead of hardcoding it, just use the content of `__name__`.

Co-authored-by: Stefan van der Walt <sjvdwalt@gmail.com>

After local testing, it seems that it has no effect for out-of-tree builds. I remember it working in the past but maybe I missed something. Because I can't make it work (--ignore-glob also doesn't), I'm instead opting to print a helpful warning in case `src/` is passed with an out-of-tree build.

`--import-mode=importlib` is now set by default in pyproject.toml.

While this is outdated, this should be handled in another PR.

Co-authored-by: Matthew Brett <matthew.brett@gmail.com>

lagru added this to the 0.26 milestone Sep 2, 2025

lagru requested a review from stefanv September 2, 2025 13:38

lagru added the 🤖 type: Infrastructure CI, packaging, tools and automation label Sep 2, 2025

stefanv previously approved these changes Sep 5, 2025

View reviewed changes

stefanv reviewed Sep 5, 2025

View reviewed changes

.spin/cmds.py Outdated Show resolved Hide resolved

lagru requested a review from stefanv September 15, 2025 15:02

lagru modified the milestones: 0.26, 0.27 Nov 18, 2025

stefanv reviewed Dec 3, 2025

View reviewed changes

lagru requested a review from stefanv December 10, 2025 15:19

lagru mentioned this pull request Dec 29, 2025

Don't trigger "Release wheels" workflow on pyproject.toml change #7995

Merged

matthew-brett reviewed Dec 31, 2025

View reviewed changes

coderabbitai bot added the 🩹 type: Bug fix Fixes unexpected or incorrect behavior label Jan 28, 2026

coderabbitai bot reviewed Jan 28, 2026

View reviewed changes

CONTRIBUTING.rst Show resolved Hide resolved

lagru requested a review from matthew-brett January 28, 2026 20:44

lagru removed the 🩹 type: Bug fix Fixes unexpected or incorrect behavior label Jan 28, 2026

lagru mentioned this pull request Jan 30, 2026

Add revised peak_local_max to skimage2 #8039

Merged

2 tasks

lagru added a commit to lagru/scikit-image that referenced this pull request Jan 30, 2026

Apply fix for importlib mode that breaks backend tests

82f1b04

This fix is picked from scikit-imagegh-7889.

stefanv reviewed Jan 31, 2026

View reviewed changes

lagru and others added 4 commits January 31, 2026 15:45

Improve grammar & spelling

aefbd03

Co-authored-by: Stefan van der Walt <sjvdwalt@gmail.com>

lagru and others added 8 commits January 31, 2026 15:45

Update --doctest option

8839f53

`--import-mode=importlib` is now set by default in pyproject.toml.

Clarify use of --pyargs in pyproject.toml

a939c2b

Update recommended spin test usage in contribution guide

9fb539a

Re-add "Warnings during testing phase" to contribution guide

b51621e

While this is outdated, this should be handled in another PR.

Update pyproject.toml

58b25f9

Co-authored-by: Matthew Brett <matthew.brett@gmail.com>

Offer example in warning

65b5f50

Address types pointed out by LLM

2db5306

stefanv force-pushed the impr-spin-test-selection branch from d0987b9 to 2db5306 Compare January 31, 2026 23:45

stefanv approved these changes Jan 31, 2026

View reviewed changes

stefanv merged commit c301380 into scikit-image:main Jan 31, 2026
22 of 25 checks passed

coderabbitai bot added the 🩹 type: Bug fix Fixes unexpected or incorrect behavior label Jan 31, 2026

lagru deleted the impr-spin-test-selection branch February 2, 2026 22:06

Uh oh!

Conversation

lagru commented Sep 2, 2025

Description

Checklist

Release note

Uh oh!

lagru commented Sep 4, 2025

Uh oh!

Uh oh!

stefanv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lagru Dec 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lagru commented Dec 10, 2025

Uh oh!

matthew-brett left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lagru commented Dec 31, 2025

Uh oh!

coderabbitai bot commented Jan 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review failed

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Suggested labels

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stefanv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

lagru Dec 10, 2025 •

edited

Loading

coderabbitai bot commented Jan 28, 2026 •

edited

Loading