Skip to content

[chores] Made SeleniumTestMixin to use its own find_element method#719

Merged
nemesifier merged 2 commits into
masterfrom
fix-selenium-test-mixin
Jul 14, 2026
Merged

[chores] Made SeleniumTestMixin to use its own find_element method#719
nemesifier merged 2 commits into
masterfrom
fix-selenium-test-mixin

Conversation

@pandafy

@pandafy pandafy commented Jul 14, 2026

Copy link
Copy Markdown
Member

Checklist

  • I have read the OpenWISP Contributing Guidelines.
  • I have manually tested the changes proposed in this pull request.
  • N/A I have written new test cases for new code and/or updated existing tests for changes to existing code.
  • N/A I have updated the documentation.

Description of Changes

Instead of using driver.find_element, the public methods of SeleniumTestMixin shall use self.find_element and pass the driver instance.

Instead of using driver.find_element, the public methods of
SeleniumTestMixin shall use `self.find_element` and pass the driver
instance.
@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

SeleniumTestMixin.login() and logout() now use the mixin’s find_element() helper with explicit selectors and the supplied driver. Internal waits in find_element() and find_elements() use that same driver. The openwisp_controller test dependency URL was updated to the archive/refs/heads/master.tar.gz format.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested labels: enhancement

Suggested reviewers: nemesifier


Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

  • Ignore

❌ Failed checks (3 errors, 2 warnings)

Check name Status Explanation Resolution
Bug Fixes ❌ Error The fix changes SeleniumTestMixin internals, but the PR adds no regression test; existing selenium tests call login/logout without a custom driver, so they don't catch the bug. Add a deterministic Selenium regression test that passes a non-default driver into login/logout/find_element and fails if driver routing is removed.
General Rules ❌ Error No regression test covers the new explicit-driver path in SeleniumTestMixin; the login/logout/find_element driver-threading changes aren’t exercised. Add a regression test using a non-default/spied driver and assert login/logout/find_element route waits and lookups through that driver.
Description check ❌ Error The description covers the checklist and change summary, but it omits the required issue reference and screenshot section. Add the Reference to Existing Issue section with Closes # and include a Screenshot section or mark it not applicable.
Changes ⚠️ Warning The change is non-trivial, but the PR only edits selenium.py; no tests cover the new driver-propagation path, and docs weren’t updated for the changed helper behavior. Add or update tests that pass a non-default driver and verify waits/find_element use it; update docs if this behavior is intended to be part of the public contract.
Title check ⚠️ Warning The title describes the SeleniumTestMixin change, but it uses an unsupported [tests] prefix instead of an allowed type. Change the prefix to an allowed type such as [fix] and keep the title focused on the SeleniumTestMixin behavior change.
✅ Passed checks (3 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Features ✅ Passed This PR is a Selenium test-utility bugfix, not a feature request, so the feature-request checklist (issue/docs/screenshots) is not applicable.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-selenium-test-mixin

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@openwisp_utils/tests/selenium.py`:
- Around line 370-378: Update the find_element calls in the login flow to ensure
the supplied driver is forwarded through the wait_for_visibility path, including
the username, password, and submit-element lookups. Keep the final element
lookup and visibility wait operating on the same driver instance rather than
falling back to self.web_driver.
- Around line 370-378: Add an in-process regression test alongside the existing
Selenium tests that performs login and logout with a distinct custom driver,
covering the username, password, submit, and logout interactions. Verify the
helper’s waits and element lookups use the supplied driver rather than the
default driver, while preserving the existing default-driver coverage.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: be3ec4e2-96ab-491b-9227-b8bf329b0a0a

📥 Commits

Reviewing files that changed from the base of the PR and between 9f3639a and 64cd554.

📒 Files selected for processing (2)
  • openwisp_utils/tests/selenium.py
  • requirements-test.txt
📜 Review details
⏰ Context from checks skipped due to timeout. (19)
  • GitHub Check: CodeQL
  • GitHub Check: Python==3.13 | django~=5.2.0
  • GitHub Check: Python==3.10 | django~=5.0.0
  • GitHub Check: Python==3.12 | django~=4.2.0
  • GitHub Check: Python==3.12 | django~=5.0.0
  • GitHub Check: Python==3.11 | django~=5.0.0
  • GitHub Check: Python==3.10 | django~=5.2.0
  • GitHub Check: Python==3.13 | django~=5.1.0
  • GitHub Check: Python==3.10 | django~=5.1.0
  • GitHub Check: Python==3.11 | django~=4.2.0
  • GitHub Check: Python==3.10 | django~=4.2.0
  • GitHub Check: Python==3.12 | django~=5.1.0
  • GitHub Check: Python==3.11 | django~=5.1.0
  • GitHub Check: Python==3.11 | django~=5.2.0
  • GitHub Check: Python==3.12 | django~=5.2.0
  • GitHub Check: Analyze (javascript-typescript)
  • GitHub Check: Analyze (actions)
  • GitHub Check: Analyze (python)
  • GitHub Check: Kilo Code Review
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{py,html,txt}

📄 CodeRabbit inference engine (Custom checks)

For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework

Files:

  • requirements-test.txt
  • openwisp_utils/tests/selenium.py
**/*.{md,rst,txt}

📄 CodeRabbit inference engine (AGENTS.md)

Update documentation when behavior, settings, public APIs, setup steps, QA rules, or supported versions change

Files:

  • requirements-test.txt
**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/*.py: Place imports at the top of the file; only defer imports when necessary (e.g., Django model imports inside functions or methods where the app registry is not yet ready)
Avoid unnecessary blank lines inside function and method bodies
Add or update tests for every behavior change
Run openwisp-qa-format after editing
Prefer in-process tests so coverage tools can measure changed code
When checking coverage for a changed module, use python -m pytest <test_path> --cov=<dotted.module.path> --cov-report=term-missing
Watch for unsafe file paths, unsafe subprocess usage, token or secret exposure, and changes that could weaken QA or release safeguards
Write comments and docstrings only when they explain why code is shaped a certain way; place comments before the relevant code block instead of scattering them inside it

Files:

  • openwisp_utils/tests/selenium.py
**/tests/**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

For bug fixes, write the regression test first, run it against the unfixed code, confirm it fails for the expected reason, then implement the fix

Files:

  • openwisp_utils/tests/selenium.py
🧠 Learnings (1)
📚 Learning: 2026-06-15T22:45:01.529Z
Learnt from: nemesifier
Repo: openwisp/openwisp-utils PR: 701
File: setup.py:72-72
Timestamp: 2026-06-15T22:45:01.529Z
Learning: When reviewing changes that require a minimum Selenium Python version for specific APIs (e.g., BiDi features like `Options.enable_bidi`, console/message handlers like `Script.add_console_message_handler`, or event handlers like `BrowsingContext.add_event_handler`), do not use the Selenium API docs site (`https://www.selenium.dev/selenium/docs/api/py/`) as the source of introduction/version history, since it reflects the latest API shape. Instead, confirm feature availability by checking the official Python changelog (`py/CHANGES` in the Selenium repo) and/or inspecting the Selenium source (tags/commits corresponding to candidate versions). Only accept the stated minimum version after verifying that the referenced APIs exist in that Selenium version (e.g., Selenium 4.32.0 includes the BiDi APIs used by openwisp-utils’ BiDi-related PR `#701` as of that version).

Applied to files:

  • openwisp_utils/tests/selenium.py
🪛 LanguageTool
requirements-test.txt

[grammar] ~2-~2: Ensure spelling is correct
Context: ...roller/archive/refs/heads/master.tar.gz freezegun pytest pytest-mock

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

🔇 Additional comments (1)
requirements-test.txt (1)

2-2: LGTM!

Comment thread openwisp_utils/tests/selenium.py
@kilo-code-bot

kilo-code-bot Bot commented Jul 14, 2026

Copy link
Copy Markdown

Code Review Summary

Status: No Issues Found | Recommendation: Merge

This incremental update forwards the supplied driver through the wait path in find_element() and find_elements() (getattr(self, method)(by, value, timeout, driver=driver)). This correctly resolves the prior mismatch where the visibility wait fell back to self.web_driver while the element lookup used the passed-in driver — a nice, focused fix. The default case (driver == self.web_driver) remains unchanged, and the target wait methods already accept a driver argument. No critical bugs or security vulnerabilities were found in the changed code.

Files Reviewed (1 file)
  • openwisp_utils/tests/selenium.py
Previous Review Summary (commit 64cd554)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit 64cd554)

