Investigate flaky download test

[asolntsev]

User description

💥 What does this PR do?

Investigate this flaky test:

canDownloadFiles() (org.openqa.selenium.grid.router.RemoteWebDriverDownloadTest)
org.opentest4j.AssertionFailedError: 
Expecting actual:
  ""
to be equal to:
  "Hello, World!"
when ignoring newlines (\n, \r\n).
	at org.openqa.selenium.grid.router.RemoteWebDriverDownloadTest.canDownloadFiles(RemoteWebDriverDownloadTest.java:163)

e.g. https://github.com/SeleniumHQ/selenium/actions/runs/20090736672/job/57637952727?pr=16713

🔄 Types of changes

• Bug fix (backwards compatible)

---

PR Type

Bug fix, Tests

---

Description

• Add diagnostic logging to investigate flaky download test

• Convert canDownloadFiles test to repeated test (20 iterations)

• Improve WebDriverWait polling with explicit poll interval

• Temporarily disable non-Java CI workflows for investigation

---

File Walkthrough

	Relevant files
Tests	RemoteWebDriverDownloadTest.java Add diagnostics and repeat flaky download test                      java/test/org/openqa/selenium/grid/router/RemoteWebDriverDownloadTest.java Added diagnostic logging to print found files and matching files during download wait Changed canDownloadFiles() from single @Test to @RepeatedTest(20) for flakiness investigation Enhanced WebDriverWait with explicit poll interval of 50ms for more responsive polling Refactored stream collection to use static import toList() and improved code readability Consolidated import statements for JUnit Jupiter annotations +17/-15
Configuration changes	ci-java.yml Disable macOS and remote CI jobs temporarily                          .github/workflows/ci-java.yml Removed macOS browser tests job entirely Removed remote tests job entirely Removed FederatedCredentialManagementTest from Windows test run Kept only Windows CI job with RemoteWebDriverDownloadTest focus +0/-44    ci.yml Disable non-Java CI workflows temporarily                                .github/workflows/ci.yml Removed .NET CI workflow job Removed Python CI workflow job Removed Ruby CI workflow job Removed Rust CI workflow job Kept only Java CI workflow active +0/-50

