fix: should print max information by default #3459
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Summary by CodeRabbit
WalkthroughLogger initialization in Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant LoggerPkg as log/logger.go
participant Stderr
App->>LoggerPkg: call Init()
LoggerPkg->>LoggerPkg: inspect os.Args for "-test.v=false"
alt "-test.v=false" present
LoggerPkg->>LoggerPkg: set defaultLogger = noopLogger
else
LoggerPkg->>Stderr: create real logger (stderr, std flags)
LoggerPkg->>LoggerPkg: set defaultLogger = real logger
end
LoggerPkg-->>App: Init complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
strowk
force-pushed
the
fix_default_logger
branch
2 times, most recently
from
0140132 to
c66c912
Compare
When tests do not fail, what they printed rarely matters. However, when they do fail, it is important to see maximum information. This change switches default behavior back as was in 0.31, when failures could be quickly triaged without adding -v
strowk
force-pushed
the
fix_default_logger
branch
from
c66c912 to
320b05e
Compare
strowk
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
log/logger.go(1 hunks)
🔇 Additional comments (2)
log/logger.go (2)
22-23: LGTM! Default logging behavior restored.This change successfully restores the default logging to stderr, which addresses the regression described in #2887. The use of
log.LstdFlagsprovides helpful timestamp context for log entries.
28-34: Fix typo in comment and confirm manual verification with issue #2887.The logic is correct and safe—the flag check only runs during test initialization and only affects test mode. However:
- Fix the typo on line 28: "Disble" → "Disable"
- Verify this resolves the regression by testing with the instructions at #2887 (comment)
The change is well-scoped (init-time only, test-mode only) and won't affect other scenarios; 16+ existing usages of
log.Default()across the codebase remain compatible.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
log/logger.go (1)
28-28: Consider removing redundant comment.The comment on line 28 repeats the information already stated in line 26. Since we're inside the
testing.Testing()block, the context is already clear.- // Disable logging if explicitly disabled via -test.v=false for _, arg := range os.Args {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
log/logger.go(1 hunks)
🔇 Additional comments (2)
log/logger.go (2)
22-23: LGTM! Restores verbose logging by default.The change from a no-op logger to an active stderr logger aligns perfectly with the PR objective to restore v0.31 behavior and addresses issue #2887 where container logs were missing on failures.
29-34: LGTM! Flag parsing logic is correct.The implementation correctly checks for
-test.v=falseformat, which is the only valid way to explicitly set the boolean test.v flag to false in Go. The logic appropriately disables logging only when this flag is explicitly present.
|
Hi @strowk I asked in #1984 about feedback, although I understand what you described in the issue and PR. This is key to me:
I'm fine in merging the changes proposed in this PR, I just want some alignment in the community about it. |
| for _, arg := range os.Args { | ||
| if strings.EqualFold(arg, "-test.v=true") || strings.EqualFold(arg, "-v") { | ||
| defaultLogger = log.New(os.Stderr, "", log.LstdFlags) | ||
| if strings.EqualFold(arg, "-test.v=false") { |
There was a problem hiding this comment.
suggestion: Create a public constructor for the noopLogger, or remove it and use log.New(io.Discard, "", 0)
There was a problem hiding this comment.
@strowk what are your thoughts on this?
3 participants
When tests do not fail, what they printed rarely matters. However, when they do fail, it is important to see maximum information. This change switches default behavior back as was in 0.31, when failures could be quickly triaged without adding -v
label: fix, qol
What does this PR do?
Simply switches default logging behavior to more convenient one in realworld scenarios.
Why is it important?
Test output matters when we need to read it, we read it when tests fail, otherwise we might not even care if they run (if it is some pipeline).
Related issues
How to test this PR
#2887 (comment)
Ideally maybe it should buffer logs and print them out only when failure happened. This could affect memory usage though, so IMO defaulting to complete output would be preferred behavior in majority of the cases.