Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: logPolitely logs on stdout #32487

Closed
achingbrain opened this issue Sep 6, 2024 · 6 comments
Closed

[Bug]: logPolitely logs on stdout #32487

achingbrain opened this issue Sep 6, 2024 · 6 comments

Comments

@achingbrain
Copy link

Version

1.47.0

Steps to reproduce

Run playwright on a platform without browsers installed.

The logPolitely function in browserFetch.ts logs using console.log which causes output on stdout.

It should use console.warn or console.error to log to stderr instead as it can interfere with the output of the script being run.

Expected behavior

Playwright informational logging on stderr

Actual behavior

Playwright informational logging on stdout

Additional context

No response

Environment

System:
    OS: macOS 14.5
    CPU: (10) arm64 Apple M1 Max
    Memory: 133.58 MB / 64.00 GB
  Binaries:
    Node: 20.12.2 - ~/.nvm/versions/node/v20.12.2/bin/node
    Yarn: 1.22.22 - ~/.nvm/versions/node/v20.12.2/bin/yarn
    npm: 10.5.0 - ~/.nvm/versions/node/v20.12.2/bin/npm
    pnpm: 9.6.0 - ~/.nvm/versions/node/v20.12.2/bin/pnpm
  IDEs:
    VSCode: 1.92.0 - /usr/local/bin/code
  Languages:
    Bash: 3.2.57 - /bin/bash
@pavelfeldman
Copy link
Member

It should use console.warn or console.error to log to stderr instead as it can interfere with the output of the script being run.

The script being run here can only be npx playwright install, could you share more about your use case? What is the other output?

@achingbrain
Copy link
Author

The script being run here can only be npx playwright install

That's correct.

could you share more about your use case? What is the other output?

I'm using playwright to run some code in a headless browser which logs results to stdout. The output from the browser is then parsed and acted upon.

Because playwright logs to stdout instead of stderr I have to handle the playwright output separately. An upgrade to playwright could also cause new logging which might break my program if it arrives formatted differently, or if it's different in some other way that I cannot predict.

In general* stdout should be used for program output, stderr should be used for everything else - informational logging, diagnostics, debug output, etc.

I think messages like "I'm going to install a browser", "I'm downloading it now", etc, fall under the latter category so belong on stderr and not stdout.


* = "In general" is quite subjective, here are some references that discuss when to use stdout vs stderr:

@dgozman
Copy link
Contributor

dgozman commented Sep 9, 2024

@achingbrain How does your script run npx playwright install and also prints something to the output? Do you spawn a subprocess? If so, why don't you suppress the output of the subprocess, or do whatever you'd like with it?

npx playwright install is a cli tool, not a library function that you are supposed to call, so it outputs to the stdout as a user would expect, kind of similar to the output of git cli. I guess there is still some misunderstanding here. Could you please share a code example that illustrates your problem?

@achingbrain
Copy link
Author

achingbrain commented Sep 9, 2024

npx playwright install is a cli tool... so it outputs to the stdout as a user would expect

That's true, unfortunately logPolitely is used during regular execution too, not just during npx playwright install.

I just saw this during a test run:

Removing unused browser at /Users/alex/Library/Caches/ms-playwright/chromium-1133
Downloading Chromium 129.0.6668.29 (playwright build v1134) from https://playwright.azureedge.net/builds/chromium/1134/chromium-mac-arm64.zip
139 MiB [====================] 100% 0.0s
Chromium 129.0.6668.29 (playwright build v1134) downloaded to /Users/alex/Library/Caches/ms-playwright/chromium-1134

This uses logPolitely:

logPolitely('Removing unused browser at ' + directory);

@dgozman
Copy link
Contributor

dgozman commented Sep 9, 2024

I just saw this during a test run:

Could you please share a repro for this? I don't think playwright should download browsers during test execution.

achingbrain added a commit to hugomrdias/playwright-test that referenced this issue Sep 10, 2024
If a browser is downloaded during a test run, playwright will log
on stdout.

This can interfere with the calling code if it's trying to parse
the output of the program as, for example, JSON.

The fix is to temporarily replace `console.log` with `console.error`
as that's what the [logPolitely](https://github.com/microsoft/playwright/blob/718bd9b35fd206245401a9ecb320289f427592d9/packages/playwright-core/src/server/registry/browserFetcher.ts#L120)
function uses to give feedback to the user.

The intention of the playwright maintainers is [not for this
functionality to be used during a test run](microsoft/playwright#32487 (comment))
so it's unlikely to move to stderr otherwise.
hugomrdias pushed a commit to hugomrdias/playwright-test that referenced this issue Sep 10, 2024
If a browser is downloaded during a test run, playwright will log
on stdout.

This can interfere with the calling code if it's trying to parse
the output of the program as, for example, JSON.

The fix is to temporarily replace `console.log` with `console.error`
as that's what the [logPolitely](https://github.com/microsoft/playwright/blob/718bd9b35fd206245401a9ecb320289f427592d9/packages/playwright-core/src/server/registry/browserFetcher.ts#L120)
function uses to give feedback to the user.

The intention of the playwright maintainers is [not for this
functionality to be used during a test run](microsoft/playwright#32487 (comment))
so it's unlikely to move to stderr otherwise.
@achingbrain
Copy link
Author

I think I've got to the bottom of this. We're using playwright-test as it's a really simple way to just run a script in many different browsers headlessly and does nice things like installing missing browsers if required.

It uses the Registry from playwright-core to install browsers so that's why it appears during a test run.

I don't think playwright should download browsers during test execution.

That's a shame, it's really useful to not have to worry about this stuff.

I'm going to close this since it's not using the tool in the way the authors intend it to be used, though it'd still be nice to have an option on the API to control the logging output.

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

No branches or pull requests

3 participants