fix: de-flake system tests #261
Conversation
googlebot
added
the
cla: yes
Ref: googleapis/nodejs-logging-winston#265 1. Synchronize the system test with logging-winston. Use the same structure and reliable mechanism for polling. 2. Apply the timeout uniformly across the test. 3. Use unique serviceContext for the error reporting tests to ensure concurrent executions of the tests do no race.
JustinBeckwith
added
the
kokoro:force-run
kokoro-team
removed
the
kokoro:force-run
|
LGTM once the tests are passing. |
Codecov Report
@@ Coverage Diff @@
## master #261 +/- ##
=======================================
Coverage 91.78% 91.78%
=======================================
Files 2 2
Lines 73 73
Branches 6 6
=======================================
Hits 67 67
Misses 6 6Continue to review full report at Codecov.
|
|
Needed to add another commit that fixes the remaining system tests. They were not running previously because of the errant This should be landed with |
These tests were not even running before because of the errant describe.only in logging-bunyan.ts fixed in the previous commit. Fix. Deflake.
|
This needs a re-review @googleapis/node-team. |
|
|
||
| describe('LoggingBunyan', () => { | ||
| const WRITE_CONSISTENCY_DELAY_MS = 90000; | ||
| const UUID = uuid.v4(); |
There was a problem hiding this comment.
Being picky, but couldn't this all be boiled down to:
const LOG_NAME = `${uuid.v4()}-logging-bunyan-system-test`;It looks like we have a method that only gets called once with a single string.
There was a problem hiding this comment.
I think the expectation is that we can have have more tests in this file in the future. They could potentially want to use unique log names.
The pedigree of this code is that is copied from sister winston library, which indeed has multiple tests.
I can make the change if you insist, but I do not think it is necessary.
| await delay(WRITE_CONSISTENCY_DELAY_MS); | ||
|
|
||
| const log = logging.log('bunyan_log'); | ||
| const log = logging.log(`${LOG_NAME}_applog`); |
There was a problem hiding this comment.
Why is this suffixed with _applog for middleware? For non middleware the it just uses ${LOG_NAME}. So now we have two different naming conventions for the same app's logs.
There was a problem hiding this comment.
Yes, this is suboptimal. The reason behind this is that for request bundling to work, the 'request log stream' (the log which has the http requests) has to be a different log name than the the 'application log' (where normal user logging should go). We chose to use a suffix for the applog, but it might have been better to use a suffix for the request log (since it is not user-generated).
This will be fixed when a counterpart of this PR is ported over to this library.
Note that you will still have two different log streams.
6 participants
Ref: googleapis/nodejs-logging-winston#265
structure and reliable mechanism for polling.
concurrent executions of the tests do no race.