Skip to content

change a few things for the PR #102

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

Open
wants to merge 1 commit into
base: temp_trunk2
Choose a base branch
from
Open

Conversation

titusfortner
Copy link
Owner

@titusfortner titusfortner commented Apr 2, 2025

User description

🔗 Related Issues

No issue, really

💥 What does this PR do?

Does some really important stuff with Rust, some great features really

🔧 Implementation Notes

It's the best way to do it, honestly. Really makes a difference to Microsoft Edge in particular

💡 Additional Considerations

Nope, that's all there is

🔄 Types of changes

  • Cleanup (formatting, renaming)
  • Bug fix (backwards compatible)
  • New feature (non-breaking change which adds functionality and tests!)
  • Breaking change (fix or feature that would cause existing functionality to change)

PR Type

Tests, Bug fix, Cleanup


Description

  • Added a new test for keyboard input validation in DefaultKeyboardTest.

  • Modified Alert.cpp to improve formatting and added a comment.

  • Updated .bazelrc.remote to adjust test tag filters for remote execution.

  • Minor cleanup and formatting changes in Alert.cpp.


Changes walkthrough 📝

Relevant files
Tests
DefaultKeyboardTest.java
Added new test for numeric keyboard input                               

java/test/org/openqa/selenium/bidi/input/DefaultKeyboardTest.java

  • Added a new test method testSomethingElse.
  • Validates numeric input handling for keyboard actions.
  • +13/-0   
    Cleanup
    Alert.cpp
    Minor formatting and comment addition in Alert.cpp             

    cpp/iedriver/Alert.cpp

  • Added a comment in the namespace section.
  • Minor formatting adjustments for better readability.
  • +3/-2     
    Configuration changes
    .bazelrc.remote
    Updated Bazel remote execution test filters                           

    .bazelrc.remote

  • Updated test tag filters to exclude -remote.
  • Adjusted configuration for remote execution.
  • +1/-1     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Test Filter Change

    The PR removes the '-skip-rbe' tag from test filters. This might cause tests that were previously skipped during remote execution to now run, potentially causing failures or unexpected behavior.

    test:rbe --test_tag_filters=-exclusive-if-local,-remote
    Test Naming

    The new test method 'testSomethingElse' has a generic name that doesn't clearly describe what it's testing. Consider renaming to something more descriptive like 'testNumericKeyboardInput'.

    void testSomethingElse() {

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Descriptive test method name

    The test method name testSomethingElse is not descriptive of what's being
    tested. Since this test is validating numeric input handling, the method name
    should reflect that purpose.

    java/test/org/openqa/selenium/bidi/input/DefaultKeyboardTest.java [74-85]

     @Test
    -void testSomethingElse() {
    +void testNumericKeyboardInput() {
       driver.get(appServer.whereIs("single_text_input.html"));
     
       WebElement input = driver.findElement(By.id("textInput"));
     
       Actions sendLowercase = getBuilder(driver).sendKeys(input, "12345");
     
       inputModule.perform(windowHandle, sendLowercase.getSequences());
     
       shortWait.until(ExpectedConditions.attributeToBe(input, "value", "12345"));
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion improves code readability and maintainability by renaming the generic test method name "testSomethingElse" to the more descriptive "testNumericKeyboardInput" which clearly indicates the purpose of the test. This follows testing best practices and makes the codebase more maintainable.

    Medium
    • More

    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.

    1 participant