Scope manager and exporter integration tests - #1224
Conversation
bengl
changed the title
| const emitter = new EventEmitter() | ||
|
|
||
| const hasSupportedAsyncLocalStorage = semver.satisfies(process.versions.node, '>=14.5 || ^12.19.0') | ||
| const hasSupportedAsyncResource = semver.satisfies(process.versions.node, '>= 14 || ^13.9.0 || ^12.19.0') |
There was a problem hiding this comment.
Shouldn't these be the exact same versions as AsyncLocalStorage?
There was a problem hiding this comment.
No, because executionAsyncResource was added before AsyncLocalStorage was.
There was a problem hiding this comment.
It's not really about when it was added, but more about when the bugs affecting us were fixed, which was in nodejs/node#33801 which affected both AsyncLocalStorage and executionAsyncResource and landed in 14.5.0.
| readline.on('line', line => { | ||
| try { | ||
| const { traces } = JSON.parse(line) | ||
| if (traces) proc.emit('logMessage', { log: traces }) |
There was a problem hiding this comment.
Can we somehow make this more robust? What if we test for example that a custom JSON logger is used by the tracer when configured? How can we validate that the JSON is actually traces? I'm also not sure that this is the right place for this parsing in any case.
There was a problem hiding this comment.
Right, I'm thinking the correct thing to do is just send log lines as events, and have any parsing be done in test code. That should be better for clarity and make it more adaptable for plugin tests or alternative logger tests later on.
| await new Promise(resolve => { | ||
| resolve() | ||
| }).then(() => { | ||
| res.end('hello, world\n') |
There was a problem hiding this comment.
The plugin attaches the span to the request object and doesn't use the scope manager, so this doesn't actually end up testing it.
There was a problem hiding this comment.
Would adding a tag to the active span suffice here then?
There was a problem hiding this comment.
What is this actually testing? If it's testing the scope manager maybe it should just be used directly.
|
@bengl is this PR still relevant? Since it's been opened a few conflicts have happened and I see some unresolved queries from Roch. |
|
I'll close this PR for now but we can consider reopening it in the future. |
3 participants
What does this PR do?
Tests that scope managers work correctly through various async operations,
re-using existing integration tests to save time.
Similarly for log exporter (agent exporter is already being tested everywhere).
Motivation
We should have integration tests for each scope manager.