LazyToolBox: add test coverage for round-2 fixes

Regular CI does not set use_lazy_toolbox=true, so the bug surfaces
fixed in 215638d..912544 are not exercised on push/PR runs unless a
test class boots Galaxy with the flag itself.

- Add unit tests under test/unit/tool_source_store/ pinning ToolIndex
  multi-version selection (packaging.version sort), explicit-version
  lookup, to_dict/from_dict round-trip including legacy backfill, and
  tokenised search across id/name/description/labels.
- Add a single TestLazyToolBoxApi integration class that boots Galaxy
  once with use_lazy_toolbox=true and asserts the user-visible
  behaviours the round-2 fixes deliver: <tool_dir> + YAML +
  ${model_tools_path} bootstrap, multi-version show/run, default-panel
  shape, panel-views fallback, multi-token search, remove_tool_by_id
  lifecycle, and admin container-resolver.
- Lift tool_source_store config into BaseToolSourceStorageIntegrationTestCase
  via STORE_KIND so subclasses share the wiring.

Author: mvdbeek

test/integration/test_tool_source_storage.py

@@ -10,13 +10,20 @@
 import os
 import tempfile
 
+from galaxy_test.base.populators import DatasetPopulator
 from galaxy_test.driver import integration_util
 
 
 class BaseToolSourceStorageIntegrationTestCase(integration_util.IntegrationTestCase):
     """Base class for tool source storage integration tests."""
 
     framework_tool_and_types = True
+    STORE_KIND: str = "database"
+
+    @classmethod
+    def handle_galaxy_config_kwds(cls, config):
+        super().handle_galaxy_config_kwds(config)
+        config["tool_source_store"] = cls.STORE_KIND
 
     def _test_api_tools_list(self):
         response = self._get("tools")
@@ -34,11 +41,6 @@ def _test_api_tools_show(self, tool_id: str = "cat1"):
 class TestDatabaseToolSourceStorage(BaseToolSourceStorageIntegrationTestCase):
     """Integration tests with database tool source storage backend."""
 
-    @classmethod
-    def handle_galaxy_config_kwds(cls, config):
-        super().handle_galaxy_config_kwds(config)
-        config["tool_source_store"] = "database"
-
     def test_api_tools_list(self):
         self._test_api_tools_list()
 
@@ -127,3 +129,169 @@ def test_api_tools_list_populated_via_bootstrap(self):
                 f"Bootstrap silently dropped {required!r} from the index "
                 f"(have {len(tool_ids)} ids: {sorted(tool_ids)[:10]}…)"
             )
