fix: adjust conda tests to not fail on apple silicon; fix #4040#4049
Conversation
update versions for some tests such that packages are available on macos-arm64. where the test behaviour depends on a specific old version, skip this test.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded Linux and Apple Silicon platform checks and a macOS-ARM pytest skip marker; applied macOS-ARM and Linux-specific skips to conda-related tests; bumped R package versions in a test Conda env; and switched a test Snakefile wrapper to bgzip. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/conftest.py`:
- Around line 12-17: Replace the unreliable platform.processor() check for Apple
Silicon with platform.machine(); specifically update the ON_APPLE_SILICON
constant (used by skip_on_macos_arm) to use platform.machine().lower() ==
"arm64" so macOS ARM64 (Apple Silicon) runners are correctly detected and the
pytest.mark.skipif gate skip_on_macos_arm behaves as intended.
In `@tests/tests_using_conda.py`:
- Line 270: The test decorator references an undefined symbol ON_LINUX causing a
NameError; fix by defining or importing ON_LINUX before use (or replace the
condition inline). Locate the pytest.mark.skipif(...) line and either import a
shared constant (e.g., add an import that exposes ON_LINUX) or add a
module-level definition such as computing ON_LINUX from
sys.platform/platform.system, or change the decorator to use an equivalent
expression directly (e.g., sys.platform.startswith('linux')) so
pytest.mark.skipif can evaluate correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 71505c6c-2eb0-424e-aa19-844d43c818fb
📒 Files selected for processing (4)
tests/conftest.pytests/test_issue635/envs/rmarkdown.yamltests/test_wrapper/Snakefiletests/tests_using_conda.py
15bc1d1 to
7800ef4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/tests_using_conda.py`:
- Line 276: The skip condition currently uses ON_LINUX which is true for all
Linux arches; change it to require linux-64 specifically by updating the
pytest.mark.skipif to check both ON_LINUX and the x86_64 architecture (e.g.,
import platform and use platform.machine() == "x86_64") or create a helper like
ON_LINUX_64 and use that; modify the decorator on the test (the
pytest.mark.skipif line referencing ON_LINUX) to be pytest.mark.skipif(not
(ON_LINUX and platform.machine() == "x86_64"), reason="This testcase only has a
linux-64 pin file") so the test only runs on linux-64.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0387f860-942f-4b6e-a4eb-c14b505232f3
📒 Files selected for processing (1)
tests/tests_using_conda.py
|
|
||
|
|
||
| @skip_on_windows # the testcase only has a linux-64 pin file | ||
| @pytest.mark.skipif(not ON_LINUX, reason="This testcase only has a linux-64 pin file") |
There was a problem hiding this comment.
Gate this on linux-64, not just Linux.
The reason says the fixture is linux-64 only, but ON_LINUX is also true on Linux/aarch64. That means this test would still run on Linux ARM, where the same pin-file mismatch should make it fail.
Suggested fix
+import platform
+
-@pytest.mark.skipif(not ON_LINUX, reason="This testcase only has a linux-64 pin file")
+@pytest.mark.skipif(
+ not (ON_LINUX and platform.machine() in {"x86_64", "amd64"}),
+ reason="This testcase only has a linux-64 pin file",
+)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/tests_using_conda.py` at line 276, The skip condition currently uses
ON_LINUX which is true for all Linux arches; change it to require linux-64
specifically by updating the pytest.mark.skipif to check both ON_LINUX and the
x86_64 architecture (e.g., import platform and use platform.machine() ==
"x86_64") or create a helper like ON_LINUX_64 and use that; modify the decorator
on the test (the pytest.mark.skipif line referencing ON_LINUX) to be
pytest.mark.skipif(not (ON_LINUX and platform.machine() == "x86_64"),
reason="This testcase only has a linux-64 pin file") so the test only runs on
linux-64.
|
Thanks a lot! |
(snakemake#4049) Update versions for the following tests such that packages are available on macos-arm64. - `test_issue635` - `test_wrapper`, `test_wrapper_qsub`, `test_wrapper_local_git_prefix` Where the test behaviour depends on a specific old version, or where no version of a package is available, skip this test: - `test_script` (Julia is not in standard Conda channels for macOS-arm64; `juliaup` is, but there's no way to pin a specific version.) - `test_conda_python_3_7_script` - `test_script_pre_py39` In one more case, that the test wasn't already skipped seems erroneous: `test_conda_pin_file` had the comment "the testcase only has a linux-64 pin file", but was only skipped on Windows. This is now skipped on any non-Linux OS. ### QC <!-- Make sure that you can tick the boxes below. --> * [X] The PR contains a test case for the changes or the changes are already covered by an existing test case. * [X] The documentation (`docs/`) is updated to reflect the changes or this is not necessary (e.g. if the change does neither modify the language nor the behavior or functionalities of Snakemake). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Added platform checks and a skip marker to skip tests on macOS Apple Silicon where required Conda packages are unavailable. * Replaced a Windows-only gating with a Linux-only condition for a pin-file test. * Applied macOS ARM skips to multiple tests. * Switched the test wrapper to use a different compression tool. * Updated the R test environment to newer r-base and r-rmarkdown versions. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
🤖 I have created a release *beep* *boop* --- ## [9.17.0](v9.16.3...v9.17.0) (2026-03-13) ### Features * Allow storing snakemake metadata in files or databases ([#4012](#4012)) ([dd75f31](dd75f31)) * Allow to specify comparison command per-unit test ([#3956](#3956)) ([b88171c](b88171c)) * job table orderd topological when run is started ([#4018](#4018)) ([75cf506](75cf506)) * lambda functions for priority in rules ([#3253](#3253)) ([d2aa226](d2aa226)) * Make on... directive of modules accessible ([#4050](#4050)) ([e9f2e1c](e9f2e1c)) ### Bug Fixes * adjust conda tests to not fail on apple silicon; fix [#4040](#4040) ([#4049](#4049)) ([f5b0142](f5b0142)) * allow "--containerize apptainer" to output apptainer format instead of dockerfile ([#4030](#4030)) ([f5cac30](f5cac30)) * apptainer command not recognized when singularity is absent ([#4010](#4010)) ([b8162e2](b8162e2)) * capture stderr when tests fail ([#3995](#3995)) ([97d74ba](97d74ba)) * **docs:** make Data-dependent conditional execution a complete example ([#4043](#4043)) ([3a1d7f2](3a1d7f2)) * don't build the DAG when running unlock. Fixes [#4000](#4000) and [#198](#198) ([#4007](#4007)) ([acf79fd](acf79fd)) * Ensure pixi tasks may be run as advertised ([#4046](#4046)) ([88253c2](88253c2)) * fix checkpoint handling corner cases ([#3870](#3870) and [#3559](#3559)) ([#4015](#4015)) ([63f4257](63f4257)) * issue 3642 ([#4054](#4054)) ([76e6fc2](76e6fc2)) * issue 3815 ([#4026](#4026)) ([b0eec96](b0eec96)) * logging None in shellcmd context causes error ([#4064](#4064)) ([d0652cd](d0652cd)) * lookup function returns default value for empty DataFrame queries ([#4056](#4056)) ([f71de97](f71de97)) * make `cache: omit-software` a rule specific property ([#4085](#4085)) ([034a9e7](034a9e7)) * reduce number of tests leaving temporary files behind ([#4033](#4033)) ([a3a1c97](a3a1c97)) * regression in dynamic resource handling ([#4038](#4038)) ([f2c554a](f2c554a)) * somewhat shorter announce message ([#4080](#4080)) ([57efc71](57efc71)) ### Performance Improvements * switch reretry with tenacity; decouple container classes (with Python 3.7 compat for old scripts) from rest of the codebase (enabling moving to newer python versions) ([#4032](#4032)) ([ffb19e7](ffb19e7)) ### Documentation * Add AI-assisted contributions policy to contributing guidelines ([#4051](#4051)) ([dd70526](dd70526)) * **codebase:** Update & simplify plugin architecture section ([#4052](#4052)) ([176cf63](176cf63)) * Correct workflow.source_path() description in documentation ([#4036](#4036)) ([45883c5](45883c5)) * fixed wrong code example for collect() function ([#4037](#4037)) ([5c85ed8](5c85ed8)) * Minor docs improvements ([#4089](#4089)) ([29ea226](29ea226)) * switch to sphinx_design for tabs ([#3976](#3976)) ([9674614](9674614)) * typo in the migration table breaking a pip install command ([#4024](#4024)) ([66f9dda](66f9dda)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Update versions for the following tests such that packages are available on macos-arm64.
test_issue635test_wrapper,test_wrapper_qsub,test_wrapper_local_git_prefixWhere the test behaviour depends on a specific old version, or where no version of a package is available, skip this test:
test_script(Julia is not in standard Conda channels for macOS-arm64;juliaupis, but there's no way to pin a specific version.)test_conda_python_3_7_scripttest_script_pre_py39In one more case, that the test wasn't already skipped seems erroneous:
test_conda_pin_filehad the comment "the testcase only has a linux-64 pin file", but was only skipped on Windows. This is now skipped on any non-Linux OS.QC
docs/) is updated to reflect the changes or this is not necessary (e.g. if the change does neither modify the language nor the behavior or functionalities of Snakemake).Summary by CodeRabbit