Skip to content

Enable browser test stdio logging by default. NFC #24180

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Apr 23, 2025

No description provided.

@sbc100 sbc100 marked this pull request as draft April 23, 2025 22:00
@sbc100 sbc100 changed the title Browser logging by default [WIP] Browser logging by default Apr 23, 2025
@sbc100 sbc100 force-pushed the browser_logging_by_default branch 2 times, most recently from 8c26ecc to 84cb611 Compare April 24, 2025 15:35
@sbc100 sbc100 marked this pull request as ready for review April 24, 2025 15:37
@sbc100 sbc100 requested review from dschuff and kripken April 24, 2025 15:37
@sbc100
Copy link
Collaborator Author

sbc100 commented Apr 24, 2025

Assuming this works do we think its good idea? I wonder how much it would slow down tests with a lof stdio?

Since browser tests are already run iserially it think it should actually make tests failures much more easy to debug.

If we choose not to do this I guess we could instead have a mechanism for enabling this on a per-test (or per-test-run) basis?

@sbc100 sbc100 changed the title [WIP] Browser logging by default Enable browser test stdio logging by default. NFC Apr 24, 2025
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

Does this slow down the tests? I specifically worry about some test having a large amount of output. Maybe we can measure in the logs?

@sbc100
Copy link
Collaborator Author

sbc100 commented Apr 24, 2025

Looks like we raced to comment :)

@kripken
Copy link
Member

kripken commented Apr 24, 2025

Heh, yeah, great minds and all that...

The debuggability is really nice, so if you can't measure a regression here, it's probably worthwhile?

@sbc100 sbc100 force-pushed the browser_logging_by_default branch 2 times, most recently from c10a14c to bd2f849 Compare April 24, 2025 16:38
@sbc100 sbc100 force-pushed the browser_logging_by_default branch from bd2f849 to 8c30be5 Compare April 24, 2025 18:11
@sbc100
Copy link
Collaborator Author

sbc100 commented Apr 24, 2025

It seems like at some point browser tests start failing due to lack a GL context.. its almost like the browser stops supported GL for all following tests.

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.

2 participants