Conversation
Two independent gaps, either alone enough to break the option: - galaxy config wrote `test_data_dir`, which is not a Galaxy option at all; the real key is `tool_test_data_directories`, feeding TestDataResolver. The existing TODO admitted as much. - interactor kwds omitted `test_data`, so the client-side fallback in `_find_in_test_data_directories` had nothing to search. galaxy-tool-util's own runner passes it. Integration test covers a tool whose input lives outside its directory, under a name no stock resolver can supply. Fixes galaxyproject#1536. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jmchilton
marked this pull request as ready for review
September 23, 2026 17:42
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1536 —
--test_datawas inert for test input resolution.What was wrong
Two independent gaps, either one alone enough to break the option.
1. The Galaxy config key does not exist.
planemo/galaxy/config.pywroteGalaxy has no
test_data_diroption —grep -rn 'test_data_dir\b' lib/galaxy/config/finds nothing. The real key istool_test_data_directories, read atlib/galaxy/config/__init__.py:1020and wired toTestDataResolveratlib/galaxy/app/__init__.py:988. The TODO was accurate; Galaxy was never going to respect a key it doesn't have.2. The client-side fallback was never given the directory. Test inputs resolve in two stages (
galaxy/tool_util/verify/interactor.py:528-579): firstGET /api/tools/{id}/test_data_download, then a local fallback overself.test_data_directories, populated from atest_datakwarg at:256. galaxy-tool-util's own runner passes it (verify/script.py:426); planemo'sgalaxy_interactor_kwdsdid not.Both are planemo-side. Galaxy and galaxy-tool-util already provide the hooks.
Reproduction
Before, against a tool whose input lives outside its directory:
which is simonbray's report verbatim.
Test
tests/test_cmd_test.py::CmdTestTestCase::test_test_data_option— an integration test, not a unit test: it stands up Galaxy, uploads, and runs the tool.The fixture matters. The input is named
planemo_external_test_data.txtand kept intests/data/external_test_data/, deliberately not atest-data/directory. A stock filename would be served byTestDataResolver's defaultgalaxy-test-data.gitresolver and the test would pass without the fix.Verified red-to-green, and each fix isolated:
interactor.py:579Also re-ran
test_tool_in_directoryandtest_test_index, which exercise the no---test_datapath — both still pass, since an unset option leaves the keyNoneand Galaxy falls back to its defaults.Open question for review
Each fix is independently sufficient, so this is belt-and-braces. Worth deciding whether both belong.
The one behavioural tradeoff is in the config key.
TestDataResolversplits its value on commas and defaults totest-data,https://github.com/galaxyproject/galaxy-test-data.git, so settingtool_test_data_directoriesreplaces those defaults — including the remote resolver. I read an explicit--test_dataas "use this", so overriding seems right, and the tool's own directory is still searched first server-side. But it would change behaviour for someone passing--test_datawhile relying on stock Galaxy test data. Alternatives:f"{test_data_dir},test-data", though that hardcodes part of an upstream defaultHappy to go whichever way. Draft until that's settled.
🤖 Generated with Claude Code