Status: No Issues Found | Recommendation: Merge

This is a clean refactor. SeleniumTestMixin.login() and logout() now route element lookups through the mixin's own find_element() helper with an explicit driver argument, and the openwisp_controller test dependency URL was switched to the archive/refs/heads/master.tar.gz format. In the default case (driver == self.web_driver) behavior is unchanged, and the dependency URL is a valid GitHub archive endpoint. No critical bugs or security vulnerabilities were found in the changed code.

Files Reviewed (2 files)
  • openwisp_utils/tests/selenium.py
  • requirements-test.txt

Reviewed by claude-opus-4.8 · Input: 20 · Output: 3.3K · Cached: 245.6K

@coveralls

coveralls commented Jul 14, 2026

Copy link
Copy Markdown

Coverage Status

coverage: 97.641%. remained the same — fix-selenium-test-mixin into master

@nemesifier

Copy link
Copy Markdown
Member

@coderabbitai resolve

@nemesifier nemesifier changed the title [chores] Made SeleniumTestMixin to use it's own find_element method [tests] Made SeleniumTestMixin to use it's own find_element method Jul 14, 2026
@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown
✅ Action performed

Comments resolved and changes approved.

@nemesifier
nemesifier merged commit ffdbf03 into master Jul 14, 2026
38 checks passed
@nemesifier
nemesifier deleted the fix-selenium-test-mixin branch July 14, 2026 14:53
@nemesifier nemesifier changed the title [tests] Made SeleniumTestMixin to use it's own find_element method [chores] Made SeleniumTestMixin to use its own find_element method Jul 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants