Skip to content

Chrome Inspector Tool: Fix info messages written to error stream#13223

Merged
graalvmbot merged 4 commits intooracle:masterfrom
florian-h05:chromeinspector-outstreams
Apr 2, 2026
Merged

Chrome Inspector Tool: Fix info messages written to error stream#13223
graalvmbot merged 4 commits intooracle:masterfrom
florian-h05:chromeinspector-outstreams

Conversation

@florian-h05
Copy link
Copy Markdown
Contributor

Fixes #13222.

@oracle-contributor-agreement
Copy link
Copy Markdown

Thank you for your pull request and welcome to our community! To contribute, please sign the Oracle Contributor Agreement (OCA).
The following contributors of this PR have not signed the OCA:

To sign the OCA, please create an Oracle account and sign the OCA in Oracle's Contributor Agreement Application.

When signing the OCA, please provide your GitHub username. After signing the OCA and getting an OCA approval from Oracle, this PR will be automatically updated.

If you are an Oracle employee, please make sure that you are a member of the main Oracle GitHub organization, and your membership in this organization is public.

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Required At least one contributor does not have an approved Oracle Contributor Agreement. label Mar 27, 2026
@fniephaus fniephaus requested review from chumer and fniephaus March 27, 2026 21:44
@fniephaus fniephaus self-assigned this Mar 27, 2026
@oracle-contributor-agreement
Copy link
Copy Markdown

Thank you for signing the OCA.

@oracle-contributor-agreement oracle-contributor-agreement bot added OCA Verified All contributors have signed the Oracle Contributor Agreement. and removed OCA Required At least one contributor does not have an approved Oracle Contributor Agreement. labels Mar 30, 2026
@fniephaus
Copy link
Copy Markdown
Member

fniephaus commented Mar 30, 2026

Thanks for the fix, and thank you for contributing this.

I took a close look at the change and ran the relevant chrome inspector tests locally with mx.

What I found:

  • The behavioral change looks correct: informational inspector output now goes to stdout, while the actual error paths in open(...) still write to stderr.
  • TimeoutTest passes.
  • InspectorMessageTransportTest passes.
  • InspectorAddressTest now fails in the cases that expect the inspector banner on stderr (testHostPortDefault, testHostPortEnabled, testPort, testPort0, and testPath).

The failures line up with the PR change: InspectorAddressTest still captures only .err(...) and parses the WebSocket banner from that stream, so it needs to be updated to read/assert stdout for the banner and keep stderr for real errors only.

So overall this looks like the right fix to me, but I do think the test update should be included before merging. Thanks again for the contribution. 🤖

Copy link
Copy Markdown
Member

@fniephaus fniephaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update InspectorAddressTest.

@florian-h05
Copy link
Copy Markdown
Contributor Author

@fniephaus Thanks for your review!
I've updated the tests, but unfortunately I haven't been able to run them locally, cannot get the mx build working on Fedora 43.
Where can I see the test results in CI?

@fniephaus
Copy link
Copy Markdown
Member

I've updated the tests, but unfortunately I haven't been able to run them locally, cannot get the mx build working on Fedora 43. Where can I see the test results in CI?

We don't run the tests on GitHub CI, too bad you cannot run them on Fedora. I will share any problems caught by our internal CI.

@fniephaus
Copy link
Copy Markdown
Member

MultiEngineTest still captures inspector startup messages from stderr at Engine.newBuilder()...err(out), but this PR moves those messages to stdout. That breaks runEngine(...) because InspectorAddressTest.parseWSPort(out.toString()) no longer sees the Debugger listening on ... line, and it also weakens the same-path assertions. This test should be updated to read stdout as well, likely with .out(out) or .out(out).err(out). I am leaving this as a general PR comment because MultiEngineTest is not part of the changed diff, so GitHub will not accept it as an inline review comment. 🤖

@florian-h05
Copy link
Copy Markdown
Contributor Author

I built me a Docker container based on Ubuntu to run mx and it works fine.
I've now been able to run the tests myself and fix all remaining test failures, mx gate now succeeds.

@fniephaus fniephaus added this to the 25.1.0 Release milestone Apr 2, 2026
@graalvmbot graalvmbot merged commit 8755c97 into oracle:master Apr 2, 2026
13 checks passed
@florian-h05 florian-h05 deleted the chromeinspector-outstreams branch April 2, 2026 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

OCA Verified All contributors have signed the Oracle Contributor Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[GR-74436] Chrome Inspector Tool writes info messages to error stream

3 participants