Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

[js] Add high-level script API examples #1834

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.

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

Merged
harsha509 merged 2 commits into trunk from add-script-high-level-js
Aug 5, 2024
Merged

Conversation

@pujagani
Copy link
Contributor

@pujagani pujagani commented Aug 2, 2024 •
edited by qodo-merge-pro bot
Loading

User description

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

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

Description

Add examples to demonstrate usage.

Motivation and Context

Types of changes

  • Change to the site (I have double-checked the Netlify deployment, and my changes look good)
  • Code example added (and I also added the example to all translated languages)
  • Improved translation
  • Added new translation (and I also added a notice to each document missing translation)

Checklist

  • I have read the contributing document.
  • I have used hugo to render the site/docs locally and I am sure it works.

PR Type

Tests

Description

  • Added a new test suite to demonstrate the usage of BiDi logging in Firefox.
  • Implemented tests to verify the functionality of console log handling, JavaScript error handling, and DOM mutation handling.
  • Included tests for pinning and unpinning scripts to ensure proper script management.

Changes walkthrough 📝

Relevant files
Tests
log.js
Add BiDi logging tests for Firefox in JavaScript

examples/javascript/test/bidirectional/w3c/log.js

  • Added a new test suite for BiDi logging in Firefox.
  • Implemented tests for console log handling, JavaScript error handling,
    and DOM mutation handling.
  • Included tests for pinning and unpinning scripts.
    • +162/-0

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link
    Contributor

    qodo-merge-pro bot commented Aug 2, 2024

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Code Duplication
    The code for setting up and tearing down the driver is repeated in each test. Consider refactoring this into a separate setup and teardown method to reduce redundancy and improve maintainability.

    Magic Numbers
    The use of magic numbers (e.g., delay of 3000ms) is found throughout the tests. It's better to define these values as constants at the beginning of the file to increase code readability and ease of maintenance.

    Error Handling
    There is no error handling for potential failures in network requests or element interactions. Consider adding try-catch blocks or using promise rejection handling to improve the robustness of the tests.

    Copy link
    Contributor

    qodo-merge-pro bot commented Aug 2, 2024 •
    edited
    Loading

    CI Failure Feedback 🧐

    (Checks updated until commit 1dfe68e)

    Action: tests (ubuntu, nightly)

    Failed stage: Install Edge for set binary test [❌]

    Failure summary:

    The action failed because it was unable to download the Edge stable artifact for the platform linux
    amd64. The error message Artifact not found of Edge stable for platform linux amd64 indicates that
    the required artifact is missing or not available.

    Relevant error logs: 
    1: ##[group]Operating System
2: Ubuntu
...
146: edge-version: stable
147: env:
148: DISPLAY: :99
149: GITHUB_TOKEN: ***
150: GH_TOKEN: ***
151: ##[endgroup]
152: Setup Edge stable
153: Attempting to download Edge stable...
154: ##[error]Artifact not found of Edge stable for platform linux amd64
    ✨ CI feedback usage guide:

    The CI feedback tool (/checks) automatically triggers when a PR has a failed check.
    The tool analyzes the failed checks and provides several feedbacks:

    • Failed stage
    • Failed test name
    • Failure summary
    • Relevant error logs

    In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR:

    /checks "https://github.com/{repo_name}/actions/runs/{run_number}/job/{job_number}"

    where {repo_name} is the name of the repository, {run_number} is the run number of the failed check, and {job_number} is the job number of the failed check.

    Configuration options

    • enable_auto_checks_feedback - if set to true, the tool will automatically provide feedback when a check is failed. Default is true.
    • excluded_checks_list - a list of checks to exclude from the feedback, for example: ["check1", "check2"]. Default is an empty list.
    • enable_help_text - if set to true, the tool will provide a help message with the feedback. Default is true.
    • persistent_comment - if set to true, the tool will overwrite a previous checks comment with the new feedback. Default is true.
    • final_update_message - if persistent_comment is true and updating a previous checks message, the tool will also create a new message: "Persistent checks updated to latest commit". Default is true.

    See more information about the checks tool in the docs.
    Copy link
    Contributor

    qodo-merge-pro bot commented Aug 2, 2024

    PR Code Suggestions ✨

    CategorySuggestion Score
    Best practice
    Use strict equality checks to avoid type coercion issues

    Use assert.strictEqual instead of assert.equal to ensure that both type and value
    are checked, which is a best practice for avoiding potential bugs due to type
    coercion.

    examples/javascript/test/bidirectional/w3c/log.js [37] 

    -assert.equal(log.text, 'Hello, world!')
+assert.strictEqual(log.text, 'Hello, world!')
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Using assert.strictEqual instead of assert.equal is a best practice that ensures both type and value are checked, reducing the risk of bugs due to type coercion. This is an important improvement for test reliability.

    		9
    Enhancement
    Ensure elements are visible before interacting with them to prevent errors

    Consider checking for the visibility of the element before attempting to click on it
    to avoid potential errors where the element is not interactable.

    examples/javascript/test/bidirectional/w3c/log.js [33] 

    -await driver.findElement({ id: 'consoleLog' }).click()
+let consoleLogButton = await driver.findElement({ id: 'consoleLog' });
+await driver.wait(until.elementIsVisible(consoleLogButton), 5000);
+await consoleLogButton.click();
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Checking for the visibility of elements before interacting with them can prevent potential errors and improve the robustness of the tests. This is a valuable enhancement.

    		8
    Maintainability
    Replace hardcoded delay times with a constant for easier adjustments

    Replace the hardcoded delay time with a variable or constant to make it easier to
    adjust the delay across multiple tests if needed.

    examples/javascript/test/bidirectional/w3c/log.js [35] 

    -await delay(3000)
+const DELAY_TIME = 3000;
+await delay(DELAY_TIME)
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: This suggestion improves maintainability by making it easier to adjust the delay time across multiple tests. However, it is not crucial for the functionality of the tests.

    		7
    Refactor handler management into functions for better reusability and maintainability

    Refactor repeated code for adding and removing handlers into separate functions to
    improve code reusability and maintainability.

    examples/javascript/test/bidirectional/w3c/log.js [28-38] 

    -const handler = await driver.script().addConsoleMessageHandler((logEntry) => {
- log = logEntry
-})
+async function addConsoleHandler() {
+ return await driver.script().addConsoleMessageHandler((logEntry) => {
+ log = logEntry
+ });
+}
+async function removeConsoleHandler(handler) {
+ await driver.script().removeConsoleMessageHandler(handler);
+}
+const handler = await addConsoleHandler();
 ...
-await driver.script().removeConsoleMessageHandler(handler)
+await removeConsoleHandler(handler);
    Suggestion importance[1-10]: 6

    Why: Refactoring repeated code into functions improves code reusability and maintainability. However, this change is more about code organization and does not impact the functionality or performance significantly.

    		6

    Copy link

    netlify bot commented Aug 2, 2024 •
    edited
    Loading

    ✅ Deploy Preview for selenium-dev ready!

    Name Link
    🔨 Latest commit 1dfe68e
    🔍 Latest deploy log https://app.netlify.com/sites/selenium-dev/deploys/66b07ccd99d70c0008c098bf
    😎 Deploy Preview https://deploy-preview-1834--selenium-dev.netlify.app
    📱 Preview on mobile
    Toggle QR Code...

    QR Code

    Use your smartphone camera to open QR code link.

    To edit notification comments on pull requests, go to your Netlify site configuration.

    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 @pujagani !

    @harsha509 harsha509 merged commit 83eda2e into trunk Aug 5, 2024
    @harsha509 harsha509 deleted the add-script-high-level-js branch August 5, 2024 07:22
    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

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    AltStyle によって変換されたページ (->オリジナル) /