Skip to content

[py] Fixed Flaky Upload Tests#14706

Merged
VietND96 merged 1 commit intoSeleniumHQ:trunkfrom
shbenzer:flaky-upload-test
Nov 2, 2024
Merged

[py] Fixed Flaky Upload Tests#14706
VietND96 merged 1 commit intoSeleniumHQ:trunkfrom
shbenzer:flaky-upload-test

Conversation

@shbenzer
Copy link
Copy Markdown
Contributor

@shbenzer shbenzer commented Nov 2, 2024 •
edited by qodo-code-review Bot
Loading

User description

Description

Fixed flaky upload tests in upload_tests.py

Motivation and Context

Flaky tests were impeding CI

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

  • Improved the reliability of upload tests by incorporating WebDriverWait to ensure that the expected text is present in the element before making assertions.
  • Replaced direct text assertions with expected_conditions to handle potential timing issues in test execution.
  • These changes address flaky tests that were causing issues in the continuous integration process.

Changes walkthrough 📝

Relevant files
Tests
upload_tests.py
Improve reliability of upload tests with WebDriverWait     

py/test/selenium/webdriver/common/upload_tests.py

  • Added WebDriverWait to ensure text presence in upload tests.
  • Replaced direct text assertions with expected_conditions.
  • Improved test reliability by waiting for conditions.
    		•  +5/-5     

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

    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review Bot commented Nov 2, 2024

    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

    Timeout Handling
    Verify if the 10-second timeout in WebDriverWait is sufficient for all test environments and slower systems.

    Error Handling
    Check if appropriate error handling is implemented for cases where the expected text doesn't appear within the timeout period.

    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review Bot commented Nov 2, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Create a helper method for repeated WebDriverWait assertions to improve code organization and reduce duplication

    Consider creating a helper method for the WebDriverWait assertion to reduce code
    duplication and improve readability.

    py/test/selenium/webdriver/common/upload_tests.py [55-56]

    -WebDriverWait(driver, 10).until(EC.text_to_be_present_in_element((By.CSS_SELECTOR, "body"), "test_file.txt"))
-WebDriverWait(driver, 10).until(EC.text_to_be_present_in_element((By.CSS_SELECTOR, "body"), "test_file2.txt"))
+def assert_file_uploaded(driver, filename):
+    WebDriverWait(driver, 10).until(EC.text_to_be_present_in_element((By.CSS_SELECTOR, "body"), filename))
 
+assert_file_uploaded(driver, "test_file.txt")
+assert_file_uploaded(driver, "test_file2.txt")
+
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Creating a helper method for repeated WebDriverWait assertions reduces code duplication and improves readability, making the tests more organized and easier to maintain. This is a valuable enhancement for code quality.

    		8
    Best practice
    Use a constant for timeout values to improve maintainability and consistency

    Consider using a constant for the timeout value in WebDriverWait to improve
    maintainability and consistency across tests.

    py/test/selenium/webdriver/common/upload_tests.py [45]

    -WebDriverWait(driver, 10).until(EC.text_to_be_present_in_element((By.CSS_SELECTOR, "body"), "test_file.txt"))
+UPLOAD_WAIT_TIMEOUT = 10
+WebDriverWait(driver, UPLOAD_WAIT_TIMEOUT).until(EC.text_to_be_present_in_element((By.CSS_SELECTOR, "body"), "test_file.txt"))
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Using a constant for the timeout value enhances maintainability and consistency across tests, making it easier to update the timeout in one place if needed. This is a good practice for improving code quality.

    		7

    💡 Need additional feedback ? start a PR chat

    @VietND96 VietND96 merged commit 39fee38 into SeleniumHQ:trunk Nov 2, 2024
    @VietND96
    Copy link
    Copy Markdown
    Member

    VietND96 commented Nov 2, 2024

    Thank you, @shbenzer!

    @shbenzer shbenzer deleted the flaky-upload-test branch January 16, 2025 00:38
    @dependabot dependabot Bot mentioned this pull request Sep 18, 2025
    Merged
    @dependabot dependabot Bot mentioned this pull request Oct 1, 2025
    Closed
    @dependabot dependabot Bot mentioned this pull request Oct 24, 2025
    Closed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    No reviews

    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.

    2 participants

    @shbenzer @VietND96