-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Test NP logging config reload on SIGHUP #57681
Conversation
Pinging @elastic/kibana-platform (Team:Platform) |
@elasticmachine merge upstream |
The old test was not skipped before, so it must have been passing, however we know the feature is/was broken. Do we know why it was passing? A quick scan of the old test looks like it may not have been reopening the file handle? |
I think so. "Broken" means that Kibana writes in both new and old log files simultaneously. The current test is a smoke test verifying the logging system manages a new destination after SIGHUP. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could DRY this up a bit, but probably not worth it since we'll just remove the legacy one in 8.0.
|
||
if (matcher.test(contents)) { | ||
clearTimeout(timeoutHandle); | ||
Fs.unwatchFile(path); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: you could move this to a finally
on the outer promise
); | ||
|
||
it( | ||
'should recreate file handler on SIGHUP', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
'should recreate file handler on SIGHUP', | |
'should recreate file handle on SIGHUP', |
30 * second | ||
); | ||
it( | ||
'should recreate file handler on SIGHUP', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
'should recreate file handler on SIGHUP', | |
'should recreate file handle on SIGHUP', |
0f2c0bf
to
4d7cbb1
Compare
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
* TSify reload config test and simplify logic * get rid of mutable config in tests * increase timeouts * address comments
I have noticed a few timeouts in the jest integration tests in the last couple days, trying to track down what's causing them in #58543 but I suspect this PR |
Summary
Refactored tests for LP getting rid of mutability, it should slightly improve readability.
Added similar tests for the NP logging config.
For maintainers