Use logp.NewNopLogger to avoid panic on test #47505
Conversation
Use logp.NewNopLogger on TestRunnerFactoryWithCommonInputSettings to avoid panic: " Log in goroutine after TestRunnerFactoryWithCommonInputSettings has completed"
belimawr
added
the
Team:Elastic-Agent-Data-Plane
belimawr
added
backport-active-all
botelastic
bot
added
the
needs_team
|
Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane) |
botelastic
bot
removed
the
needs_team
🤖 GitHub commentsJust comment with:
|
| require.NoError(t, err) | ||
|
|
||
| b := beat.Info{Logger: logptest.NewTestingLogger(t, "")} // not important for the test | ||
| b := beat.Info{Logger: logp.NewNopLogger()} // not important for the test |
There was a problem hiding this comment.
It's not obvious to me why it would panic. It's not explained in the description of this PR either.
I think it would be preferable if we made the test logger in elastic-agent-libs to never panic somehow:
There was a problem hiding this comment.
It's not obvious to me why it would panic.
logptest.NewTestingLogger uses the testing.T as the logger output, if the test ends and the logger tries to log, then the testing.T panics.
It's not explained in the description of this PR either.
It is, the explanation is in the proposed commit message:
Log in goroutine after Testxxx has completed
Use logp.NewNopLogger on TestRunnerFactoryWithCommonInputSettings to avoid panic: "
Log in goroutine after TestRunnerFactoryWithCommonInputSettings has completed"
I think it would be preferable if we made the test logger in elastic-agent-libs to never panic somehow:
The root cause of the panics are components that do not get completely shutdown and try to log something after the test ends, leading to a panic. Which actually exposes an issue with the test design or the component design.
For the test logger never to panic we "just" need to stop using zaptest.NewLogger, which I believe is the main feature of logptest.NewTestingLogger.
You have a good point there, I'll make a PR for it.
There was a problem hiding this comment.
@belimawr after merging elastic/elastic-agent-libs#368 do you still think it makes sense to make this change?
There was a problem hiding this comment.
No, if we merge elastic/elastic-agent-libs#368, we won't need this PR any more.
3 participants
Proposed commit message
Checklist
I have commented my code, particularly in hard-to-understand areasI have made corresponding changes to the documentationI have made corresponding change to the default configuration filesI have added tests that prove my fix is effective or that my feature works. Where relevant, I have used thestresstest.shscript to run them under stress conditions and race detector to verify their stability.I have added an entry in./changelog/fragmentsusing the changelog tool.## Disruptive User Impact## Author's ChecklistHow to test this PR locally
## Related issues## Use cases## Screenshots## Logs