---

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Diagnostic logging: Added diagnostic logging prints to stdout which are not structured audit logs and may not align with audit trail requirements though this is test code. Referred Code HasDownloads hasDownloads = (HasDownloads) driver; new WebDriverWait(driver, Duration.ofSeconds(5), Duration.ofMillis(50)) .until( d -> { List<String> files = hasDownloads.getDownloadableFiles(); List<String> matchingFiles = files.stream() .filter((f) -> FILE_EXTENSIONS.stream().anyMatch(f::endsWith)) .collect(toList()); System.out.printf( "[*****] FOUND %s FILES: %s; MATCHING %s FILES: %s%n", files.size(), files, matchingFiles.size(), matchingFiles); // ensure we hit no temporary file created by the browser while downloading return matchingFiles.size() == 2; }); Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Test wait logic: The polling wait assumes exactly two matching files without explicit handling/logging for timeouts beyond a console print, which may hinder diagnosing certain edge cases. Referred Code HasDownloads hasDownloads = (HasDownloads) driver; new WebDriverWait(driver, Duration.ofSeconds(5), Duration.ofMillis(50)) .until( d -> { List<String> files = hasDownloads.getDownloadableFiles(); List<String> matchingFiles = files.stream() .filter((f) -> FILE_EXTENSIONS.stream().anyMatch(f::endsWith)) .collect(toList()); System.out.printf( "[*****] FOUND %s FILES: %s; MATCHING %s FILES: %s%n", files.size(), files, matchingFiles.size(), matchingFiles); // ensure we hit no temporary file created by the browser while downloading return matchingFiles.size() == 2; }); Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Unstructured logs: Uses System.out.printf to log lists of filenames which are unstructured and could expose file names; consider structured logging or redaction even in tests if sensitive. Referred Code System.out.printf( "[*****] FOUND %s FILES: %s; MATCHING %s FILES: %s%n", files.size(), files, matchingFiles.size(), matchingFiles); // ensure we hit no temporary file created by the browser while downloading Learn more about managing compliance generic rules or creating your own custom rules
Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Revert disruptive CI configuration changes The suggestion advises reverting the CI configuration changes in files like ci.yml and ci-java.yml. These changes disable tests for multiple languages and platforms, which is unsuitable for a final merge as it compromises regression testing. Examples: .github/workflows/ci.yml [48-58] java: name: Java needs: check uses: ./.github/workflows/ci-java.yml if: > github.event_name == 'schedule' || github.event_name == 'workflow_dispatch' || github.event_name == 'workflow_call' || contains(needs.check.outputs.targets, '//java') || contains(join(github.event.commits.*.message), '[java]') || ... (clipped 1 lines) .github/workflows/ci-java.yml [24-31] run: | fsutil 8dot3name set 0 bazel test --flaky_test_attempts 3 --pin_browsers=true //java/test/org/openqa/selenium/chrome:ChromeDriverFunctionalTest ` //java/test/org/openqa/selenium/firefox:FirefoxDriverBuilderTest ` //java/test/org/openqa/selenium/grid/router:RemoteWebDriverDownloadTest ` //java/test/org/openqa/selenium/remote:RemoteWebDriverBuilderTest ` //java/test/org/openqa/selenium/grid/router:RemoteWebDriverDownloadTest ` //java/test/org/openqa/selenium/devtools:NetworkInterceptorRestTest Solution Walkthrough: Before: # .github/workflows/ci.yml jobs: check: ... dotnet: name: .NET ... java: name: Java ... python: name: Python ... ruby: name: Ruby ... rust: name: Rust ... After: # .github/workflows/ci.yml jobs: check: ... java: name: Java ... Suggestion importance[1-10]: 10 __ Why: The suggestion correctly identifies that the PR disables most of the CI pipeline, which is a critical issue, and rightly advises reverting these temporary debugging changes before merging.	High
Learned best practice	Ensure driver cleanup with finally Wrap driver usage in try/finally and call driver.quit() in finally so the session closes even if waits or assertions fail. java/test/org/openqa/selenium/grid/router/RemoteWebDriverDownloadTest.java [105-135] WebDriver driver = new RemoteWebDriver(gridUrl, capabilities); driver = new Augmenter().augment(driver); +try { + driver.get(appServer.whereIs("downloads/download.html")); + driver.findElement(By.id("file-1")).click(); + driver.findElement(By.id("file-2")).click(); -driver.get(appServer.whereIs("downloads/download.html")); -driver.findElement(By.id("file-1")).click(); -driver.findElement(By.id("file-2")).click(); + HasDownloads hasDownloads = (HasDownloads) driver; + new WebDriverWait(driver, Duration.ofSeconds(5), Duration.ofMillis(50)) + .until( + d -> { + List<String> files = hasDownloads.getDownloadableFiles(); + List<String> matchingFiles = + files.stream() + .filter((f) -> FILE_EXTENSIONS.stream().anyMatch(f::endsWith)) + .collect(toList()); + System.out.printf( + "[*****] FOUND %s FILES: %s; MATCHING %s FILES: %s%n", + files.size(), files, matchingFiles.size(), matchingFiles); + return matchingFiles.size() == 2; + }); -HasDownloads hasDownloads = (HasDownloads) driver; -new WebDriverWait(driver, Duration.ofSeconds(5), Duration.ofMillis(50)) - .until( - d -> { - List<String> files = hasDownloads.getDownloadableFiles(); - List<String> matchingFiles = - files.stream() - .filter((f) -> FILE_EXTENSIONS.stream().anyMatch(f::endsWith)) - .collect(toList()); - System.out.printf( - "[*****] FOUND %s FILES: %s; MATCHING %s FILES: %s%n", - files.size(), files, matchingFiles.size(), matchingFiles); - // ensure we hit no temporary file created by the browser while downloading - return matchingFiles.size() == 2; - }); + List<String> downloadableFiles = hasDownloads.getDownloadableFiles(); + assertThat(downloadableFiles).contains("file_1.txt", "file_2.jpg"); -List<String> downloadableFiles = hasDownloads.getDownloadableFiles(); -assertThat(downloadableFiles).contains("file_1.txt", "file_2.jpg"); + List<DownloadedFile> downloadedFiles = hasDownloads.getDownloadedFiles(); + assertThat(downloadedFiles.stream().map(f -> f.getName()).collect(toList())) + .contains("file_1.txt", "file_2.jpg"); +} finally { + driver.quit(); +} -List<DownloadedFile> downloadedFiles = hasDownloads.getDownloadedFiles(); -assertThat(downloadedFiles.stream().map(f -> f.getName()).collect(toList())) - .contains("file_1.txt", "file_2.jpg"); - -driver.quit(); - [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Always ensure external resources (e.g., WebDriver sessions) are cleaned up via try/finally to prevent leaks when assertions or waits fail.	Low
Possible issue	Remove duplicated test from CI Remove the duplicate //java/test/org/openqa/selenium/grid/router:RemoteWebDriverDownloadTest target from the bazel test command to avoid running the same test suite twice and reduce CI job duration. .github/workflows/ci-java.yml [26-31] bazel test --flaky_test_attempts 3 --pin_browsers=true //java/test/org/openqa/selenium/chrome:ChromeDriverFunctionalTest ` //java/test/org/openqa/selenium/firefox:FirefoxDriverBuilderTest ` //java/test/org/openqa/selenium/grid/router:RemoteWebDriverDownloadTest ` //java/test/org/openqa/selenium/remote:RemoteWebDriverBuilderTest ` - //java/test/org/openqa/selenium/grid/router:RemoteWebDriverDownloadTest ` //java/test/org/openqa/selenium/devtools:NetworkInterceptorRestTest [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies a redundant test target in the CI configuration, which unnecessarily increases job duration.	Low
Update