Hani/ Test etp panel correctly displayed blocked trackers detected - #1419
Hani/ Test etp panel correctly displayed blocked trackers detected#1419sv-hyacoub wants to merge 15 commits into
Conversation
|
Overall the test is well-structured and the new BOM/POM additions follow the existing patterns. A few minor issues to address:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Overall the test is well-structured and clear. A few issues worth addressing: Duplicate logic in TrustPanel: detected_category_visible and open_detected_category both compute the same canonical / locator inline. Consider extracting a small private helper (e.g. _category_locator(category)) to keep them DRY. Missing input validation in select_etp_level: Unlike trustpanel_status (which raises ValueError on bad input), select_etp_level will silently attempt to click a non-existent element like etp-foobar-radio and produce an obscure timeout error. A guard like 'if level not in (standard, strict, custom): raise ValueError(...)' would make failures clearer. Test appears incomplete for the second page: The test ends at line 78 with trust_panel.detected_category_visible(tracking cookies) with no further assertions. If additional checks were planned for the tracking page (e.g. confirming tracking content is NOT shown as blocked), they appear to be missing. |
…ked-trackers-detected
PR Review: ETP Panel Trackers Detected TestOverall this is a clean and well-structured test. A few things to address: Code duplication in browser_object_trust_panel.py The new detected_category_visible method duplicates the same canonical transformation and locator construction already in open_detected_category. Both methods perform the identical canonical = category.strip().lower().replace(SPACE_DASH) normalization and build the same locator tuple. This logic should be extracted into a private helper method shared by both. No input validation in select_etp_level select_etp_level constructs the key etp-{level}-radio dynamically with no guard against invalid inputs. Passing an invalid level silently produces a confusing NoSuchElementException rather than a meaningful ValueError. The docstring already documents the three valid values (standard, strict, custom), so a simple validation guard would greatly improve debuggability. Missing return type annotation on detected_category_visible The method returns self but unlike every other method in the class it lacks a return type annotation of BasePage — minor consistency issue. |
…ked-trackers-detected
| from modules.browser_object import TrustPanel | ||
| from modules.browser_object_tabbar import TabBar | ||
| from modules.page_object import GenericPage | ||
| from modules.page_object_prefs import AboutPrefs | ||
|
|
There was a problem hiding this comment.
Inconsistent imports: TabBar and AboutPrefs are imported from their direct modules instead of the aggregate modules. Per project convention (CLAUDE.md), imports should come from modules.browser_object and modules.page_object:
| from modules.browser_object import TrustPanel | |
| from modules.browser_object_tabbar import TabBar | |
| from modules.page_object import GenericPage | |
| from modules.page_object_prefs import AboutPrefs | |
| from modules.browser_object import TrustPanel, TabBar | |
| from modules.page_object import GenericPage, AboutPrefs |
|
|
||
| # Tracking content is not blocked and is shown separately under the section "Firefox allowed these things so sites | ||
| # don't break:", which includes: "Tracking content" | ||
| trust_panel.detected_category_visible("tracking content") |
There was a problem hiding this comment.
The comment says "Tracking content" is shown under the "allowed" section, but detected_category_visible uses the same detected-category selector as open_detected_category. Since the detected-category JSON selector is moz-button.moz-button-subviewbutton-nav[data-l10n-id='{}'], it's worth confirming this selector targets items in both the blocked and allowed sections — otherwise this assertion may silently pass or fail for the wrong reason.
| canonical = category.strip().lower().replace(" ", "-") | ||
| locator = ( | ||
| "detected-category", | ||
| [f"trustpanel-list-label-{canonical}"], | ||
| ) |
There was a problem hiding this comment.
The normalization and locator-building logic here is identical to what's in open_detected_category (lines ~163-167). Consider extracting a private helper to avoid duplication:
def _category_locator(self, category: str):
canonical = category.strip().lower().replace(" ", "-")
return ("detected-category", [f"trustpanel-list-label-{canonical}"])Then both methods call self._category_locator(category).
| def select_etp_level(self, level: str): | ||
| """Clicks the ETP level radio button, accepts 'standard', 'strict' or 'custom'""" | ||
| self.click_on(f"etp-{level}-radio") | ||
| return self |
There was a problem hiding this comment.
No validation on level — an invalid string silently constructs a nonexistent element key (e.g. "etp-typo-radio") and fails with a cryptic element-not-found error. A simple guard helps:
VALID_LEVELS = {"standard", "strict", "custom"}
if level not in VALID_LEVELS:
raise ValueError(f"Invalid ETP level '{level}'; expected one of {VALID_LEVELS}")
Review SummaryGood addition overall - the test is well-structured and the step comments clearly mirror the TestRail case. A few things to address: Bugs / Correctness
Code quality
Convention
|
|
Overall this is a clean, well-structured test addition. A few things to address: Code duplication in Missing input validation in Unrelated SELECTOR_INFO.md changes |
| [f"trustpanel-list-label-{canonical}"], | ||
| ) | ||
|
|
||
| self.element_visible(*locator) |
There was a problem hiding this comment.
The locator construction here is identical to what's already in open_detected_category. Consider extracting a small helper:
def _detected_category_locator(self, category: str):
canonical = category.strip().lower().replace(" ", "-")
return ("detected-category", [f"trustpanel-list-label-{canonical}"])Then both methods can call self._detected_category_locator(category), eliminating the duplication.
| self.click_on("etp-advanced-settings-button") | ||
| return self | ||
|
|
||
| def select_etp_level(self, level: str): |
There was a problem hiding this comment.
No guard against invalid levels. An invalid input will hit the BOM with a nonsense selector and produce an unhelpful NoSuchElementException. Consider:
| def select_etp_level(self, level: str): | |
| def select_etp_level(self, level: str): | |
| """Clicks the ETP level radio button, accepts 'standard', 'strict' or 'custom'""" | |
| valid = {"standard", "strict", "custom"} | |
| if level not in valid: | |
| raise ValueError(f"level must be one of {valid}, got {level!r}") | |
| self.click_on(f"etp-{level}-radio") | |
| return self |
ReviewThe test is well-structured and readable. One critical bug and a few minor issues below. Critical: component key mismatch in
|
|
|
||
| def open_etp_advanced_settings(self): | ||
| """Opens the ETP advanced settings in Privacy & Security preferences""" | ||
| self.click_on("etp-advanced-settings-button") |
There was a problem hiding this comment.
Bug: key "etp-advanced-settings-button" does not exist in about_prefs.components.json.
The file only has "etp-advanced-button". This will fail at runtime. Use the correct key or add a new entry to the JSON manifest.
| self.click_on("etp-advanced-settings-button") | |
| self.click_on("etp-advanced-button") |
|
|
||
| def select_etp_level(self, level: str): | ||
| """Clicks the ETP level radio button, accepts 'standard', 'strict' or 'custom'""" | ||
| self.click_on(f"etp-{level}-radio") |
There was a problem hiding this comment.
Bug: keys like "etp-standard-radio" do not exist in about_prefs.components.json.
The existing keys are "etp-level-standard", "etp-level-strict", "etp-level-custom". Use the correct key pattern or add new JSON entries.
| self.click_on(f"etp-{level}-radio") | |
| self.click_on(f"etp-level-{level}") |
|
|
||
| Canonical input format: hyphenated singular (e.g. "tracking-content") | ||
| """ | ||
| canonical = category.strip().lower().replace(" ", "-") |
There was a problem hiding this comment.
This canonical normalisation + locator construction is identical to open_detected_category (lines 224–227). Consider extracting a private helper:
def _category_locator(self, category: str) -> tuple:
canonical = category.strip().lower().replace(" ", "-")
return ("detected-category", [f"trustpanel-list-label-{canonical}"])Then both methods call self._category_locator(category) instead of duplicating this logic.
…ked-trackers-detected
Review NotesThere are two bugs in the Bug 1: Non-existent element key in
|
|
|
||
| def open_etp_advanced_settings(self): | ||
| """Opens the ETP advanced settings in Privacy & Security preferences""" | ||
| self.click_on("etp-advanced-settings-button") |
There was a problem hiding this comment.
This key does not exist in about_prefs.components.json. The correct existing key is "etp-advanced-button". As written, this will raise an element-not-found error at runtime.
|
|
||
| def select_etp_level(self, level: str): | ||
| """Clicks the ETP level radio button, accepts 'standard', 'strict' or 'custom'""" | ||
| self.click_on(f"etp-{level}-radio") |
There was a problem hiding this comment.
This generates keys like "etp-standard-radio" / "etp-strict-radio" / "etp-custom-radio", none of which exist in about_prefs.components.json. The actual keys are "etp-level-standard", "etp-level-strict", "etp-level-custom". Also, select_etp_level is already defined at line 473 on this same class — Python will silently use this definition and discard the earlier one, breaking all existing callers of that method.
Review SummaryThe test logic and structure look solid. However, the two new helper methods in Bug 1 — Bug 2 — Both will raise an element-not-found error before the test exercises any ETP UI. Minor: The |
|
|
||
| def open_etp_advanced_settings(self): | ||
| """Opens the ETP advanced settings in Privacy & Security preferences""" | ||
| self.click_on("etp-advanced-settings-button") |
There was a problem hiding this comment.
The element key "etp-advanced-settings-button" does not exist in about_prefs.components.json. The correct key is "etp-advanced-button".
| self.click_on("etp-advanced-settings-button") | |
| self.click_on("etp-advanced-button") |
|
|
||
| def select_etp_level(self, level: str): | ||
| """Clicks the ETP level radio button, accepts 'standard', 'strict' or 'custom'""" | ||
| self.click_on(f"etp-{level}-radio") |
There was a problem hiding this comment.
The constructed keys (etp-standard-radio, etp-strict-radio, etp-custom-radio) don't exist in about_prefs.components.json. The JSON entries follow the pattern etp-level-{level}.
| self.click_on(f"etp-{level}-radio") | |
| self.click_on(f"etp-level-{level}") |
| @BasePage.context_chrome | ||
| def detected_category_visible(self, category: str): | ||
| """ | ||
| Verify a detected tracker category is visible in the protections panel. |
There was a problem hiding this comment.
The docstring example shows hyphenated input ("tracking-content"), but the call sites pass space-separated strings ("tracking cookies", "tracking content"). Consider updating the example to match actual usage, e.g. "tracking cookies".
|
|
||
| def open_etp_advanced_settings(self): | ||
| """Opens the ETP advanced settings in Privacy & Security preferences""" | ||
| self.click_on("etp-advanced-settings-button") |
There was a problem hiding this comment.
Bug: the component key "etp-advanced-settings-button" doesn't exist in about_prefs.components.json. The registered key is "etp-advanced-button".
| self.click_on("etp-advanced-settings-button") | |
| self.click_on("etp-advanced-button") |
|
|
||
| def select_etp_level(self, level: str): | ||
| """Clicks the ETP level radio button, accepts 'standard', 'strict' or 'custom'""" | ||
| self.click_on(f"etp-{level}-radio") |
There was a problem hiding this comment.
Bug: f"etp-{level}-radio" generates keys like "etp-standard-radio" which don't exist. The component JSON keys follow the pattern "etp-level-{level}" (e.g. "etp-level-standard", "etp-level-strict", "etp-level-custom").
| self.click_on(f"etp-{level}-radio") | |
| self.click_on(f"etp-level-{level}") |
| from modules.browser_object_tabbar import TabBar | ||
| from modules.page_object import GenericPage | ||
| from modules.page_object_prefs import AboutPrefs | ||
|
|
There was a problem hiding this comment.
Per project convention (CLAUDE.md), imports should come from the aggregate module, not individual files. TabBar is already exported by modules.browser_object.
| from modules.browser_object import TabBar, TrustPanel |
(and remove the TrustPanel import on line 7)
1 participant
Relevant Links
Bugzilla: 2025830
TestRail: 3054905
Description of Code / Doc Changes
Process Changes Required
Mark the relevant boxes, delete irrelevant lines.
pipenv install)./devsetup.sh)Screenshots or Explanations
N/A
Comments or Future Work
N/A
Workflow Checklist
Thank you!