From 4aacb2ca03c4ac1464242d1db13d7dfc022e9152 Mon Sep 17 00:00:00 2001 From: John Chilton Date: Tue, 13 Oct 2026 13:48:52 -0400 Subject: [PATCH 1/2] Tighten workbook import browser test assertions Follow-up to #23922 review: - Compare the template download URL's query values exactly. - Assert split rows (identifiers, forward/reverse URLs, paired identifiers) in the paired and nested workbook tests, via a new rule_builder_column_values helper. - Assert collection workbooks switch the builder to collection mode. Co-Authored-By: Claude Opus 5.5 --- client/src/utils/navigation/navigation.yml | 2 ++ lib/galaxy/selenium/navigates_galaxy.py | 17 +++++++++++ .../selenium/test_workbook_mapping.py | 29 +++++++++++++++++-- 3 files changed, 46 insertions(+), 2 deletions(-) diff --git a/client/src/utils/navigation/navigation.yml b/client/src/utils/navigation/navigation.yml index 2740de2c860..66f97333aee 100644 --- a/client/src/utils/navigation/navigation.yml +++ b/client/src/utils/navigation/navigation.yml @@ -1748,6 +1748,8 @@ rule_builder: view_source: '.rule-builder-view-source' source: '.rule-source' table: '#rules-ag-grid .ag-root' + table_row: '#rules-ag-grid .ag-row' + table_cell: '#rules-ag-grid .ag-row[row-index="${row_index}"] .ag-cell[aria-colindex="${column_number}"]' extension_select: '.rule-footer-extension-group .extension-select' charts: diff --git a/lib/galaxy/selenium/navigates_galaxy.py b/lib/galaxy/selenium/navigates_galaxy.py index 0d524821ad5..f8450d7606a 100644 --- a/lib/galaxy/selenium/navigates_galaxy.py +++ b/lib/galaxy/selenium/navigates_galaxy.py @@ -1235,6 +1235,23 @@ class NavigatesGalaxy(HasDriverProxy[WaitType]): def rule_builder_show_and_get_source_as_json(self) -> dict: return json.loads(self.rule_builder_show_and_get_source()) + def rule_builder_column_values(self, column_index: int) -> list[str]: + """Text of each rendered cell in a rule builder column, in row order. + + The grid only renders cells in view, so this moves focus to the column to + scroll it in. Rows are not scrolled, so this suits small tables. + """ + rule_builder = self.components.rule_builder + # Focus without clicking - a wide first cell's center can lie outside the grid. + first_cell = rule_builder.table_cell(row_index=0, column_number=1).wait_for_visible() + self.execute_script("arguments[0].focus();", first_cell) + self.send_keys_to_page(Keys.ARROW_RIGHT * column_index) + row_count = len(rule_builder.table_row.all()) + return [ + rule_builder.table_cell(row_index=row_index, column_number=column_index + 1).wait_for_visible().text + for row_index in range(row_count) + ] + def rule_builder_enter_source_text(self, json): rule_builder = self.components.rule_builder text_area_elem = rule_builder.source.wait_for_visible() diff --git a/lib/galaxy_test/selenium/test_workbook_mapping.py b/lib/galaxy_test/selenium/test_workbook_mapping.py index d9af576999a..bb4492f5ddc 100644 --- a/lib/galaxy_test/selenium/test_workbook_mapping.py +++ b/lib/galaxy_test/selenium/test_workbook_mapping.py @@ -5,6 +5,11 @@ these tests upload a workbook and assert the mapping it produced rather than building rules by hand. """ +from urllib.parse import ( + parse_qs, + urlparse, +) + from .framework import ( selenium_test, SeleniumTestCase, @@ -30,7 +35,14 @@ class TestWorkbookMapping(SeleniumTestCase, UsesUploadActivity): assert entry["type"] == expected_type, f"Expected mapping {index} to be {expected_type}, got {entry}" assert entry["columns"] == expected_columns, f"Expected mapping {index} on {expected_columns}, got {entry}" + def _file_names(self, column_index: int) -> list[str]: + return [url.rsplit("/", 1)[-1] for url in self.rule_builder_column_values(column_index)] + + def _assert_creating_collections(self) -> None: + self.components.rule_builder.collection_name_input.wait_for_visible() + def _assert_example_1_mapping(self) -> None: + self.components.rule_builder.collection_name_input.assert_absent() mapping = self._mapping(3) self._assert_mapping(mapping, 0, "name", [0]) self._assert_mapping(mapping, 1, "url", [1]) @@ -60,13 +72,15 @@ class TestWorkbookMapping(SeleniumTestCase, UsesUploadActivity): .workbook_for_collection_type("list:paired") ) download_url = rule_import.workbook_download_url() - assert "type=collection" in download_url, download_url - assert "collection_type=list:paired" in download_url.replace("%3A", ":"), download_url + query = parse_qs(urlparse(download_url).query) + assert query["type"] == ["collection"], download_url + assert query["collection_type"] == ["list:paired"], download_url @selenium_test def test_list_collection_mapping_from_workbook(self): """URI and TYPE are recognized as alternate spellings of url and file type.""" self.upload_context("rule").upload_workbook(self._workbook(2)) + self._assert_creating_collections() mapping = self._mapping(3) self._assert_mapping(mapping, 0, "list_identifiers", [0]) @@ -77,6 +91,7 @@ class TestWorkbookMapping(SeleniumTestCase, UsesUploadActivity): def test_list_paired_collection_mapping_from_workbook(self): """Two URL columns per row are split into forward and reverse elements.""" self.upload_context("rule").upload_workbook(self._workbook(3)) + self._assert_creating_collections() mapping = self._mapping(4) self._assert_mapping(mapping, 0, "list_identifiers", [0]) @@ -84,12 +99,22 @@ class TestWorkbookMapping(SeleniumTestCase, UsesUploadActivity): self._assert_mapping(mapping, 2, "dbkey", [2]) self._assert_mapping(mapping, 3, "paired_identifier", [3]) + accessions = ["DRX036147", "DRX036148", "DRX036149"] + assert self.rule_builder_column_values(0) == [a for a in accessions for _ in range(2)] + assert self._file_names(1) == [f"{a}_{i}.fastq.gz" for a in accessions for i in "12"] + assert self.rule_builder_column_values(3) == ["1", "2"] * 3 + @selenium_test def test_nested_list_paired_collection_mapping_from_workbook(self): """Two identifier columns nest, outermost first, around the split pairs.""" self.upload_context("rule").upload_workbook(self._workbook(4)) + self._assert_creating_collections() mapping = self._mapping(3) self._assert_mapping(mapping, 0, "url", [0]) self._assert_mapping(mapping, 1, "list_identifiers", [1, 2]) self._assert_mapping(mapping, 2, "paired_identifier", [3]) + + runs = ["SRR5680996", "SRR5680997", "SRR5681007", "SRR5681006", "SRR5680998", "SRR5681008"] + assert self._file_names(0) == [f"{r}_{i}.fastq.gz" for r in runs for i in "12"] + assert self.rule_builder_column_values(3) == ["1", "2"] * 6 From debd555d9cd8a66efd0f3fe6597f32043ce3d83d Mon Sep 17 00:00:00 2001 From: John Chilton Date: Tue, 13 Oct 2026 13:58:46 -0400 Subject: [PATCH 2/2] Use press() in rule_builder_column_values, check nested identifiers Anchor on any rendered cell and press Home first so columns can be read in any order. Assert outer/inner identifiers in the nested test. Co-Authored-By: Claude Opus 5.5 --- client/src/utils/navigation/navigation.yml | 2 ++ lib/galaxy/selenium/navigates_galaxy.py | 10 ++++------ .../selenium/test_workbook_mapping.py | 19 ++++++++++++++++--- 3 files changed, 22 insertions(+), 9 deletions(-) diff --git a/client/src/utils/navigation/navigation.yml b/client/src/utils/navigation/navigation.yml index 66f97333aee..4e68d06e557 100644 --- a/client/src/utils/navigation/navigation.yml +++ b/client/src/utils/navigation/navigation.yml @@ -1749,6 +1749,8 @@ rule_builder: source: '.rule-source' table: '#rules-ag-grid .ag-root' table_row: '#rules-ag-grid .ag-row' + table_row_cell: '#rules-ag-grid .ag-row[row-index="${row_index}"] .ag-cell' + # row_index is 0-based, column_number is ag-grid's 1-based aria-colindex table_cell: '#rules-ag-grid .ag-row[row-index="${row_index}"] .ag-cell[aria-colindex="${column_number}"]' extension_select: '.rule-footer-extension-group .extension-select' diff --git a/lib/galaxy/selenium/navigates_galaxy.py b/lib/galaxy/selenium/navigates_galaxy.py index f8450d7606a..ea87fe0d799 100644 --- a/lib/galaxy/selenium/navigates_galaxy.py +++ b/lib/galaxy/selenium/navigates_galaxy.py @@ -1238,14 +1238,12 @@ class NavigatesGalaxy(HasDriverProxy[WaitType]): def rule_builder_column_values(self, column_index: int) -> list[str]: """Text of each rendered cell in a rule builder column, in row order. - The grid only renders cells in view, so this moves focus to the column to - scroll it in. Rows are not scrolled, so this suits small tables. + The grid only renders cells in view, so this moves keyboard focus to the + column to scroll it in. Rows are not scrolled, so this suits small tables. """ rule_builder = self.components.rule_builder - # Focus without clicking - a wide first cell's center can lie outside the grid. - first_cell = rule_builder.table_cell(row_index=0, column_number=1).wait_for_visible() - self.execute_script("arguments[0].focus();", first_cell) - self.send_keys_to_page(Keys.ARROW_RIGHT * column_index) + rendered_cell = rule_builder.table_row_cell(row_index=0).wait_for_visible() + self.press(Key.HOME, *[Key.ARROW_RIGHT] * column_index, element=rendered_cell) row_count = len(rule_builder.table_row.all()) return [ rule_builder.table_cell(row_index=row_index, column_number=column_index + 1).wait_for_visible().text diff --git a/lib/galaxy_test/selenium/test_workbook_mapping.py b/lib/galaxy_test/selenium/test_workbook_mapping.py index bb4492f5ddc..14aed9391b0 100644 --- a/lib/galaxy_test/selenium/test_workbook_mapping.py +++ b/lib/galaxy_test/selenium/test_workbook_mapping.py @@ -38,11 +38,13 @@ class TestWorkbookMapping(SeleniumTestCase, UsesUploadActivity): def _file_names(self, column_index: int) -> list[str]: return [url.rsplit("/", 1)[-1] for url in self.rule_builder_column_values(column_index)] + def _assert_creating_datasets(self) -> None: + self.components.rule_builder.collection_name_input.assert_absent() + def _assert_creating_collections(self) -> None: self.components.rule_builder.collection_name_input.wait_for_visible() def _assert_example_1_mapping(self) -> None: - self.components.rule_builder.collection_name_input.assert_absent() mapping = self._mapping(3) self._assert_mapping(mapping, 0, "name", [0]) self._assert_mapping(mapping, 1, "url", [1]) @@ -52,6 +54,7 @@ class TestWorkbookMapping(SeleniumTestCase, UsesUploadActivity): def test_dataset_mapping_from_workbook(self): """Dataset name, URL and genome columns are recognized by their headers.""" self.upload_context("rule").upload_workbook(self._workbook(1)) + self._assert_creating_datasets() self._assert_example_1_mapping() @selenium_test @@ -60,6 +63,7 @@ class TestWorkbookMapping(SeleniumTestCase, UsesUploadActivity): rule_import = self.upload_context("rule").creating("datasets").from_source("workbook") assert "type=datasets" in rule_import.workbook_download_url() rule_import.upload_workbook_from_card(self._workbook(1)) + self._assert_creating_datasets() self._assert_example_1_mapping() @selenium_test @@ -115,6 +119,15 @@ class TestWorkbookMapping(SeleniumTestCase, UsesUploadActivity): self._assert_mapping(mapping, 1, "list_identifiers", [1, 2]) self._assert_mapping(mapping, 2, "paired_identifier", [3]) - runs = ["SRR5680996", "SRR5680997", "SRR5681007", "SRR5681006", "SRR5680998", "SRR5681008"] - assert self._file_names(0) == [f"{r}_{i}.fastq.gz" for r in runs for i in "12"] + rows = [ + ("SRR5680996", "H3K4me3", "rep1"), + ("SRR5680997", "H3K27me3", "rep2"), + ("SRR5681007", "H3K27me3", "rep1"), + ("SRR5681006", "H3K4me3", "rep2"), + ("SRR5680998", "CTCF", "rep1"), + ("SRR5681008", "CTCF", "rep2"), + ] + assert self._file_names(0) == [f"{run}_{i}.fastq.gz" for run, _, _ in rows for i in "12"] + assert self.rule_builder_column_values(1) == [outer for _, outer, _ in rows for _ in range(2)] + assert self.rule_builder_column_values(2) == [inner for _, _, inner in rows for _ in range(2)] assert self.rule_builder_column_values(3) == ["1", "2"] * 6