Suppress SettingsDialog debug logs during test runs#23477
Suppress SettingsDialog debug logs during test runs#23477chandu-ops wants to merge 1 commit intogoogle-gemini:mainfrom
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request aims to improve the clarity of test output by suppressing unnecessary debug logs from the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request aims to reduce noisy test output by suppressing debug logs from the SettingsDialog. The change is effective for its stated purpose. However, I've suggested a more centralized and maintainable approach by modifying the debugLogger itself, which would provide a global solution for suppressing test logs.
| if (process.env.NODE_ENV !== 'test') { | ||
| debugLogger.log( | ||
| `[DEBUG SettingsDialog] Saving ${key} immediately with value:`, | ||
| newValue, | ||
| ); | ||
| } |
There was a problem hiding this comment.
While this change correctly suppresses the debug log, a more robust and maintainable approach would be to add this logic directly into the debugLogger implementation in packages/core/src/utils/debugLogger.ts. By making the logger's methods (e.g., log, warn) no-ops when process.env.NODE_ENV === 'test', you would centralize the logic and apply it to all calls across the codebase. This would prevent other components from producing noisy test logs and avoid repeating this environment check elsewhere. If you apply that change, the if condition here would no longer be necessary.
c2cc77f to
a465049
Compare
|
Thanks for the suggestion — that makes a lot of sense. I agree that centralizing this in I’ve updated the implementation to move this logic into the logger so it applies consistently across the codebase, while keeping the change focused on reducing test noise. |
a465049 to
d35e577
Compare
d35e577 to
0d103ab
Compare
|
Hi maintainers 👋 It looks like the required workflows are still awaiting approval, so CI hasn't started yet. Could someone please approve the workflows so the checks can run? Happy to address any feedback once CI results are available. Thanks! |
Fixes #23328
Summary
Suppress
SettingsDialogdebug logs during test runs to reduce noisy test output.Changes
debugLogger.log(...)inSettingsDialogso it does not run whenNODE_ENV === 'test'Result