Added locators for UI autotests (CORE-13504) #2
Conversation
WalkthroughAdds locator modules, page objects, and a UI constant to establish structured Playwright-based test helpers (Dashboard, Nodes, Settings, Monitoring), expose package re-exports, and replace a hard-coded timeout with a shared constant. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @tests/ui/locators/monitoring_locators.py:
- Around line 7-11: MONITORING_WRAPPER currently uses a Vue-generated attribute
selector ("div[data-v-...]" ) which is brittle; update the MONITORING_WRAPPER
constant in monitoring_locators.py to use a stable selector (e.g., a semantic
class like ".monitoring-wrapper" or a data-testid such as
'div[data-testid="monitoring-wrapper"]'), and then update any tests referencing
MONITORING_WRAPPER to use the new stable selector so locators won’t break when
Vue recompiles scoped styles.
In @tests/ui/locators/settings_locators.py:
- Around line 57-60: The selector in input_by_placeholder is wrong because it
looks for placeholder on the wrapper; update the function to target the input
element's placeholder attribute instead (e.g., return a selector like
"input[placeholder='{placeholder}']" or if you still need to scope to the
wrapper use ".form-field input[placeholder='{placeholder}']"); modify
input_by_placeholder to return the corrected selector.
In @tests/ui/pages/monitoring_page.py:
- Around line 140-152: The apply_filter method currently ignores unknown
filter_type values and then always clicks the filter option, which can cause
confusing failures; update apply_filter to use an explicit mapping (e.g.,
FILTER_MAP = {"node": self.metrics.FILTER_BY_NODE, "type":
self.metrics.FILTER_BY_TYPE, "status": self.metrics.FILTER_BY_STATUS}) or
similar local dict inside the method, look up the locator for the given
filter_type, if not found raise a clear ValueError (or AssertionError)
mentioning the invalid filter_type and allowed keys, then click the resolved
locator and finally click the filter option selector; reference the apply_filter
function and the metrics constants (FILTER_BY_NODE, FILTER_BY_TYPE,
FILTER_BY_STATUS) when making the change.
🧹 Nitpick comments (7)
tests/ui/locators/base_locators.py (1)
34-51: Minor duplication withBaseLocators.SIDEBAR_NAV_ITEM_ACTIVE.
ACTIVE_NAV_ITEM(line 51) uses the same selector asBaseLocators.SIDEBAR_NAV_ITEM_ACTIVE(line 21). Consider referencing the base constant to avoid drift if the selector changes.♻️ Suggested refactor
- # Active navigation item - ACTIVE_NAV_ITEM = ".sidebar-nav-item-active" + # Active navigation item (reference base for consistency) + ACTIVE_NAV_ITEM = BaseLocators.SIDEBAR_NAV_ITEM_ACTIVEtests/ui/pages/dashboard_page.py (1)
57-65: Use explicitstr | Noneinstead of implicitOptional.Per PEP 484, using
= Noneas a default without explicit type annotation can be ambiguous. Usestr | Nonefor clarity.♻️ Suggested fix
@allure.step("Verify form validation error") - def verify_form_error(self, expected_error: str = None): + def verify_form_error(self, expected_error: str | None = None): """Verify form validation error is displayed."""tests/ui/pages/settings_page.py (4)
10-13: Unnecessary instantiation of locator classes.
SettingsPageLocatorsandSettingsFormLocatorscontain only class-level constants and@staticmethodmethods. There's no need to instantiate them—you can reference them directly as class attributes.♻️ Suggested refactor
def __init__(self, page: Page, base_url: str): super().__init__(page, base_url) - self.locators = SettingsPageLocators() - self.form_locators = SettingsFormLocators() + self.locators = SettingsPageLocators + self.form_locators = SettingsFormLocators
87-95: Use explicitOptionalor union type for nullable parameter.Per PEP 484 and the static analysis hint (RUF013), implicit
Optionalvia= Noneis discouraged. Use explicit type annotation.♻️ Suggested fix
-def verify_form_error(self, expected_error: str = None): +def verify_form_error(self, expected_error: str | None = None):
111-118: Use explicitOptionalor union types for nullable parameters.Both
usernameandOptionaltypes. Use explicit type annotations for clarity and PEP 484 compliance.♻️ Suggested fix
@allure.step("Update personal information") -def update_personal_info(self, username: str = None, email: str = None): +def update_personal_info(self, username: str | None = None, email: str | None = None): """Update personal information.""" if username: self.fill_username(username) if email: self.fill_email(email) self.click_save()
126-139: Allure step text is misleading forverify_confirmation_dialog.The
@allure.step("Open confirmation dialog")decorator doesn't match the method's actual behavior, which verifies (not opens) the dialog.♻️ Suggested fix
-@allure.step("Open confirmation dialog") +@allure.step("Verify confirmation dialog visible") def verify_confirmation_dialog(self): """Verify confirmation dialog is displayed.""" self.verify_element_visible(self.form_locators.CONFIRM_DIALOG)tests/ui/pages/monitoring_page.py (1)
14-18: Unnecessary instantiation of locator classes.Same as in
SettingsPage—these locator classes contain only class-level constants and static methods. Reference them directly instead of instantiating.♻️ Suggested refactor
def __init__(self, page: Page, base_url: str): super().__init__(page, base_url) - self.locators = MonitoringPageLocators() - self.commands = MonitoringCommandsLocators() - self.metrics = MonitoringMetricsLocators() + self.locators = MonitoringPageLocators + self.commands = MonitoringCommandsLocators + self.metrics = MonitoringMetricsLocators
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
tests/ui/locators/__init__.pytests/ui/locators/base_locators.pytests/ui/locators/monitoring_locators.pytests/ui/locators/nodes_locators.pytests/ui/locators/settings_locators.pytests/ui/pages/__init__.pytests/ui/pages/dashboard_page.pytests/ui/pages/monitoring_page.pytests/ui/pages/nodes_page.pytests/ui/pages/settings_page.py
🧰 Additional context used
🧬 Code graph analysis (4)
tests/ui/locators/__init__.py (6)
tests/ui/locators/base_locators.py (2)
BaseLocators(4-31)SidebarNavigationLocators(34-51)tests/ui/locators/login_page_locators.py (1)
LoginPageLocators(1-26)tests/ui/locators/dashboard_page_locators.py (1)
DashboardPageLocators(1-48)tests/ui/locators/nodes_locators.py (2)
NodesPageLocators(4-44)NodesListLocators(47-70)tests/ui/locators/settings_locators.py (2)
SettingsPageLocators(4-65)SettingsFormLocators(68-103)tests/ui/locators/monitoring_locators.py (3)
MonitoringPageLocators(4-66)MonitoringCommandsLocators(69-95)MonitoringMetricsLocators(98-134)
tests/ui/pages/dashboard_page.py (2)
tests/ui/pages/base_page.py (8)
BasePage(6-75)navigate(13-16)wait_for_load(19-20)verify_element_visible(66-67)wait_for_element(50-51)fill(58-59)click(54-55)get_text(62-63)tests/ui/locators/dashboard_page_locators.py (1)
DashboardPageLocators(1-48)
tests/ui/pages/settings_page.py (3)
tests/ui/pages/base_page.py (8)
BasePage(6-75)navigate(13-16)wait_for_load(19-20)verify_element_visible(66-67)click(54-55)get_text(62-63)wait_for_element(50-51)fill(58-59)tests/ui/locators/settings_locators.py (4)
SettingsPageLocators(4-65)SettingsFormLocators(68-103)input_by_label(63-65)section_by_title(48-50)fixtures/playwright_fixtures.py (1)
base_url(38-39)
tests/ui/pages/nodes_page.py (2)
tests/ui/pages/base_page.py (7)
BasePage(6-75)navigate(13-16)wait_for_load(19-20)verify_element_visible(66-67)click(54-55)get_text(62-63)fill(58-59)tests/ui/locators/nodes_locators.py (2)
NodesPageLocators(4-44)NodesListLocators(47-70)
🪛 Ruff (0.14.10)
tests/ui/pages/__init__.py
8-16: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
tests/ui/locators/__init__.py
12-26: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
tests/ui/pages/dashboard_page.py
58-58: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
tests/ui/pages/settings_page.py
88-88: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
112-112: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
112-112: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
tests/ui/locators/settings_locators.py
39-39: Possible hardcoded password assigned to: "PASSWORD_INPUT"
(S105)
44-44: Possible hardcoded password assigned to: "CHANGE_PASSWORD_BUTTON"
(S105)
🔇 Additional comments (21)
tests/ui/pages/__init__.py (1)
1-16: LGTM!The package re-exports are well-organized with a logical grouping (base page first, then specific page objects). The
__all__list correctly matches the imports.tests/ui/locators/base_locators.py (1)
4-31: LGTM!Well-structured base locators with clear categorization. The selectors are organized logically by UI area (container, sidebar, navigation, main content, icons).
tests/ui/locators/monitoring_locators.py (3)
34-41: LGTM!The dynamic
step_by_numberhelper and specific step locators are well-designed for flexible element targeting.
69-95: LGTM!Good organization of command-related locators with a useful
command_by_textdynamic helper.
98-134: LGTM!Comprehensive metrics locators covering display elements, charts, time controls, export options, and filters.
tests/ui/locators/settings_locators.py (2)
4-65: LGTM!Well-structured locators with useful dynamic helpers for section and input lookups. The reuse of
.nodes-list-*classes for Settings appears intentional as these pages share UI components.
68-103: LGTM!Comprehensive form locators covering validation states, field groups, action buttons, and modal/confirmation dialogs.
tests/ui/locators/__init__.py (1)
1-26: LGTM!Clean consolidation of all locator classes with logical grouping. The
__all__list correctly matches all imports and the comments help organize by category.tests/ui/pages/dashboard_page.py (2)
7-12: LGTM!Clean page object setup extending BasePage with proper locator initialization.
26-33: LGTM!Well-structured form filling method with appropriate wait for element readiness before interactions.
tests/ui/pages/nodes_page.py (3)
7-56: LGTM!Well-structured NodesPage object with comprehensive methods for page verification and interaction. Good use of Allure step annotations for test reporting.
90-93: Verify search behavior: may need to trigger search action.
search_nodesfills the search input but doesn't trigger the search (e.g., pressing Enter or waiting for auto-search). Depending on the UI behavior, the search may not execute until an additional action is taken.♻️ Potential enhancement if search requires Enter key
@allure.step("Search for node: {search_term}") def search_nodes(self, search_term: str): """Search for nodes.""" self.fill(self.locators.NODES_SEARCH, search_term) + self.page.keyboard.press("Enter") # If search requires Enter to trigger
59-116: LGTM!NodesListPage provides comprehensive list management capabilities including tab navigation, search, node counting, and verification methods.
tests/ui/pages/settings_page.py (3)
15-31: LGTM!The navigation, page load verification, and tab interaction methods follow the established
BasePagepatterns correctly, with proper Allure step decorators and clean delegation to base methods.
39-70: LGTM!Form field interactions and button click methods are well-structured with appropriate wait-for-element calls before filling inputs, ensuring stability in tests.
72-85: LGTM!Value retrieval and section visibility methods correctly use
input_value()for getting field values and delegate visibility checks to the base class.tests/ui/pages/monitoring_page.py (3)
20-36: LGTM!Navigation and page load verification methods follow the established
BasePagepattern with proper Allure step decorators.
38-59: LGTM!Step interaction methods are well-structured, correctly composing selectors using the locator's
step_by_numberhelper method.
92-138: LGTM!Metrics interactions are cleanly implemented with appropriate Allure steps and consistent delegation to base class methods.
tests/ui/locators/nodes_locators.py (2)
4-44: LGTM!Well-organized locator class with clear section comments and logical grouping. The
button_by_textstatic helper method provides good flexibility for dynamic button selection.
47-70: LGTM!Clean and consistent structure for the nodes list locators, following the same patterns as other locator classes in this PR.
| # Page structure | ||
| MONITORING_WRAPPER = "div[data-v-3904b986]" | ||
| MONITORING_HEADER = ".monitoring-header" | ||
| MONITORING_TITLE = ".monitoring-title" | ||
| MONITORING_SUBTITLE = ".monitoring-subtitle" |
There was a problem hiding this comment.
Fragile selector: data-v-* attributes are auto-generated by Vue.
MONITORING_WRAPPER = "div[data-v-3904b986]" uses a Vue scoped CSS hash that will change when the component is recompiled. This makes the locator brittle and likely to break across builds.
Consider using a stable selector such as a semantic class name, data-testid attribute, or another structural selector.
🐛 Suggested fix
# Page structure
- MONITORING_WRAPPER = "div[data-v-3904b986]"
+ MONITORING_WRAPPER = ".monitoring-wrapper" # Add this class to the component or use data-testid🤖 Prompt for AI Agents
In @tests/ui/locators/monitoring_locators.py around lines 7 - 11,
MONITORING_WRAPPER currently uses a Vue-generated attribute selector
("div[data-v-...]" ) which is brittle; update the MONITORING_WRAPPER constant in
monitoring_locators.py to use a stable selector (e.g., a semantic class like
".monitoring-wrapper" or a data-testid such as
'div[data-testid="monitoring-wrapper"]'), and then update any tests referencing
MONITORING_WRAPPER to use the new stable selector so locators won’t break when
Vue recompiles scoped styles.
There was a problem hiding this comment.
All selectors will be revised once we have a stable UI without many fixes.
There was a problem hiding this comment.
@0xc7c5, understood! That makes sense—it's practical to wait until the UI is more stable before investing time in refactoring all the selectors. Feel free to revisit this when you're ready to lock down the locator strategy. 👍
| @staticmethod | ||
| def input_by_placeholder(placeholder: str) -> str: | ||
| """Get input field by placeholder text.""" | ||
| return f".form-field[placeholder='{placeholder}'] input" |
There was a problem hiding this comment.
Incorrect selector: placeholder attribute should be on input, not .form-field.
The selector .form-field[placeholder='{placeholder}'] input looks for a .form-field element with the placeholder attribute. However, placeholder is typically an attribute on <input> elements, not wrapper divs.
🐛 Suggested fix
@staticmethod
def input_by_placeholder(placeholder: str) -> str:
"""Get input field by placeholder text."""
- return f".form-field[placeholder='{placeholder}'] input"
+ return f".form-field input[placeholder='{placeholder}']"🤖 Prompt for AI Agents
In @tests/ui/locators/settings_locators.py around lines 57 - 60, The selector in
input_by_placeholder is wrong because it looks for placeholder on the wrapper;
update the function to target the input element's placeholder attribute instead
(e.g., return a selector like "input[placeholder='{placeholder}']" or if you
still need to scope to the wrapper use ".form-field
input[placeholder='{placeholder}']"); modify input_by_placeholder to return the
corrected selector.
| @staticmethod | ||
| def input_by_placeholder(placeholder: str) -> str: | ||
| """Get input field by placeholder text.""" | ||
| return f".form-field[placeholder='{placeholder}'] input" |
There was a problem hiding this comment.
placeholder attribute belongs to input, not wrapper
seems like this selector will never match anything
| self.click(self.metrics.AUTO_REFRESH_TOGGLE) | ||
|
|
||
| @allure.step("Apply filter: {filter_type} = {filter_value}") | ||
| def apply_filter(self, filter_type: str, filter_value: str): |
There was a problem hiding this comment.
If an invalid filter_type is passed, no filter button is clicked but the method proceeds to select an option - causing confusing test failures
tests/ui/pages/dashboard_page.py
Outdated
| @allure.step("Verify form validation error") | ||
| def verify_form_error(self, expected_error: str = None): | ||
| """Verify form validation error is displayed.""" | ||
| self.wait_for_element(self.locators.error_message, timeout=3000) |
There was a problem hiding this comment.
Hardcoded timeout=3000 here, but timeout=5000 in settings_page.py. Extract timeouts to constants for consistency and easier maintenance.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/ui/pages/login_page.py (1)
78-91: Avoid bareexcept:clause.The bare
except:on line 88 catches all exceptions, includingKeyboardInterruptandSystemExit, which can mask unexpected errors and make debugging difficult. Consider catchingTimeoutError(Playwright's timeout exception) specifically.Proposed fix
try: self.wait_for_element(self.error_message, timeout=TIMEOUT_MAX) error_text = self.get_text(self.error_message) allure.attach(error_text, "Login Error", allure.attachment_type.TEXT) if expected_error: assert expected_error in error_text, f"Expected '{expected_error}' in error, got '{error_text}'" - except: + except TimeoutError: # If no error message appears, verify we're still on login page assert "/login" in self.page.url, "Expected to stay on login page after failed login" allure.attach("No error message displayed, but stayed on login page", "Login Error", allure.attachment_type.TEXT)
🤖 Fix all issues with AI agents
In @tests/ui/pages/monitoring_page.py:
- Around line 92-108: The selectors in verify_metric_visible and
get_metric_value use invalid Playwright syntax (":text(...)") and bypass the
central MonitoringMetricsLocators; update these methods to use Playwright's
:has-text or :has() grammar and reference the locators in
MonitoringMetricsLocators instead of hardcoding strings: replace
".metric-item:has(.metric-label:text('{metric_name}'))" with a valid selector
such as
f"{self.metrics.METRIC_ITEM}:has(.metric-label:has-text('{metric_name}'))" (or
use f"{self.metrics.METRIC_ITEM} >> text={metric_name}" depending on your
Playwright version) in verify_metric_visible and similarly use the metric-value
locator via MonitoringMetricsLocators (e.g., combine self.metrics.METRIC_ITEM
with self.metrics.METRIC_VALUE_TEXT using :has(...) or descendant selector) in
get_metric_value; ensure get_metrics_count continues to use
self.metrics.METRIC_ITEM for count.
- Around line 81-86: The click_external_link method uses a fragile text selector
that breaks on quotes and doesn't handle new tabs; update click_external_link to
use a role-based locator (e.g., locate by role "link" with name=link_text) to
avoid quoting issues, ensure you wait for and capture the new page opened by the
link (target='_blank') before clicking or use a context that listens for new
pages, and return the new Page object so callers can interact with the external
documentation; reference the click_external_link function and the link role
locator when making the change.
In @tests/ui/pages/settings_page.py:
- Around line 127-130: The Allure step description is incorrect for
verify_confirmation_dialog — change the @allure.step string from "Open
confirmation dialog" to something that reflects verification, e.g., "Verify
confirmation dialog is visible", so the decorator on the
verify_confirmation_dialog method matches its behavior which calls
verify_element_visible(self.form_locators.CONFIRM_DIALOG).
🧹 Nitpick comments (5)
tests/ui/constants/ui_constants.py (1)
1-1: Consider renaming for clarity.
TIMEOUT_MAXis misleading since 3000ms is actually shorter than the default timeout of 30000ms used inBasePage.wait_for_element. A name likeTIMEOUT_ERROR_WAITorTIMEOUT_SHORTwould better convey its purpose. Also consider adding a brief comment explaining the intended use case.Suggested improvement
-TIMEOUT_MAX = 3000 +# Shorter timeout (in ms) for waiting on transient UI elements like error messages +TIMEOUT_ERROR_WAIT = 3000tests/ui/pages/settings_page.py (2)
88-96: Use explicitOptionaltype annotation.Per PEP 484, use explicit
Optional[str]orstr | Noneinstead of implicit optional (str = None).Proposed fix
+from typing import Optional + ... @allure.step("Verify form error: {expected_error}") - def verify_form_error(self, expected_error: str = None): + def verify_form_error(self, expected_error: Optional[str] = None): """Verify form validation error."""
112-119: Use explicitOptionaltype annotations.Same PEP 484 issue—both
usernameandOptional[str]orstr | None.Proposed fix
@allure.step("Update personal information") - def update_personal_info(self, username: str = None, email: str = None): + def update_personal_info(self, username: Optional[str] = None, email: Optional[str] = None): """Update personal information."""tests/ui/pages/dashboard_page.py (1)
58-66: Use explicitOptionaltype annotation.Same PEP 484 issue as in
SettingsPage—useOptional[str]orstr | Nonefor theexpected_errorparameter.Proposed fix
+from typing import Optional + ... @allure.step("Verify form validation error") - def verify_form_error(self, expected_error: str = None): + def verify_form_error(self, expected_error: Optional[str] = None): """Verify form validation error is displayed."""tests/ui/pages/monitoring_page.py (1)
38-55: Consider using Locator.locator() chaining for more robust selector composition.Current code concatenates pure CSS selectors (
f"{step_selector} {self.locators.MONITORING_COMMAND_TEXT}"), which works safely given thatstep_by_number()always returns CSS selectors. However, chaining viaself.page.locator().locator()would be more defensive against future changes if selector types ever become mixed or vary (e.g., xpath, text= prefixes).Proposed change (safer locator chaining)
@allure.step("Get command text from step: {step_number}") def get_command_text(self, step_number: int) -> str: """Get command text from a specific step.""" - step_selector = self.locators.step_by_number(step_number) - command_selector = f"{step_selector} {self.locators.MONITORING_COMMAND_TEXT}" - return self.get_text(command_selector) + step_selector = self.locators.step_by_number(step_number) + return ( + self.page + .locator(step_selector) + .locator(self.locators.MONITORING_COMMAND_TEXT) + .inner_text() + )
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
tests/ui/constants/ui_constants.pytests/ui/locators/settings_locators.pytests/ui/pages/dashboard_page.pytests/ui/pages/login_page.pytests/ui/pages/monitoring_page.pytests/ui/pages/settings_page.py
🧰 Additional context used
🧬 Code graph analysis (2)
tests/ui/pages/settings_page.py (3)
tests/ui/pages/base_page.py (8)
BasePage(6-75)navigate(13-16)wait_for_load(19-20)verify_element_visible(66-67)click(54-55)get_text(62-63)wait_for_element(50-51)fill(58-59)tests/ui/locators/settings_locators.py (4)
SettingsPageLocators(4-65)SettingsFormLocators(68-103)input_by_label(63-65)section_by_title(48-50)fixtures/playwright_fixtures.py (1)
base_url(38-39)
tests/ui/pages/login_page.py (1)
tests/ui/pages/base_page.py (1)
wait_for_element(50-51)
🪛 Ruff (0.14.10)
tests/ui/pages/dashboard_page.py
59-59: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
tests/ui/locators/settings_locators.py
39-39: Possible hardcoded password assigned to: "PASSWORD_INPUT"
(S105)
44-44: Possible hardcoded password assigned to: "CHANGE_PASSWORD_BUTTON"
(S105)
tests/ui/pages/settings_page.py
89-89: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
113-113: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
113-113: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
tests/ui/pages/monitoring_page.py
150-150: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: guardrails/scan
🔇 Additional comments (8)
tests/ui/pages/login_page.py (1)
6-6: LGTM on the import.The import of
TIMEOUT_MAXto standardize timeouts is a good improvement for maintainability.tests/ui/locators/settings_locators.py (3)
7-16: Verify CSS class naming consistency.The Settings page locators use
.nodes-list-*class names (e.g.,SETTINGS_WRAPPER = ".nodes-list-page",SETTINGS_TAB = ".nodes-list-tab"). This appears to be either a copy-paste from nodes page locators or the actual DOM reuses these classes. Please verify these selectors match the actual Settings page DOM structure.
47-65: Well-structured dynamic locator helpers.The static helper methods
section_by_title,input_by_placeholder, andinput_by_labelprovide clean reusable patterns for dynamic selector generation.
68-103: LGTM on SettingsFormLocators.Good organization of form-related selectors including validation states, field groups, and dialog elements.
tests/ui/pages/settings_page.py (1)
8-14: LGTM on class setup.Clean constructor with proper locator instantiation and inheritance from
BasePage.tests/ui/pages/dashboard_page.py (2)
8-26: LGTM on DashboardPage core structure.Clean page object setup with proper locator wiring and well-defined navigation/verification methods.
27-44: LGTM on form handling methods.Good approach:
@allure.step("Fill setup form")avoids exposing sensitive data in the step name while the method handles password fields properly.tests/ui/pages/monitoring_page.py (1)
1-37: Nice, clean Page Object surface; navigation + load checks look consistent.
Theopen()/verify_page_loaded()/verify_steps_visible()flow reads well and matches the PR objective of structured helpers.
| @allure.step("Click external link: {link_text}") | ||
| def click_external_link(self, link_text: str): | ||
| """Click an external documentation link.""" | ||
| link_selector = f"a:has-text('{link_text}')" | ||
| self.click(link_selector) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n tests/ui/pages/monitoring_page.py | head -100Repository: chainstack/cp-autotests
Length of output: 4873
🏁 Script executed:
# Check usages of click_external_link to see what link_text values are passed
rg "click_external_link" -B 2 -A 2Repository: chainstack/cp-autotests
Length of output: 446
🏁 Script executed:
# Check if there are any popup handling patterns in the codebase
rg "expect_popup|page.context|newPage" -lRepository: chainstack/cp-autotests
Length of output: 98
🏁 Script executed:
# Check the imports in monitoring_page.py to understand the page/locator API being used
head -30 tests/ui/pages/monitoring_page.pyRepository: chainstack/cp-autotests
Length of output: 1051
🏁 Script executed:
# Search for actual test calls to click_external_link
rg "click_external_link" -B 5 -A 5 tests/Repository: chainstack/cp-autotests
Length of output: 891
🏁 Script executed:
# Check BasePage implementation to understand click() method and API
find . -name "base_page.py" -type f | head -5Repository: chainstack/cp-autotests
Length of output: 96
🏁 Script executed:
# Search for link text patterns that might contain quotes or special chars
rg "docs|link|external" tests/ -i | grep -E "(Grafana|documentation)" | head -20Repository: chainstack/cp-autotests
Length of output: 511
🏁 Script executed:
# Read BasePage to understand click() method
cat -n tests/ui/pages/base_page.py | head -150Repository: chainstack/cp-autotests
Length of output: 3313
🏁 Script executed:
# Search for test files that might call click_external_link
fd "test_.*\.py" tests/ -x rg "click_external_link" {}Repository: chainstack/cp-autotests
Length of output: 49
🏁 Script executed:
# Check monitoring_locators to see GRAFANA_DOCS_LINK and understand the pattern
cat -n tests/ui/locators/monitoring_locators.pyRepository: chainstack/cp-autotests
Length of output: 5483
🏁 Script executed:
# Search more broadly for test functions that might call click_external_link
rg "click_external_link|monitoring_page\." tests/ -l | head -10Repository: chainstack/cp-autotests
Length of output: 100
🏁 Script executed:
# Check if there are test files that actually test external links
find tests/ -name "test_*.py" -o -name "*_test.py" | head -10Repository: chainstack/cp-autotests
Length of output: 89
🏁 Script executed:
# Look for any fixture or test setup that uses monitoring_page
rg "MonitoringPage|monitoring_page" tests/ -B 3 -A 3 | head -60Repository: chainstack/cp-autotests
Length of output: 3153
🏁 Script executed:
# Check if get_by_role is used anywhere in the codebase
rg "get_by_role" tests/Repository: chainstack/cp-autotests
Length of output: 49
🏁 Script executed:
# List all test files to understand what tests exist
find tests/ -name "test_*.py" -type fRepository: chainstack/cp-autotests
Length of output: 89
🏁 Script executed:
# Check the monitoring test file if it exists
ls -la tests/ui/test_*.py 2>/dev/null || echo "No test_*.py files in tests/ui/"Repository: chainstack/cp-autotests
Length of output: 138
🏁 Script executed:
# Search for any test that calls click_external_link or mentions external links
rg "external.*link|docs.*link" tests/ -i -B 2 -A 2Repository: chainstack/cp-autotests
Length of output: 1746
Use role-based locator to handle quotes and improve external link testing.
The a:has-text('{link_text}') selector breaks if link_text contains a single quote (e.g., "Grafana's Docs"). External documentation links open in new tabs (target='_blank' confirmed in locators), which can cause test flakiness if not handled properly.
Suggested fix
@allure.step("Click external link: {link_text}")
def click_external_link(self, link_text: str):
"""Click an external documentation link."""
- link_selector = f"a:has-text('{link_text}')"
- self.click(link_selector)
+ with self.page.expect_popup() as popup_info:
+ self.page.get_by_role("link", name=link_text).click()
+ return popup_info.valueReturns the new page so callers can interact with the external documentation if needed.
🤖 Prompt for AI Agents
In @tests/ui/pages/monitoring_page.py around lines 81 - 86, The
click_external_link method uses a fragile text selector that breaks on quotes
and doesn't handle new tabs; update click_external_link to use a role-based
locator (e.g., locate by role "link" with name=link_text) to avoid quoting
issues, ensure you wait for and capture the new page opened by the link
(target='_blank') before clicking or use a context that listens for new pages,
and return the new Page object so callers can interact with the external
documentation; reference the click_external_link function and the link role
locator when making the change.
| @allure.step("Get metrics count") | ||
| def get_metrics_count(self) -> int: | ||
| """Get the number of metrics displayed.""" | ||
| return self.page.locator(self.metrics.METRIC_ITEM).count() | ||
|
|
||
| @allure.step("Verify metric visible: {metric_name}") | ||
| def verify_metric_visible(self, metric_name: str): | ||
| """Verify a specific metric is visible.""" | ||
| metric_selector = f".metric-item:has(.metric-label:text('{metric_name}'))" | ||
| self.verify_element_visible(metric_selector) | ||
|
|
||
| @allure.step("Get metric value: {metric_name}") | ||
| def get_metric_value(self, metric_name: str) -> str: | ||
| """Get the value of a specific metric.""" | ||
| metric_selector = f".metric-item:has(.metric-label:text('{metric_name}')) .metric-value-text" | ||
| return self.get_text(metric_selector) | ||
|
|
There was a problem hiding this comment.
Metric selectors likely invalid Playwright syntax; also bypasses MonitoringMetricsLocators.
.metric-label:text('{metric_name}') is suspicious Playwright selector syntax and may never match; plus these methods hardcode selectors instead of going through MonitoringMetricsLocators (less maintainable).
Proposed fix (use :has-text; keep CSS-only)
@allure.step("Verify metric visible: {metric_name}")
def verify_metric_visible(self, metric_name: str):
"""Verify a specific metric is visible."""
- metric_selector = f".metric-item:has(.metric-label:text('{metric_name}'))"
+ safe_name = metric_name.replace('"', '\\"')
+ metric_selector = f'.metric-item:has(.metric-label:has-text("{safe_name}"))'
self.verify_element_visible(metric_selector)
@allure.step("Get metric value: {metric_name}")
def get_metric_value(self, metric_name: str) -> str:
"""Get the value of a specific metric."""
- metric_selector = f".metric-item:has(.metric-label:text('{metric_name}')) .metric-value-text"
+ safe_name = metric_name.replace('"', '\\"')
+ metric_selector = f'.metric-item:has(.metric-label:has-text("{safe_name}")) .metric-value-text'
return self.get_text(metric_selector)Please sanity-check the selector grammar against your Playwright version:
Playwright selector syntax: is `.metric-label:text("foo")` valid, or should it be `.metric-label:has-text("foo")` / `text=foo`?
🤖 Prompt for AI Agents
In @tests/ui/pages/monitoring_page.py around lines 92 - 108, The selectors in
verify_metric_visible and get_metric_value use invalid Playwright syntax
(":text(...)") and bypass the central MonitoringMetricsLocators; update these
methods to use Playwright's :has-text or :has() grammar and reference the
locators in MonitoringMetricsLocators instead of hardcoding strings: replace
".metric-item:has(.metric-label:text('{metric_name}'))" with a valid selector
such as
f"{self.metrics.METRIC_ITEM}:has(.metric-label:has-text('{metric_name}'))" (or
use f"{self.metrics.METRIC_ITEM} >> text={metric_name}" depending on your
Playwright version) in verify_metric_visible and similarly use the metric-value
locator via MonitoringMetricsLocators (e.g., combine self.metrics.METRIC_ITEM
with self.metrics.METRIC_VALUE_TEXT using :has(...) or descendant selector) in
get_metric_value; ensure get_metrics_count continues to use
self.metrics.METRIC_ITEM for count.
| @allure.step("Apply filter: {filter_type} = {filter_value}") | ||
| def apply_filter(self, filter_type: str, filter_value: str): | ||
| """Apply a filter to metrics.""" | ||
| if filter_type == "node": | ||
| self.click(self.metrics.FILTER_BY_NODE) | ||
| elif filter_type == "type": | ||
| self.click(self.metrics.FILTER_BY_TYPE) | ||
| elif filter_type == "status": | ||
| self.click(self.metrics.FILTER_BY_STATUS) | ||
| else: | ||
| raise ValueError(f"Invalid filter type: {filter_type}") | ||
|
|
||
| # Select filter value | ||
| filter_option = f"[role='option']:has-text('{filter_value}')" | ||
| self.click(filter_option) |
There was a problem hiding this comment.
apply_filter() could misclick due to unscoped option selector; simplify filter-type dispatch.
[role='option']:has-text(...) is global and can match the wrong dropdown if multiple are present; also the if/elif chain is easy to drift. Consider mapping + role-based option click (and it also avoids quoting issues).
Proposed refactor (mapping + role option)
@allure.step("Apply filter: {filter_type} = {filter_value}")
def apply_filter(self, filter_type: str, filter_value: str):
"""Apply a filter to metrics."""
- if filter_type == "node":
- self.click(self.metrics.FILTER_BY_NODE)
- elif filter_type == "type":
- self.click(self.metrics.FILTER_BY_TYPE)
- elif filter_type == "status":
- self.click(self.metrics.FILTER_BY_STATUS)
- else:
- raise ValueError(f"Invalid filter type: {filter_type}")
-
- # Select filter value
- filter_option = f"[role='option']:has-text('{filter_value}')"
- self.click(filter_option)
+ filter_buttons = {
+ "node": self.metrics.FILTER_BY_NODE,
+ "type": self.metrics.FILTER_BY_TYPE,
+ "status": self.metrics.FILTER_BY_STATUS,
+ }
+ try:
+ self.click(filter_buttons[filter_type])
+ except KeyError:
+ raise ValueError(f"Invalid filter type: {filter_type}") from None
+
+ self.page.get_by_role("option", name=filter_value).click()(Also addresses the Ruff TRY003 complaint pattern by keeping the raise site small; if you want to fully comply, a tiny custom exception type would be the next step.)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @allure.step("Apply filter: {filter_type} = {filter_value}") | |
| def apply_filter(self, filter_type: str, filter_value: str): | |
| """Apply a filter to metrics.""" | |
| if filter_type == "node": | |
| self.click(self.metrics.FILTER_BY_NODE) | |
| elif filter_type == "type": | |
| self.click(self.metrics.FILTER_BY_TYPE) | |
| elif filter_type == "status": | |
| self.click(self.metrics.FILTER_BY_STATUS) | |
| else: | |
| raise ValueError(f"Invalid filter type: {filter_type}") | |
| # Select filter value | |
| filter_option = f"[role='option']:has-text('{filter_value}')" | |
| self.click(filter_option) | |
| @allure.step("Apply filter: {filter_type} = {filter_value}") | |
| def apply_filter(self, filter_type: str, filter_value: str): | |
| """Apply a filter to metrics.""" | |
| filter_buttons = { | |
| "node": self.metrics.FILTER_BY_NODE, | |
| "type": self.metrics.FILTER_BY_TYPE, | |
| "status": self.metrics.FILTER_BY_STATUS, | |
| } | |
| try: | |
| self.click(filter_buttons[filter_type]) | |
| except KeyError: | |
| raise ValueError(f"Invalid filter type: {filter_type}") from None | |
| self.page.get_by_role("option", name=filter_value).click() |
🧰 Tools
🪛 Ruff (0.14.10)
150-150: Avoid specifying long messages outside the exception class
(TRY003)
| @allure.step("Open confirmation dialog") | ||
| def verify_confirmation_dialog(self): | ||
| """Verify confirmation dialog is displayed.""" | ||
| self.verify_element_visible(self.form_locators.CONFIRM_DIALOG) |
There was a problem hiding this comment.
Fix step description mismatch.
The @allure.step says "Open confirmation dialog" but the method is named verify_confirmation_dialog and only verifies visibility. Update the step text to match the method's behavior.
Proposed fix
- @allure.step("Open confirmation dialog")
+ @allure.step("Verify confirmation dialog visible")
def verify_confirmation_dialog(self):
"""Verify confirmation dialog is displayed."""
self.verify_element_visible(self.form_locators.CONFIRM_DIALOG)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @allure.step("Open confirmation dialog") | |
| def verify_confirmation_dialog(self): | |
| """Verify confirmation dialog is displayed.""" | |
| self.verify_element_visible(self.form_locators.CONFIRM_DIALOG) | |
| @allure.step("Verify confirmation dialog visible") | |
| def verify_confirmation_dialog(self): | |
| """Verify confirmation dialog is displayed.""" | |
| self.verify_element_visible(self.form_locators.CONFIRM_DIALOG) |
🤖 Prompt for AI Agents
In @tests/ui/pages/settings_page.py around lines 127 - 130, The Allure step
description is incorrect for verify_confirmation_dialog — change the
@allure.step string from "Open confirmation dialog" to something that reflects
verification, e.g., "Verify confirmation dialog is visible", so the decorator on
the verify_confirmation_dialog method matches its behavior which calls
verify_element_visible(self.form_locators.CONFIRM_DIALOG).
| @staticmethod | ||
| def input_by_placeholder(placeholder: str) -> str: | ||
| """Get input field by placeholder text on parent form-field.""" | ||
| return f".form-field[placeholder='{placeholder}'] .form-input" |
There was a problem hiding this comment.
Seems still incorrect. The placeholder attribute belongs to elements, not to .form-field wrapper. This selector will never match anything.
Am I right that it should be return f"input[placeholder='{placeholder}']"?
There was a problem hiding this comment.
No, because actual input has placeholder=" ". Therefore to get input via placeholder we should address it through parent.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.