[CUS-10915] Verifies if element is clickable in mobile web.#345
[CUS-10915] Verifies if element is clickable in mobile web.#345ManojTestsigma wants to merge 1 commit intodevfrom
Conversation
📝 WalkthroughWalkthroughA new mobile web UI action class, Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
element_validation_actions/src/main/java/com/testsigma/addons/mobileweb/VerifyElementIsTapable.java (1)
38-38: Prefer parameterized logging over string concatenationString concatenation in
logger.debug(...)eagerly builds the message string even when debug logging is disabled at runtime, causing unnecessary allocations on every invocation.♻️ Proposed refactor
- logger.debug("Element locator: " + this.element.getValue() + " by: " + this.element.getBy()); + logger.debug("Element locator: {} by: {}", this.element.getValue(), this.element.getBy());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@element_validation_actions/src/main/java/com/testsigma/addons/mobileweb/VerifyElementIsTapable.java` at line 38, Replace the concatenated debug log call so it uses the logger's parameterized message API to avoid eager string construction; in VerifyElementIsTapable (the logger.debug call that currently concatenates this.element.getValue() and this.element.getBy()), change it to a parameterized form that passes the two values as arguments to the logger (or guard with logger.isDebugEnabled() if parameterization isn't supported) so the message is only formatted when debug is enabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@element_validation_actions/src/main/java/com/testsigma/addons/mobileweb/VerifyElementIsTapable.java`:
- Line 36: The method declaration for VerifyElementIsTapable.execute currently
declares "throws NoSuchElementException" but that exception is caught inside the
method and is unchecked, so remove the redundant throws clause from the
execute() signature in class VerifyElementIsTapable; update the method
declaration "public Result execute()" accordingly and run a quick compile to
ensure no callers rely on the checked-exception contract.
- Around line 44-48: Remove the unreachable null-check block that tests "if
(webElement == null)" after the wait.until(ExpectedCondition<WebElement>) call
in VerifyElementIsTapable; wait.until(...) either returns a non-null WebElement
or throws a TimeoutException, so delete the webElement==null branch (including
setErrorMessage(...), logger.warn(...), and return Result.FAILED) and rely on
the existing TimeoutException catch to handle the not-interactable/not-found
case.
---
Nitpick comments:
In
`@element_validation_actions/src/main/java/com/testsigma/addons/mobileweb/VerifyElementIsTapable.java`:
- Line 38: Replace the concatenated debug log call so it uses the logger's
parameterized message API to avoid eager string construction; in
VerifyElementIsTapable (the logger.debug call that currently concatenates
this.element.getValue() and this.element.getBy()), change it to a parameterized
form that passes the two values as arguments to the logger (or guard with
logger.isDebugEnabled() if parameterization isn't supported) so the message is
only formatted when debug is enabled.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
element_validation_actions/src/main/java/com/testsigma/addons/mobileweb/VerifyElementIsTapable.java
| private com.testsigma.sdk.Element element; | ||
|
|
||
| @Override | ||
| public Result execute() throws NoSuchElementException { |
There was a problem hiding this comment.
Misleading throws NoSuchElementException declaration
NoSuchElementException is caught internally on lines 59–62 and never rethrown, so declaring it in the method signature communicates a contract that is never fulfilled. Since NoSuchElementException is also an unchecked exception (extends RuntimeException), the declaration is not compiler-enforced — it is purely misleading documentation noise. Remove it.
🐛 Proposed fix
- public Result execute() throws NoSuchElementException {
+ public Result execute() {📝 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.
| public Result execute() throws NoSuchElementException { | |
| public Result execute() { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@element_validation_actions/src/main/java/com/testsigma/addons/mobileweb/VerifyElementIsTapable.java`
at line 36, The method declaration for VerifyElementIsTapable.execute currently
declares "throws NoSuchElementException" but that exception is caught inside the
method and is unchecked, so remove the redundant throws clause from the
execute() signature in class VerifyElementIsTapable; update the method
declaration "public Result execute()" accordingly and run a quick compile to
ensure no callers rely on the checked-exception contract.
| if (webElement == null) { | ||
| setErrorMessage("Element is not tapable: element not found or not interactable within " + DEFAULT_WAIT_SECONDS + " seconds."); | ||
| logger.warn(getErrorMessage()); | ||
| return Result.FAILED; | ||
| } |
There was a problem hiding this comment.
Dead code — wait.until() never returns null for ExpectedCondition<WebElement>
until() returns the located element once the condition is met. On failure, a TimeoutException is thrown if no element is found in time. Because wait.until() either returns a non-null WebElement or throws, the webElement == null guard on line 44 can never be true, making lines 44–48 entirely unreachable dead code. The TimeoutException catch block on lines 54–58 already handles the non-clickable scenario.
🐛 Proposed fix — remove the dead null check
WebElement webElement = wait.until(ExpectedConditions.elementToBeClickable(element.getBy()));
- if (webElement == null) {
- setErrorMessage("Element is not tapable: element not found or not interactable within " + DEFAULT_WAIT_SECONDS + " seconds.");
- logger.warn(getErrorMessage());
- return Result.FAILED;
- }
-
setSuccessMessage("Element is tapable (visible, enabled, and ready for interaction).");📝 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.
| if (webElement == null) { | |
| setErrorMessage("Element is not tapable: element not found or not interactable within " + DEFAULT_WAIT_SECONDS + " seconds."); | |
| logger.warn(getErrorMessage()); | |
| return Result.FAILED; | |
| } | |
| WebElement webElement = wait.until(ExpectedConditions.elementToBeClickable(element.getBy())); | |
| setSuccessMessage("Element is tapable (visible, enabled, and ready for interaction)."); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@element_validation_actions/src/main/java/com/testsigma/addons/mobileweb/VerifyElementIsTapable.java`
around lines 44 - 48, Remove the unreachable null-check block that tests "if
(webElement == null)" after the wait.until(ExpectedCondition<WebElement>) call
in VerifyElementIsTapable; wait.until(...) either returns a non-null WebElement
or throws a TimeoutException, so delete the webElement==null branch (including
setErrorMessage(...), logger.warn(...), and return Result.FAILED) and rely on
the existing TimeoutException catch to handle the not-interactable/not-found
case.
please review this addon and publish as PUBLIC
Addon name : element_validation_actions
Addon accont: https://jarvis.testsigma.com/ui/tenants/3072/addons
Jira: https://testsigma.atlassian.net/browse/CUS-10915
Summary by CodeRabbit