Skip to content

Conversation

@navin772
Copy link
Member

@navin772 navin772 commented Nov 12, 2025

User description

🔗 Related Issues

💥 What does this PR do?

Enables test_add_event_handler_download_end and test_add_event_handler_download_will_begin tests for Firefox.
From Firefox 145, these events are supported.

Also, makes the assertion generalized in case of existing downloads.

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

  • Cleanup (formatting, renaming)

PR Type

Tests


Description

  • Remove xfail markers for Firefox download event tests

  • Generalize filename assertion to handle existing downloads

  • Support Firefox 145+ download event functionality


Diagram Walkthrough

flowchart LR
  A["Firefox 145+ Support"] --> B["Remove xfail Markers"]
  B --> C["test_add_event_handler_download_will_begin"]
  B --> D["test_add_event_handler_download_end"]
  E["Generalize Assertion"] --> F["Handle Filename Variations"]
  F --> C
Loading

File Walkthrough

Relevant files
Tests
bidi_browsing_context_tests.py
Enable Firefox download event tests with generalized assertions

py/test/selenium/webdriver/common/bidi_browsing_context_tests.py

  • Removed @pytest.mark.xfail_firefox decorator from
    test_add_event_handler_download_will_begin test
  • Removed @pytest.mark.xfail_firefox decorator from
    test_add_event_handler_download_end test
  • Updated filename assertion to use substring matching instead of exact
    equality
  • Added comment explaining filename variation due to existing downloads
+2/-3     

@selenium-ci selenium-ci added the C-py Python Bindings label Nov 12, 2025
@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Nov 12, 2025

PR Compliance Guide 🔍

(Compliance updated until commit 5ca7c96)

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: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

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:
No audit logs: The new test code adds assertions and event handling for downloads without introducing or
verifying any audit logging for critical actions, which may be acceptable for tests but
cannot be confirmed from this diff.

Referred Code
assert len(events_received) == 1
# filename maybe file_1.txt or file_1(1).txt depending on existing files in download dir
assert "file_1" in events_received[0].suggested_filename

driver.browsing_context.remove_event_handler("download_will_begin", callback_id)

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:
Limited edge handling: The generalized filename assertion only checks for "file_1" substring and relies
on a 5s wait without explicit error handling or alternate outcomes, which may be
acceptable in tests but lacks explicit edge-case handling in this diff.

Referred Code
WebDriverWait(driver, 5).until(lambda d: len(events_received) > 0)

assert len(events_received) == 1
# filename maybe file_1.txt or file_1(1).txt depending on existing files in download dir
assert "file_1" in events_received[0].suggested_filename

Learn more about managing compliance generic rules or creating your own custom rules

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

Previous compliance checks

Compliance check up to commit 56e98cd
Security Compliance
Weak assertion

Description: The assertion on suggested_filename is loosened to a substring match ("file_1"), which
could allow false positives and mask regressions in download handling; consider validating
against a stricter expected pattern to avoid missing discrepancies.
bidi_browsing_context_tests.py [803-806]

Referred Code
assert len(events_received) == 1
# filename maybe file_1.txt or file_1(1).txt depending on existing files in download dir
assert "file_1" in events_received[0].suggested_filename
Ticket Compliance
🟡
🎫 #5678
🔴 I
n
v
e
s
t
i
g
a
d
r
o
l
p
"
E
:
C
c
F
u
(
f
)
w
h
m
D
U
b
1
6
.
0
4
5
2
3
S
9
P
x
q
-
🟡
🎫 #1234
🔴 E
n
s
u
r
e
W
b
D
i
v
2
.
4
8
t
g
J
a
S
c
p
h
o
'
f
l
k
(
)
F
x
w
y
d
7
1
P
m
-
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: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

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:
No audit logs: The new assertions and event handling in tests add no audit logging for critical actions,
but as test code this may be acceptable and outside production audit requirements.

Referred Code
assert len(events_received) == 1
# filename maybe file_1.txt or file_1(1).txt depending on existing files in download dir
assert "file_1" in events_received[0].suggested_filename

driver.browsing_context.remove_event_handler("download_will_begin", callback_id)

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:
Minimal error handling: The test relies on assertions and a short wait without explicit handling for timing or
environment edge cases, which is typical for tests but may cause flakiness.

Referred Code
WebDriverWait(driver, 5).until(lambda d: len(events_received) > 0)

assert len(events_received) == 1
# filename maybe file_1.txt or file_1(1).txt depending on existing files in download dir
assert "file_1" in events_received[0].suggested_filename

Learn more about managing compliance generic rules or creating your own custom rules

@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Nov 12, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix race condition in test

Add a short time.sleep() delay before asserting the number of events received to
prevent a potential race condition and improve test robustness.

py/test/selenium/webdriver/common/bidi_browsing_context_tests.py [799-805]

 download_xpath_file_1_txt = '//*[@id="file-1"]'
 driver.find_element(By.XPATH, download_xpath_file_1_txt).click()
 WebDriverWait(driver, 5).until(lambda d: len(events_received) > 0)
+
+# Allow a moment for any unexpected additional events to arrive.
+time.sleep(0.2)
 
 assert len(events_received) == 1
 # filename maybe file_1.txt or file_1(1).txt depending on existing files in download dir
 assert "file_1" in events_received[0].suggested_filename

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies a potential race condition in an asynchronous test and proposes adding a short delay to improve its stability, which is a valid approach to prevent test flakiness.

Low
  • Update

@navin772 navin772 merged commit 55fa188 into SeleniumHQ:trunk Nov 13, 2025
22 checks passed
@navin772 navin772 deleted the py-enable-download-event-tests-ff branch November 13, 2025 04:52
This was referenced Dec 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants