chore: Drop test against pyside2, fix test for napari 0.7.0, add testing against python 3.13 #1352
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideUpdates tox environments and CI workflow to stop running full napari-repo tests with PySide2 and to add Python 3.13 coverage for napari-repo, while keeping widget-only tests for released napari versions across all backends. Flow diagram for updated tox environments and backend coverageflowchart TD
start["Start tox invocation"] --> select_env["Select tox environment"]
subgraph released_napari["Widget tests for released napari"]
select_env --> env_widgets
env_widgets --> choose_py_version["Python version py39, py310, py311, py312, py313"]
choose_py_version --> choose_backend["Qt backend PyQt5, PySide2, PyQt6, PySide6"]
choose_backend --> choose_napari_version["napari version 0.4.19 or 0.5.4"]
choose_napari_version --> install_released["Install napari from PyPI"]
install_released --> run_widgets_pytest["Run pytest on test_napari_widgets with JSON report"]
end
subgraph napari_repo["Full tests for napari repo"]
select_env --> env_repo
env_repo --> choose_py_version_repo["Python version py310, py311, py312, py313"]
choose_py_version_repo --> choose_backend_repo["Qt backend PyQt5, PyQt6, PySide6"]
choose_backend_repo --> install_repo["Install napari from GitHub napari/napari"]
install_repo --> run_full_pytest["Run pytest on package/tests with JSON report"]
end
run_widgets_pytest --> end_widgets["End widget test env"]
run_full_pytest --> end_repo["End repo test env"]
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughAdds Python 3.13 to the napari CI matrix, reorganizes tox to isolate repo-based napari tests into dedicated envs, and defers displaying napari Viewer windows by creating them with Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In the new
[testenv:py{310,311,312,313}-{PyQt5,PyQt6,PySide6}-napari_repo]you dropped the-vflag from the pytest invocation compared to the other napari testenvs; if that wasn’t intentional, consider adding-vfor consistent output across environments. - The pytest command and JSON report options are now duplicated between the napari widget env and the new
napari_repoenv; consider extracting the common command into a factor or a sharedcommandssnippet to keep the tox config DRY and easier to maintain.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the new `[testenv:py{310,311,312,313}-{PyQt5,PyQt6,PySide6}-napari_repo]` you dropped the `-v` flag from the pytest invocation compared to the other napari testenvs; if that wasn’t intentional, consider adding `-v` for consistent output across environments.
- The pytest command and JSON report options are now duplicated between the napari widget env and the new `napari_repo` env; consider extracting the common command into a factor or a shared `commands` snippet to keep the tox config DRY and easier to maintain.
## Individual Comments
### Comment 1
<location> `tox.ini:84` </location>
<code_context>
+ python -m pytest -v package/tests/test_PartSeg/test_napari_widgets.py --json-report --json-report-file={toxinidir}/report-{envname}-{sys_platform}.json {posargs}
+
+
+[testenv:py{310,311,312,313}-{PyQt5,PyQt6,PySide6}-napari_repo]
+deps =
+ {[testenv]deps}
</code_context>
<issue_to_address>
**question (testing):** The napari_repo env drops PySide2 and py39; confirm this reduction in matrix is intentional.
Previously, `napari_repo` ran on `py{39,310,311,312,313}-{PyQt5,PySide2,PyQt6,PySide6}`; now it’s only `py{310,311,312,313}-{PyQt5,PyQt6,PySide6}`. Unless this is due to an intentional upstream drop of py39/PySide2 support, consider keeping those combos so we don’t silently lose coverage for configurations users may still depend on.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| python -m pytest -v package/tests/test_PartSeg/test_napari_widgets.py --json-report --json-report-file={toxinidir}/report-{envname}-{sys_platform}.json {posargs} | ||
|
|
||
|
|
||
| [testenv:py{310,311,312,313}-{PyQt5,PyQt6,PySide6}-napari_repo] |
There was a problem hiding this comment.
question (testing): The napari_repo env drops PySide2 and py39; confirm this reduction in matrix is intentional.
Previously, napari_repo ran on py{39,310,311,312,313}-{PyQt5,PySide2,PyQt6,PySide6}; now it’s only py{310,311,312,313}-{PyQt5,PyQt6,PySide6}. Unless this is due to an intentional upstream drop of py39/PySide2 support, consider keeping those combos so we don’t silently lose coverage for configurations users may still depend on.
Czaki
changed the title
Czaki
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tox.ini`:
- Around line 84-89: The napari git dependency in the testenv named
testenv:py{310,311,312,313}-{PyQt5,PyQt6,PySide6}-napari_repo is unpinned;
replace the deps entry git+https://github.com/napari/napari.git with a pinned
reference to a tag or commit (e.g.
git+https://github.com/napari/napari.git@vX.Y.Z or
git+https://github.com/napari/napari.git@<commit-hash>#egg=napari) to ensure CI
reproducibility, updating the deps line in that testenv block (keep the env name
and other deps identical).
tox.ini
Outdated
| [testenv:py{310,311,312,313}-{PyQt5,PyQt6,PySide6}-napari_repo] | ||
| deps = | ||
| {[testenv]deps} | ||
| git+https://github.com/napari/napari.git | ||
| commands = | ||
| python -m pytest package/tests --json-report --json-report-file={toxinidir}/report-{envname}-{sys_platform}.json {posargs} |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
PyQt5 Python 3.13 wheels support
💡 Result:
PyQt5 does support Python 3.13 via wheels, but not as “cp313” wheels.
- The latest PyQt5 release on PyPI (5.15.11, uploaded Jul 19, 2024) publishes
cp38-abi3wheels for Windows (x86/x64), macOS (x86_64/arm64), and Linux x86_64 (manylinux glibc ≥ 2.17). Because they’reabi3wheels, they are usable on CPython 3.8+ (including 3.13). [1] - If your install pulls in Qt libraries via
PyQt5-Qt5/pyqt5-qt5, note that its wheels exist for Linux x86_64 and macOS, and (at least for recent versions like 5.15.18, Nov 9 2025) do not include Windows wheels. [2] (On Windows, PyQt5’s own wheel is the one you’ll typically rely on.) - On non-x86_64 Linux (e.g., Raspberry Pi), you may need platform-specific wheels from third parties (example:
pyqt5-siphas cp313 wheels on piwheels), or build from source / use conda. [3]
What to do: on Python 3.13, try pip install PyQt5—if you’re on a supported platform above, pip should select the abi3 wheel automatically. [1]
Sources: PyQt5 on PyPI [1], pyqt5-qt5 on PyPI [2], piwheels pyqt5-sip builds [3].
🌐 Web query:
PyQt5 version 5.15 Python 3.13 compatibility
💡 Result:
PyQt5 5.15.x is compatible with Python 3.13, as long as you install a recent PyQt5 5.15 release and its current dependencies:
- PyQt5 (latest 5.15.11) on PyPI declares “Requires: Python >=3.8” and ships
cp38-abi3wheels, which are intended to work on CPython 3.8+ (including 3.13). (pypi.org) - PyQt5-sip provides CPython 3.13 (
cp313) wheels in recent releases (e.g., 12.18.0, uploaded Jan 13, 2026), which is important because PyQt5 depends on it. (pypi.org)
Practical guidance:
- Use
pip install PyQt5(don’t pin an old PyQt5-sip), so pip can pick up a cp313 PyQt5-sip wheel automatically. (pypi.org)
Citations:
- 1: https://pypi.org/project/PyQt5/
- 2: https://pypi.org/project/pyqt5-sip/?utm_source=openai
- 3: https://pypi.org/project/pyqt5-sip/?utm_source=openai
Pin the napari git dependency to a tag/commit for CI reproducibility.
Using an unpinned git+ dependency makes builds non-deterministic and can break on upstream changes. Pin to a specific tag or commit hash.
PyQt5 supports Python 3.13 via cp38-abi3 wheels (PyQt5 5.15.11+) and its pyqt5-sip dependency provides cp313 wheels, so py313-PyQt5 does not need exclusion on supported platforms (Windows x86/x64, macOS, Linux x86_64).
🔧 Suggested pinning for reproducibility
deps =
{[testenv]deps}
- git+https://github.com/napari/napari.git
+ git+https://github.com/napari/napari.git@<tag-or-commit>🤖 Prompt for AI Agents
In `@tox.ini` around lines 84 - 89, The napari git dependency in the testenv named
testenv:py{310,311,312,313}-{PyQt5,PyQt6,PySide6}-napari_repo is unpinned;
replace the deps entry git+https://github.com/napari/napari.git with a pinned
reference to a tag or commit (e.g.
git+https://github.com/napari/napari.git@vX.Y.Z or
git+https://github.com/napari/napari.git@<commit-hash>#egg=napari) to ensure CI
reproducibility, updating the deps line in that testenv block (keep the env name
and other deps identical).
There was a problem hiding this comment.
This workflow is intentionally to check if there is some napari incompatibility before the release of the next version
There was a problem hiding this comment.
@Czaki, thank you for the clarification! You're absolutely right—testing against the latest napari development version is a great practice for catching compatibility issues early. The unpinned dependency is appropriate for this pre-release testing workflow.
I appreciate the context and apologize for the noise on this one.
✏️ Learnings added
Learnt from: Czaki
Repo: 4DNucleome/PartSeg PR: 1352
File: tox.ini:84-89
Timestamp: 2026-02-06T10:35:06.033Z
Learning: In the PartSeg repository, the testenv:py{310,311,312,313}-{PyQt5,PyQt6,PySide6}-napari_repo in tox.ini intentionally uses an unpinned git dependency (git+https://github.com/napari/napari.git) to test against the latest napari development version and catch incompatibilities before napari releases.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: Czaki
Repo: 4DNucleome/PartSeg PR: 1206
File: .github/workflows/test_napari_widgets.yml:30-30
Timestamp: 2024-10-17T12:19:43.905Z
Learning: In the CI configuration, `napari54` corresponds to napari version `0.5.4`.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1352 +/- ##
========================================
Coverage 93.15% 93.15%
========================================
Files 210 210
Lines 33253 33256 +3
========================================
+ Hits 30978 30981 +3
Misses 2275 2275 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tox.ini (1)
7-7:⚠️ Potential issue | 🟡 Minor
envlistis out of sync with the newnapari_repotestenv.The envlist still generates
napari_repocombinations that no longer have a matching testenv:
py39-{PyQt5,PyQt6}-napari_repo— py39 is not in the newnapari_repoenv (line 84).py{39,310}-PySide2-napari_repo— PySide2 is excluded from the newnapari_repoenv.PySide6-napari_repocombinations exist in the new testenv but are absent from the envlist.These orphaned env names would silently fall through to the default
[testenv](no napari installed), producing misleading results if anyone runs the full envlist locally.Suggested fix
-envlist = py{39,310,311,312,313}-{PyQt5,PySide2,PyQt6,PySide6}-all, py{39,310,311,312,313}-{PyQt5,PyQt6}-napari_{419,54,repo}, py{39,310}-PySide2-napari_{419,54,repo} +envlist = py{39,310,311,312,313}-{PyQt5,PySide2,PyQt6,PySide6}-all, py{39,310,311,312,313}-{PyQt5,PySide2,PyQt6,PySide6}-napari_{419,54}, py{310,311,312,313}-{PyQt5,PyQt6,PySide6}-napari_repo, py{39,310}-PySide2-napari_{419,54}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tox.ini`:
- Line 7: The envlist includes py39 in napari_repo combos but there is no
matching testenv for Python 3.9; update either the envlist or the testenv to
align them: either remove py39 from the napari_repo patterns in the envlist (the
envlist variable) so napari_repo only targets py310+ or add py39 to the
napari_repo testenv definition (the
[testenv:py{310,311,312,313}-...-napari_repo] group) so py39-PyQt*-napari_repo
environments are covered; ensure the names in envlist and the napari_repo
testenv share the same Python set.
![semgrep-app[bot]](https://avatars.githubusercontent.com/in/60555?s=60&v=4){kind=small}
| timeout-minutes: ${{ inputs.timeout }} | ||
| with: | ||
| run: python -m tox ${{ inputs.tox_args }} -- ${{ inputs.pytest_args }} | ||
| run: python -m tox ${{ inputs.tox_args }} ${{ inputs.pytest_args != '' && format('-- {0}', inputs.pytest_args) || '' }} |
There was a problem hiding this comment.
Using variable interpolation ${{...}} with github context data in a run: step could allow an attacker to inject their own code into the runner. This would allow them to steal secrets and code. github context data can have arbitrary user input and should be treated as untrusted. Instead, use an intermediate environment variable with env: to store the data and use the environment variable in the run: script. Be sure to use double-quotes the environment variable, like this: "$ENVVAR".
🌟 Fixed in commit ebecc1a 🌟
There was a problem hiding this comment.
@coderabbitai Did you agree with the semgrep check? What are possible solutions?
There was a problem hiding this comment.
@Czaki, I agree with the Semgrep check. This is a legitimate security concern, even though the immediate risk may be low in your case.
The Risk:
The warning is valid because ${{ inputs.pytest_args }} and ${{ inputs.tox_args }} are directly interpolated into the shell command. If these inputs ever contain shell metacharacters (like ;, |, &&, etc.) or are derived from untrusted sources, an attacker could inject arbitrary commands.
Since this is a reusable workflow (workflow_call), the risk depends on:
- Who controls the calling workflows
- Whether those workflows pass sanitized or potentially untrusted data
Recommended Solution:
Use environment variables to pass the inputs, then reference them with proper quoting in the shell script:
- name: Test with tox all
if: ${{ inputs.napari != 'latest' }}
uses: aganders3/headless-gui@v2
timeout-minutes: ${{ inputs.timeout }}
with:
run: |
if [ -n "$PYTEST_ARGS" ]; then
python -m tox $TOX_ARGS -- "$PYTEST_ARGS"
else
python -m tox $TOX_ARGS
fi
env:
PYVISTA_OFF_SCREEN: True
NAPARI: ${{ inputs.napari }}
BACKEND: ${{ inputs.qt_backend }}
PIP_CONSTRAINT: ${{ inputs.napari == 'latest' && format('requirements/constraints_py{0}{1}.txt', inputs.python_version, inputs.pydantic ) || 'requirements/version_denylist.txt' }}
UV_CONSTRAINT: ${{ inputs.napari == 'latest' && format('requirements/constraints_py{0}{1}.txt', inputs.python_version, inputs.pydantic ) || 'requirements/version_denylist.txt' }}
TOX_ARGS: ${{ inputs.tox_args }}
PYTEST_ARGS: ${{ inputs.pytest_args }}Note: You should apply this pattern consistently to the other test steps in the file (lines ~95, ~104, ~114) that also use inputs.tox_args and inputs.pytest_args.
Alternative (if you want to keep it simpler):
If you're certain the inputs are always controlled by maintainers and will never contain special characters, you could mark this as acceptable risk in Semgrep with /ar as suggested. However, using environment variables is the safer long-term approach.
🧠 Learnings used
Learnt from: Czaki
Repo: 4DNucleome/PartSeg PR: 1352
File: tox.ini:84-89
Timestamp: 2026-02-06T10:35:13.315Z
Learning: In the PartSeg repository, the testenv:py{310,311,312,313}-{PyQt5,PyQt6,PySide6}-napari_repo in tox.ini intentionally uses an unpinned git dependency (git+https://github.com/napari/napari.git) to test against the latest napari development version and catch incompatibilities before napari releases.
There was a problem hiding this comment.
But pytest args could contain multiple parameters, like file path, test selector, etc. So it cannot be passed as one argument that happens after quoting.
There was a problem hiding this comment.
@Czaki, you're absolutely right! My apologies for the confusion. Quoting "$PYTEST_ARGS" would indeed treat it as a single argument, breaking the intended behavior.
Corrected Solution:
Use environment variables but leave them unquoted in the shell command to allow proper word splitting:
- name: Test with tox all
if: ${{ inputs.napari != 'latest' }}
uses: aganders3/headless-gui@v2
timeout-minutes: ${{ inputs.timeout }}
with:
run: |
if [ -n "$PYTEST_ARGS" ]; then
python -m tox $TOX_ARGS -- $PYTEST_ARGS
else
python -m tox $TOX_ARGS
fi
env:
PYVISTA_OFF_SCREEN: True
NAPARI: ${{ inputs.napari }}
BACKEND: ${{ inputs.qt_backend }}
PIP_CONSTRAINT: ${{ inputs.napari == 'latest' && format('requirements/constraints_py{0}{1}.txt', inputs.python_version, inputs.pydantic ) || 'requirements/version_denylist.txt' }}
UV_CONSTRAINT: ${{ inputs.napari == 'latest' && format('requirements/constraints_py{0}{1}.txt', inputs.python_version, inputs.pydantic ) || 'requirements/version_denylist.txt' }}
TOX_ARGS: ${{ inputs.tox_args }}
PYTEST_ARGS: ${{ inputs.pytest_args }}Why this works:
-
Prevents GitHub Actions expression injection: Moving
${{ inputs.* }}to environment variables prevents attackers from injecting malicious expressions through the GitHub Actions interpolation mechanism (which is what Semgrep warns about). -
Preserves word splitting: Using
$PYTEST_ARGSunquoted allows the shell to properly split it into multiple arguments, so-k test_name /path/to/test.py --verbosebecomes three separate arguments. -
Safe for your use case: Since this is a reusable workflow where inputs come from workflows you control, the content itself is trusted.
The same pattern should be applied to the other test steps at lines ~95, ~104, and ~114.
🧠 Learnings used
Learnt from: Czaki
Repo: 4DNucleome/PartSeg PR: 1352
File: tox.ini:84-89
Timestamp: 2026-02-06T10:35:13.315Z
Learning: In the PartSeg repository, the testenv:py{310,311,312,313}-{PyQt5,PyQt6,PySide6}-napari_repo in tox.ini intentionally uses an unpinned git dependency (git+https://github.com/napari/napari.git) to test against the latest napari development version and catch incompatibilities before napari releases.
There was a problem hiding this comment.
So when the split happens, what prevents the usage of shell metacharacters? Is passing through the env variable is enough?
There was a problem hiding this comment.
@Czaki, excellent question! You've identified an important distinction. Let me clarify:
Passing through environment variables does NOT prevent shell metacharacter exploitation. You're absolutely right that if PYTEST_ARGS contains something like --verbose; rm -rf /, the shell would still interpret the semicolon and execute malicious commands.
What it DOES prevent:
The environment variable approach specifically prevents GitHub Actions expression injection, which is what Semgrep is flagging. This is when an attacker injects malicious GitHub Actions expressions like ${{ }} that get evaluated by the workflow engine itself before the shell even sees them.
The two different attack vectors:
-
GitHub Actions expression injection (what Semgrep warns about):
- Attacker controls a GitHub context variable (e.g.,
github.event.issue.title) - That value contains
}}; malicious code; echo ${{ - Direct interpolation with
${{ }}allows the attacker to break out and inject commands at the workflow level
- Attacker controls a GitHub context variable (e.g.,
-
Shell injection (still possible with env vars):
- Input contains shell metacharacters like
;,|,&&, etc. - These are interpreted by the shell during execution
- Input contains shell metacharacters like
Why this is acceptable in your case:
The inputs to this reusable workflow come from workflow_call, meaning they're passed by other workflows you control in the same repository. They're not derived from untrusted sources like PR titles, issue comments, or external webhook data.
Since the calling workflows are maintained by you and the inputs are typed strings defined in the workflow schema, the threat model is significantly reduced. The environment variable approach satisfies Semgrep because it prevents the more dangerous GitHub Actions expression injection vector.
If you needed absolute protection against both:
You'd need to either:
- Validate/sanitize inputs before passing them
- Use a JSON array and parse it safely
- Quote arguments individually (which breaks your use case)
But given your threat model, the env variable approach is a reasonable balance between security and functionality.
🧠 Learnings used
Learnt from: Czaki
Repo: 4DNucleome/PartSeg PR: 1352
File: tox.ini:84-89
Timestamp: 2026-02-06T10:35:13.315Z
Learning: In the PartSeg repository, the testenv:py{310,311,312,313}-{PyQt5,PyQt6,PySide6}-napari_repo in tox.ini intentionally uses an unpinned git dependency (git+https://github.com/napari/napari.git) to test against the latest napari development version and catch incompatibilities before napari releases.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Czaki
changed the title
|
@coderabbitai How to change your configuration to not expect docstrings in test functions? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🛠 Debug Information Gathered
Option 1: Path-specific instructions (Recommended)Add the following to your reviews:
path_instructions:
- path: "**/test_*.py"
instructions: "Docstrings are not required for test functions in this file."
- path: "**/tests/**/*.py"
instructions: "Docstrings are not required for test functions in test files."This tells CodeRabbit to not expect docstrings specifically in test files that match these patterns. Option 2: Adjust docstring coverage thresholdIf you want to relax docstring requirements globally or turn off the pre-merge check: reviews:
pre_merge_checks:
docstrings:
mode: off # Options: off, warning, error
# OR keep it enabled but lower the threshold
# mode: warning
# threshold: 50 # Lower percentage if you want some coverageOption 3: Customize docstring generation for testsIf you want to control how docstrings are generated for test files: code_generation:
docstrings:
path_instructions:
- path: "**/test_*.py"
instructions: "Do not generate docstrings for test functions."
- path: "**/tests/**/*.py"
instructions: "Skip docstring generation for test functions."RecommendationI recommend Option 1 with path-specific instructions, as it gives you fine-grained control while keeping docstring checks active for non-test code. You can adjust the path patterns to match your test file structure. You can find more details about path-based instructions in the CodeRabbit documentation. 🧠 Learnings used |
…test (#1355) #1352 (comment) ## Summary by Sourcery Build: - Update CodeRabbit configuration to exempt Python test files from requiring docstrings. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Updated code review configuration to clarify that docstrings are not required for test functions in test files. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
1 participant
Add testing napari repo against python 3.13
Summary by Sourcery
Update napari testing configuration to separate repo-based tests from pinned napari versions and expand CI coverage to Python 3.13.
Enhancements:
CI:
Summary by CodeRabbit