[py][bidi]: add test for downloadEnd event

[navin772]

User description

🔗 Related Issues

Test for #16209, Chrome 140 supports this event.

💥 What does this PR do?

Adds test for the downloadEnd browsing context event.

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

• Tests

---

PR Type

Tests

---

Description

• Add test for downloadEnd BiDi browsing context event

• Verify download completion event handling and parameters

• Test multiple file downloads with status validation

---

Diagram Walkthrough

flowchart LR
  A["Test Setup"] --> B["Add Event Handler"]
  B --> C["Navigate to Download Page"]
  C --> D["Click Download Links"]
  D --> E["Wait for Events"]
  E --> F["Validate Download Parameters"]
  F --> G["Remove Event Handler"]

Loading

File Walkthrough

Relevant files

Tests

bidi_browsing_context_tests.py Add downloadEnd event handler test                                              py/test/selenium/webdriver/common/bidi_browsing_context_tests.py Add test_add_event_handler_download_end test function Test download completion event with multiple file downloads Validate download parameters including status, context, and filepath Mark test as xfail for Firefox browser +35/-0

---

[qodo-merge-pro]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 5678 - Partially compliant Compliant requirements: Non-compliant requirements: Investigate/resolve repeated "ConnectFailure" errors for ChromeDriver instantiation. Provide reproducible steps and mitigation or fix. Requires further human verification: None 1234 - Partially compliant Compliant requirements: Non-compliant requirements: Provide fix/test for JS-in-href click behavior regression. Requires further human verification: None

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Flaky Timing WebDriverWait timeout of 5s for receiving two download_end events might be flaky on slower CI or networks; consider increasing timeout or waiting per event with robust conditions. WebDriverWait(driver, 5).until(lambda d: len(events_received) > 1) assert len(events_received) == 2 Assertion Robustness The assertion that filepath contains "file_1" may fail across platforms or browsers if naming differs; consider normalizing or relaxing with regex tied to suggested filename mapping. # we assert that atleast the str "file_1" is present in the downloaded file since multiple downloads # will have numbered suffix like file_1 (1) assert "file_1" in download_event.download_params.filepath Event Ordering Assumption Test assumes first received event corresponds to the first click; event ordering may not be guaranteed for parallel downloads; validate by matching URL or use per-download correlation. download_event = events_received[0] assert download_event.download_params is not None assert download_event.download_params.status == "complete" assert download_event.download_params.context == context_id assert download_event.download_params.timestamp is not None assert "downloads/file_1.txt" in download_event.download_params.url # we assert that atleast the str "file_1" is present in the downloaded file since multiple downloads # will have numbered suffix like file_1 (1)

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	✅ Sort events to prevent flaky tests Suggestion Impact: The commit adopted the deterministic wait for exactly two events and changed assertions to avoid order dependence by validating properties for both events using order-agnostic checks. While it didn't sort the events as suggested, it addressed the flakiness concern by asserting via any() across events. code diff: - WebDriverWait(driver, 5).until(lambda d: len(events_received) > 1) + WebDriverWait(driver, 5).until(lambda d: len(events_received) == 2) assert len(events_received) == 2 - download_event = events_received[0] - assert download_event.download_params is not None - assert download_event.download_params.status == "complete" - assert download_event.download_params.context == context_id - assert download_event.download_params.timestamp is not None - assert "downloads/file_1.txt" in download_event.download_params.url - # we assert that atleast the str "file_1" is present in the downloaded file since multiple downloads + for ev in events_received: + assert ev.download_params is not None + assert ev.download_params.status == "complete" + assert ev.download_params.context == context_id + assert ev.download_params.timestamp is not None + + # we assert that atleast "file_1" is present in the downloaded file since multiple downloads # will have numbered suffix like file_1 (1) - assert "file_1" in download_event.download_params.filepath + assert any( + "downloads/file_1.txt" in ev.download_params.url and "file_1" in ev.download_params.filepath + for ev in events_received + ) + + assert any( + "downloads/file_2.jpg" in ev.download_params.url and "file_2" in ev.download_params.filepath + for ev in events_received + ) To prevent flaky tests, wait for both download events, sort them by URL to ensure a deterministic order, and then validate the properties of each event. py/test/selenium/webdriver/common/bidi_browsing_context_tests.py [828-840] -WebDriverWait(driver, 5).until(lambda d: len(events_received) > 1) +WebDriverWait(driver, 5).until(lambda d: len(events_received) == 2) assert len(events_received) == 2 -download_event = events_received[0] -assert download_event.download_params is not None -assert download_event.download_params.status == "complete" -assert download_event.download_params.context == context_id -assert download_event.download_params.timestamp is not None -assert "downloads/file_1.txt" in download_event.download_params.url -# we assert that atleast the str "file_1" is present in the downloaded file since multiple downloads +# Sort events to have a deterministic order for assertions +events_received.sort(key=lambda e: e.download_params.url) + +download_event_1 = events_received[0] +assert download_event_1.download_params is not None +assert download_event_1.download_params.status == "complete" +assert download_event_1.download_params.context == context_id +assert download_event_1.download_params.timestamp is not None +assert "downloads/file_1.txt" in download_event_1.download_params.url +# we assert that at least the str "file_1" is present in the downloaded file since multiple downloads # will have numbered suffix like file_1 (1) -assert "file_1" in download_event.download_params.filepath +assert "file_1" in download_event_1.download_params.filepath +download_event_2 = events_received[1] +assert download_event_2.download_params is not None +assert download_event_2.download_params.status == "complete" +assert download_event_2.download_params.context == context_id +assert download_event_2.download_params.timestamp is not None +assert "downloads/file_2.txt" in download_event_2.download_params.url +assert "file_2" in download_event_2.download_params.filepath + [Suggestion processed] Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a potential for flaky tests due to the non-deterministic order of asynchronous download events and provides a robust solution by sorting the events before assertion, which is a critical improvement for test reliability.	High
Learned best practice	Ensure handler removal with finally Ensure the event handler is removed in a finally block so it executes even if the test fails or raises. This prevents leaking listeners across tests. py/test/selenium/webdriver/common/bidi_browsing_context_tests.py [818-842] callback_id = driver.browsing_context.add_event_handler("download_end", on_download_end) assert callback_id is not None -... -assert "file_1" in download_event.download_params.filepath +try: + context_id = driver.current_window_handle + url = pages.url("downloads/download.html") + driver.browsing_context.navigate(context=context_id, url=url, wait=ReadinessState.COMPLETE) -driver.browsing_context.remove_event_handler("download_end", callback_id) + driver.find_element(By.ID, "file-1").click() + driver.find_element(By.ID, "file-2").click() + WebDriverWait(driver, 5).until(lambda d: len(events_received) > 1) + + assert len(events_received) == 2 + + download_event = events_received[0] + assert download_event.download_params is not None + assert download_event.download_params.status == "complete" + assert download_event.download_params.context == context_id + assert download_event.download_params.timestamp is not None + assert "downloads/file_1.txt" in download_event.download_params.url + assert "file_1" in download_event.download_params.filepath +finally: + driver.browsing_context.remove_event_handler("download_end", callback_id) + [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Always dispose resources and listeners; ensure event handlers are removed even if assertions fail (use try/finally).	Low
Update

[cgoldberg]

See the PR bot's mention of "Event Ordering Assumption". The test assertions assume events_received[0] corresponds to the first click. We might want to check all events in case they arrive out of order and make the test flaky.

Besides that, LGTM.

[navin772]

We might want to check all events in case they arrive out of order and make the test flaky

Done

[cgoldberg]

I just thought of something else...

I think we currently launch a new browser for every BiDi test, so this probably isn't an issue... but if we switch to re-using the browser like we do on non-BiDi tests, we will want to ensure that event handlers get removed in the case of a test failure so they don't leak into other tests.

Maybe we should add something to the driver fixture that checks if BiDi is enabled and removes all handlers?

[navin772]

That's true, I will see if we can reuse the browsers after removing the handlers. The issue I see is how do we remove all the handlers for all the modules, we do have some methods in some modules that remove all handlers in that module but still calling all of them in conftest seems not so good.