Skip to content

[js]: Fix sendKeys command fail on FileDetector.handleFile error. #14663

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Oct 28, 2024
Merged

[js]: Fix sendKeys command fail on FileDetector.handleFile error. #14663

merged 3 commits into from
Oct 28, 2024

Conversation

@garg3133
Copy link
Contributor

@garg3133 garg3133 commented Oct 27, 2024 •
edited by qodo-merge-pro bot
Loading

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

When using the .sendKeys command with a file detector, if the promise returned by the handleFile method of file detector results in a rejection, the control moves to the catch clause and then to the last this.execute_ function call.

Now, the problem with this flow is that in case of the handleFile error, the keys variable remains an Array as opposed to it being converted to a string if handleFile runs without any error. But, the last this.execute_ function call expects keys to be a string.

This results in keys.split is not a function error when the flow goes through the catch() clause.

So, this PR fixes this issue by converting keys to string if the flow goes through the catch clause so that the last this.execute_ always receives keys in a string format, as expected.

Motivation and Context

One of the Nightwatch.js users is facing an issue due to this: nightwatchjs/nightwatch#4280. They are using a remote file detector (new require('selenium-webdriver/remote').FileDetector()) in their test and when they try to send a large block of text to a text input using the .sendKeys command, they are getting the keys.split is not a function error

Please check this comment for more detailed analysis. tl;dr: Sending long text using .sendKeys with the file detector enabled leads to ENAMETOOLONG error which is not handled by the remote file detector's handleFile method, causing the method to be rejected, due to which the control flow lands inside the catch() clause in webdriver.js > sendKeys with keys variable still an Array, and using the .split method on an Array leads to the keys.split is not a function error.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

Bug fix, Tests

Description

  • Fixed a bug in the sendKeys command where an error in handleFile would cause a keys.split is not a function error by ensuring keys is converted to a string in the catch block.
  • Added a new test case to verify the behavior of sendKeys when the file detector's handleFile method rejects, ensuring robust error handling.

Changes walkthrough 📝

Relevant files
Bug fix
webdriver.js
Fix `sendKeys` command to handle file detector errors correctly

javascript/node/selenium-webdriver/lib/webdriver.js

  • Added a line to convert keys to a string in the catch block.
  • Ensures keys is always a string before calling this.execute_.
    		•  +1/-0     
    Tests
    webdriver_test.js
    Add test for `sendKeys` handling file detector errors       

    javascript/node/selenium-webdriver/test/lib/webdriver_test.js

  • Added a new test case for sendKeys with a file detector handling
    errors.
  • Ensures the sendKeys function behaves correctly when handleFile
    rejects.
    		•  +26/-0   

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    @CLAassistant
    Copy link

    CLAassistant commented Oct 27, 2024 •
    edited
    Loading

    CLA assistant check
    All committers have signed the CLA.

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Oct 27, 2024

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ❌

    4280 - Not compliant

    Not compliant requirements:

    • Fix the issue where calling location_once_scrolled_into_view before click() causes the wrong element to be clicked
    • Ensure that location_once_scrolled_into_view does not change the element being interacted with
    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Error Handling
    The added line converts keys to a string in the catch block, but it's unclear if this fully addresses the original issue or if it might introduce new problems.

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Oct 27, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Improve test robustness by explicitly asserting the expected rejection

    Consider using a more specific assertion method like assert.rejects() to test for
    the rejected promise, ensuring the error is handled correctly.

    javascript/node/selenium-webdriver/test/lib/webdriver_test.js [959]

    -return driver.findElement(By.id('foo')).sendKeys('original/', 'path')
+return assert.rejects(
+  () => driver.findElement(By.id('foo')).sendKeys('original/', 'path'),
+  /unhandled file error/
+)
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Using assert.rejects() explicitly tests for promise rejections, ensuring that the test accurately verifies the error handling behavior. This is a best practice for testing asynchronous code and enhances test reliability.

    		9
    Possible issue
    Improve robustness by checking if keys is an array before joining

    Consider using Array.isArray() to check if keys is an array before calling join().
    This ensures the code works correctly even if keys is not an array.

    javascript/node/selenium-webdriver/lib/webdriver.js [2755]

    -keys = keys.join('')
+keys = Array.isArray(keys) ? keys.join('') : keys
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: This suggestion enhances the robustness of the code by ensuring that keys is an array before calling join(). This prevents potential runtime errors if keys is not an array, making the code more reliable.

    		8
    Enhancement
    Enhance error logging by including the full error object

    Consider logging the full error object instead of just concatenating it to the
    string. This will provide more detailed error information for debugging.

    javascript/node/selenium-webdriver/lib/webdriver.js [2754]

    -this.log_.severe('Error trying parse string as a file with file detector; sending keys instead' + ex)
+this.log_.severe('Error trying parse string as a file with file detector; sending keys instead', ex)
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Logging the full error object provides more detailed information for debugging, which can be crucial for diagnosing issues. This suggestion improves the quality of error logging without altering the core functionality.

    		7

    💡 Need additional feedback ? start a PR chat

    @garg3133
    Copy link
    Contributor Author

    garg3133 commented Oct 28, 2024

    cc: @harsha509 @pujagani

    harsha509
    harsha509 approved these changes Oct 28, 2024
    View reviewed changes
    Copy link
    Member

    @harsha509 harsha509 left a comment

    Choose a reason for hiding this comment

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

    Thank you @garg3133 !

    @garg3133
    Copy link
    Contributor Author

    garg3133 commented Oct 28, 2024

    Somehow got the tests working locally and without the change made in this PR, the added test failed with the below error (as expected):

    image

    So, the added test is working correctly.

    cc: @AutomatedTester

    @harsha509 harsha509 merged commit 68f82b3 into SeleniumHQ:trunk Oct 28, 2024
    11 checks passed
    @garg3133 garg3133 deleted the sendKeys-fileDetector branch October 28, 2024 19:17
    @garg3133 garg3133 mentioned this pull request Oct 28, 2024
    Closed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @harsha509 harsha509 harsha509 approved these changes

    Assignees

    No one assigned

    Labels

    Review effort [1-5]: 2

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    4 participants

    @garg3133 @CLAassistant @harsha509 @AutomatedTester