Skip to content

Conversation

hbenl
Copy link
Contributor

@hbenl hbenl commented Sep 4, 2025

Fixes #37217 and the following tests in tests/library/download.spec.ts in Chrome:

  • "should report downloads with acceptDownloads: false"
  • "should report proper download url when download is from download attribute"
  • "should error when saving with downloads disabled"
  • "should close the context without awaiting the download"
  • "should be able to cancel pending downloads"
  • "should download even if there is no "attachment" value"
  • "should convert navigation to a resource with unsupported mime type into download"
  • "should download links with data url"

Most of the download tests are still failing because Chrome sends the browsingContext.downloadEnd event with status: "canceled" and so methods like download.path() fail.
In Firefox the tests fail mostly because the browsingContext.downloadEnd event isn't implemented yet and so methods like download.path() hang.

Copy link
Contributor

github-actions bot commented Sep 4, 2025

Test results for "tests 1"

1 failed
❌ [playwright-test] › runner.spec.ts:118 › should ignore subprocess creation error because of SIGINT @macos-latest-node18-1

3 flaky ⚠️ [firefox-library] › library/inspector/cli-codegen-1.spec.ts:1079 › cli codegen › should not throw csp directive violation errors `@firefox-ubuntu-22.04-node18`
⚠️ [firefox-page] › page/page-emulate-media.spec.ts:144 › should keep reduced motion and color emulation after reload `@firefox-ubuntu-22.04-node18`
⚠️ [webkit-page] › page/page-set-input-files.spec.ts:146 › should upload large file `@webkit-ubuntu-22.04-node18`

46708 passed, 819 skipped


Merge workflow run.

@whimboo
Copy link
Collaborator

whimboo commented Sep 5, 2025

Fixes #37217 and the following tests in tests/library/download.spec.ts in Chrome:
Most of the download tests are still failing because Chrome sends the browsingContext.downloadEnd event with status: "canceled" and so methods like download.path() fail.

CC @OrKoN and @sadym-chromium given that you might be interested in those details. Looks like you may miss to update the status property in case of a successful download.

@whimboo
Copy link
Collaborator

whimboo commented Sep 5, 2025

In Firefox the tests fail mostly because the browsingContext.downloadEnd event isn't implemented yet and so methods like download.path() hang.

@hbenl, how many tests are these which are hanging? We probably should add them as timing out to the expectation file.

@OrKoN
Copy link
Contributor

OrKoN commented Sep 5, 2025

I think playwright is not using the latest chromium-bidi? Could that be bumped? The latest code looks correct https://github.com/GoogleChromeLabs/chromium-bidi/blob/main/src/bidiMapper/modules/context/BrowsingContextImpl.ts#L835 so it might be indeed cancelled?

@hbenl
Copy link
Contributor Author

hbenl commented Sep 5, 2025

@hbenl, how many tests are these which are hanging? We probably should add them as timing out to the expectation file.

All of the tests in tests/library/download.spec.ts (36 in total) are hanging in Firefox, with or without this PR, and they're already in the expectations file.

@whimboo
Copy link
Collaborator

whimboo commented Sep 5, 2025

I think playwright is not using the latest chromium-bidi? Could that be bumped? The latest code looks correct https://github.com/GoogleChromeLabs/chromium-bidi/blob/main/src/bidiMapper/modules/context/BrowsingContextImpl.ts#L835 so it might be indeed cancelled?

I would suggest to get this done in a separate PR once this landed. Could you do this @OrKoN?

@hbenl, how many tests are these which are hanging? We probably should add them as timing out to the expectation file.

All of the tests in tests/library/download.spec.ts (36 in total) are hanging in Firefox, with or without this PR, and they're already in the expectations file.

Perfect. Then BiDi-wise this PR looks good to me. Thanks!

@OrKoN
Copy link
Contributor

OrKoN commented Sep 5, 2025

Bumping chromium-bidi here #37318

Actually I looked up existing tests and I think the headless shell would cancel all downloads by default (https://github.com/GoogleChromeLabs/chromium-bidi/blob/main/tests/browsing_context/test_download.py#L83) I think playwright might be using the headless shell for the headless mode? or is it running with chrome --headless? Feel free to file an issue for https://github.com/GoogleChromeLabs/chromium-bidi/issues if it still does not work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: Implement the browsingContext.downloadWillBegin event for WebDriver BiDi
3 participants