+
+
+class TestLazyToolBoxApi(BaseToolSourceStorageIntegrationTestCase):
+    """End-to-end coverage of LazyToolBox-served API behaviours.
+
+    Regular CI does not run with ``use_lazy_toolbox=true``, so the bug
+    surfaces fixed in commits 215638d..912544 are not covered by any
+    push/PR run unless this class boots Galaxy with the flag itself.
+    Every behaviour the round-2 fixes were meant to deliver is asserted
+    here as a single API call against one shared boot:
+
+    - ``<tool_dir>``, YAML, and ``${model_tools_path}`` bootstrap paths.
+    - Multi-version index + version-aware ``/api/tools/{id}`` lookup.
+    - Default panel-view response shape consumed by the UI.
+    - Tokenised tool search across name + description.
+    - ``remove_tool_by_id`` lifecycle on the live toolbox.
+    - Container-resolver admin endpoint (sensitive to placeholder
+      ``None`` Tool entries in ``_LazyToolsByIdView``).
+
+    All methods share one boot via the class-scoped ``setUpClass`` —
+    don't add tests that mutate global state in ways that would leak
+    into sibling methods (besides ``test_remove_tool_makes_get_tool_return_none``,
+    which deliberately removes a tool that no other method touches).
+    """
+
+    dataset_populator: DatasetPopulator
+
+    @classmethod
+    def handle_galaxy_config_kwds(cls, config):
+        super().handle_galaxy_config_kwds(config)
+        config["use_lazy_toolbox"] = True
+
+    def setUp(self):
+        super().setUp()
+        self.dataset_populator = DatasetPopulator(self.galaxy_interactor)
+
+    # --- Bootstrap correctness ----------------------------------------------
+
+    def test_tool_dir_directive_indexes_parameters_tools(self):
+        # ``gx_int`` lives under ``test/functional/tools/parameters/`` and
+        # gets pulled in by ``<tool_dir dir="parameters/" />``. The bootstrap
+        # used to silently drop these because the discovery walker didn't
+        # honour the directive.
+        response = self._get("tools/gx_int")
+        self._assert_status_code_is(response, 200)
+        assert response.json()["id"] == "gx_int"
+
+    def test_yaml_user_defined_tool_indexed_under_yaml_id(self):
+        # YAML tool's id comes from the body's ``id:`` field, not the
+        # filename — the previous bootstrap walked ``xml_tree`` and dropped
+        # every YAML source.
+        response = self._get("tools/cat_user_defined")
+        self._assert_status_code_is(response, 200)
+        assert response.json()["id"] == "cat_user_defined"
+
+    def test_model_tools_path_template_substitution(self):
+        # ``${model_tools_path}/build_list.xml`` resolves to
+        # ``lib/galaxy/tools/build_list.xml`` (id ``__BUILD_LIST__``).
+        # Exercises ``_resolve_file_template_kwds`` for the
+        # ``model_tools_path`` substitution.
+        response = self._get("tools/__BUILD_LIST__")
+        self._assert_status_code_is(response, 200)
+        assert response.json()["id"] == "__BUILD_LIST__"
+
+    # --- Multi-version + version-aware lookup -------------------------------
+
+    def test_show_unknown_version_falls_back_to_latest(self):
+        response = self._get("tools/multiple_versions", data={"tool_version": "0.01"})
+        self._assert_status_code_is(response, 200)
+        # Default selection uses ``packaging.version.parse``; lex sort would
+        # have picked ``"0.1+galaxy6"`` over ``"0.2"`` for a different prefix
+        # so the regression matters even though it's invisible in this case.
+        assert response.json()["version"] == "0.2"
+
+    def test_show_lists_every_indexed_version(self):
+        response = self._get("tools/multiple_versions_hidden", data={"tool_version": "0.1"})
+        self._assert_status_code_is(response, 200)
+        info = response.json()
+        assert info["version"] == "0.1"
+        assert info["versions"] == ["0.1", "0.2"]
+        assert info["hidden_versions"] == ["0.1"]
+
+    def test_run_specific_version_executes_that_version(self):
+        with self.dataset_populator.test_history() as history_id:
+            payload = self.dataset_populator.run_tool_payload(
+                tool_id="multiple_versions_hidden",
+                inputs={},
+                history_id=history_id,
+            )
+            payload["tool_version"] = "0.1"
+            response = self.dataset_populator._post("tools", data=payload)
+            self._assert_status_code_is(response, 200)
+            output = response.json()["outputs"][0]
+            self.dataset_populator.wait_for_history(history_id, assert_ok=True)
+            content = self.dataset_populator.get_history_dataset_content(history_id, dataset=output)
+            assert content.strip() == "Hidden Version 0.1"
+
+    # --- Default-panel response shape ---------------------------------------
+
+    def test_default_panel_view_returns_root_level_tools_and_section_tools_key(self):
+        # One call asserts both the root-level dedup fix (``upload1`` survives
+        # at top level) and the section-shape fix (sections use ``tools`` key
+        # with id list, mirroring ``ToolSection.to_dict(only_ids=True)``).
+        response = self._get("tool_panels/default")
+        self._assert_status_code_is(response, 200)
+        panel = response.json()
+
+        assert "upload1" in panel, (
+            "upload1 should appear at the top level of the default panel; the "
+            "previous bug stripped root-level tools that also appeared inside "
+            "a section."
+        )
+
+        for entry_id, entry in panel.items():
+            if isinstance(entry, dict) and entry.get("model_class") == "ToolSection":
+                assert "tools" in entry, f"section {entry_id} missing 'tools' key"
+                assert all(
+                    isinstance(t, str) for t in entry["tools"]
+                ), f"section {entry_id} should hold tool ids as strings, got {entry['tools'][:3]}"
+
+    def test_panel_views_endpoint_returns_views(self):
+        # ``GET /api/tool_panels`` used to return ``views={}`` when the lazy
+        # index hadn't pre-computed panel_views. The fallback to
+        # ``toolbox.panel_view_dicts()`` keeps callers working.
+        response = self._get("tool_panels")
+        self._assert_status_code_is(response, 200)
+        body = response.json()
+        assert "views" in body and "default_panel_view" in body
+        assert body["views"], "expected at least one panel view to be registered"
+
+    # --- Search -------------------------------------------------------------
+
+    def test_search_finds_tool_by_multi_token_query_across_fields(self):
+        # ``cat1`` (for_workflows/catWrapper.xml) has name "Concatenate
+        # multiple datasets or collections". A query whose tokens span
+        # "Concatenate" + "datasets" forces the tokenised conjunction
+        # path; the previous OR-within-single-field implementation
+        # returned empty here.
+        response = self._get("tools", data={"q": "Concatenate multiple datasets"})
+        self._assert_status_code_is(response, 200)
+        assert "cat1" in response.json()
+
+    # --- Removal lifecycle --------------------------------------------------
+
+    def test_remove_tool_makes_get_tool_return_none(self):
+        # ``remove_tool_by_id`` had to clear ``_tool_index.entries`` /
+        # ``entries_by_version`` / LRU + populate ``_tools_by_old_id``;
+        # without that fix the call raised KeyError. We pick
+        # ``cat_data_and_sleep`` because nothing else in this class
+        # references it, so we can mutate the live toolbox without
+        # breaking sibling tests.
+        toolbox = self._app.toolbox
+        assert toolbox.get_tool("cat_data_and_sleep") is not None
+        toolbox.remove_tool_by_id("cat_data_and_sleep")
+        assert toolbox.get_tool("cat_data_and_sleep") is None
+
+    # --- Container resolution -----------------------------------------------
+
+    def test_container_resolvers_resolve_tool(self):
+        # Admin-only endpoint. Used to fail with
+        # ``'NoneType' object has no attribute 'tool_requirements'`` when
+        # ``_LazyToolsByIdView`` returned a ``None`` placeholder for an
+        # un-materialised tool — fixed in c763b03 by populating
+        # ``_tools_by_old_id`` and exposing a real ``.copy()``.
+        response = self._get("container_resolvers/resolve", data={"tool_id": "cat1"}, admin=True)
+        self._assert_status_code_is(response, 200)

test/unit/tool_source_store/__init__.py

@@ -0,0 +1,27 @@
+"""Shared fixtures for tool_source_store unit tests."""
+
+import pytest
+
+from galaxy.tool_source_store.index import (
+    ToolIndex,
+    ToolIndexEntry,
+)
+
+
+@pytest.fixture
+def index_entry():
+    def _make(id, version=None, **kwargs):
+        return ToolIndexEntry(id=id, version=version, **kwargs)
+
+    return _make
+
+
+@pytest.fixture
+def tool_index(index_entry):
+    def _make(*entries):
+        index = ToolIndex()
+        for entry in entries:
+            index.add_entry(entry)
+        return index
+
+    return _make

test/unit/tool_source_store/conftest.py

@@ -0,0 +1,155 @@
+"""Pin ``ToolIndex`` behaviours that drive the LazyToolBox API responses.
+
+These unit tests run against a real ``ToolIndex`` (no mocks, no Galaxy
+app) so they're cheap and pinpoint regressions in the round-2
+LazyToolBox fixes:
+
+- per-id default selection by ``packaging.version.parse``
+- ``entries_by_version`` records every indexed version
+- explicit-version lookup via ``ToolIndex.get(tool_id, tool_version)``
+- ``to_dict`` / ``from_dict`` round-trips ``entries_by_version``
+- tokenised search across id / name / description / labels
+"""
+
+from galaxy.tool_source_store.index import ToolIndex
+
+
+def test_add_entry_default_picks_highest_packaging_version(index_entry, tool_index):
+    # ``"0.1+galaxy6"`` lex-compares between ``"0.1"`` and ``"0.2"``, so a
+    # plain string sort would still pick ``"0.2"`` here — but it would pick
+    # ``"0.1+galaxy6"`` for ``["0.1+galaxy6", "0.10"]``. Cover the
+    # packaging-aware path explicitly.
+    index = tool_index(
+        index_entry("multi", version="0.1+galaxy6"),
+        index_entry("multi", version="0.1"),
+        index_entry("multi", version="0.2"),
+    )
+    assert index.entries["multi"].version == "0.2"
+
+
+def test_add_entry_default_handles_post_release_above_two_digits(index_entry, tool_index):
+    index = tool_index(
+        index_entry("multi", version="0.10"),
+        index_entry("multi", version="0.2"),
+    )
+    assert index.entries["multi"].version == "0.10"
+
+
+def test_add_entry_records_every_version_under_entries_by_version(index_entry, tool_index):
+    index = tool_index(
+        index_entry("multi", version="0.1"),
+        index_entry("multi", version="0.2"),
+        index_entry("multi", version="0.3"),
+    )
+    assert set(index.entries_by_version["multi"]) == {"0.1", "0.2", "0.3"}
+
+
+def test_get_with_explicit_version_returns_that_entry(index_entry, tool_index):
+    e1 = index_entry("multi", version="0.1", name="V01")
+    e2 = index_entry("multi", version="0.2", name="V02")
+    index = tool_index(e1, e2)
+    assert index.get("multi", tool_version="0.1") is e1
+    assert index.get("multi", tool_version="0.2") is e2
+
+
+def test_get_with_unknown_version_returns_none(index_entry, tool_index):
+    # ``LazyToolBox.get_tool`` uses the ``None`` return to fall back to the
+    # default-version entry; pin the contract so a future refactor doesn't
+    # silently start returning the default here.
+    index = tool_index(index_entry("multi", version="0.1"))
+    assert index.get("multi", tool_version="9.9") is None
+
+
+def test_get_with_no_version_returns_default_entry(index_entry, tool_index):
+    e1 = index_entry("multi", version="0.1")
+    e2 = index_entry("multi", version="0.2")
+    index = tool_index(e1, e2)
+    assert index.get("multi") is e2
+
+
+def test_to_dict_from_dict_round_trips_entries_by_version(index_entry, tool_index):
+    index = tool_index(
+        index_entry("multi", version="0.1"),
+        index_entry("multi", version="0.2"),
+        index_entry("solo", version="1.0"),
+    )
+    restored = ToolIndex.from_dict(index.to_dict())
+    assert set(restored.entries_by_version["multi"]) == {"0.1", "0.2"}
+    assert restored.entries["multi"].version == "0.2"
+    multi_v01 = restored.get("multi", tool_version="0.1")
+    assert multi_v01 is not None and multi_v01.version == "0.1"
+    assert set(restored.entries_by_version["solo"]) == {"1.0"}
+
+
+def test_from_dict_backfills_entries_by_version_for_legacy_indexes():
+    # Indexes serialised before ``entries_by_version`` existed should still
+    # load — ``ToolIndex.get(tool_id, tool_version=...)`` relies on the
+    # backfill so legacy stores keep working after upgrade.
+    legacy = {
+        "entries": {
+            "tool1": {"id": "tool1", "version": "1.0", "name": "Tool 1"},
+        },
+    }
+    restored = ToolIndex.from_dict(legacy)
+    tool1 = restored.get("tool1", tool_version="1.0")
+    assert tool1 is not None and tool1.version == "1.0"
+
+
+def test_search_tokenised_conjunction_across_fields(index_entry, tool_index):
+    # The user-visible regression: a multi-token query whose tokens spread
+    # across name + description fields used to return 0 hits because the
+    # eager Whoosh impl would AND across fields but the lazy impl only
+    # OR'd within a single field.
+    index = tool_index(
+        index_entry(
+            "Grep1",
+            version="1.0",
+            name="Select",
+            description="Select lines that match an expression",
+        ),
+        index_entry(
+            "filter",
+            version="1.0",
+            name="Filter",
+            description="Filter by column",
+        ),
+    )
+    results = index.search("Select lines that match an expression", limit=10)
+    assert [r.id for r in results] == ["Grep1"]
+
+
+def test_search_label_match_returns_entry(index_entry, tool_index):
+    index = tool_index(
+        index_entry("tool1", version="1.0", name="Tool 1", labels=["genomics"]),
+        index_entry("tool2", version="1.0", name="Tool 2", labels=["text"]),
+    )
+    results = index.search("genomics", limit=10)
+    assert [r.id for r in results] == ["tool1"]
+
+
+def test_search_full_phrase_in_description_outranks_partial(index_entry, tool_index):
+    # Both entries hit every token, but only ``hit_full`` contains the full
+    # phrase — the ``query_lower in desc_l`` bonus should rank it first.
+    index = tool_index(
+        index_entry(
+            "hit_partial",
+            version="1.0",
+            description="alpha and beta separately",
+        ),
+        index_entry(
+            "hit_full",
+            version="1.0",
+            description="alpha beta gamma in one phrase",
+        ),
+    )
+    results = index.search("alpha beta", limit=10)
+    assert [r.id for r in results][0] == "hit_full"
+
+
+def test_search_skips_entries_missing_a_token(index_entry, tool_index):
+    index = tool_index(
+        index_entry("a", version="1.0", name="alpha bravo"),
+        index_entry("b", version="1.0", name="alpha"),
+    )
+    results = index.search("alpha bravo", limit=10)
+    assert [r.id for r in results] == ["a"]