Skip to content

Commit 676146a

Browse files
tlhuntertlhunter
authored
test: remove manual promise handling from test (#5759)
* jsdoc and clarify test agent code
1 parent f7287a1 commit 676146a

File tree

2 files changed

+150
-111
lines changed

2 files changed

+150
-111
lines changed

‎packages/datadog-plugin-ioredis/test/index.spec.js

Lines changed: 60 additions & 81 deletions
Original file line numberDiff line numberDiff line change
@@ -29,25 +29,22 @@ describe('Plugin', () => {
2929

3030
afterEach(() => agent.close({ ritmReset: false }))
3131

32-
it('should do automatic instrumentation when using callbacks', done => {
33-
agent.assertSomeTraces(() => {}) // wait for initial info command
34-
agent.assertSomeTraces(traces => {
35-
expect(traces[0][0]).to.have.property('name', expectedSchema.outbound.opName)
36-
expect(traces[0][0]).to.have.property('service', expectedSchema.outbound.serviceName)
37-
expect(traces[0][0]).to.have.property('resource', 'get')
38-
expect(traces[0][0]).to.have.property('type', 'redis')
39-
expect(traces[0][0].meta).to.have.property('component', 'ioredis')
40-
expect(traces[0][0].meta).to.have.property('db.name', '0')
41-
expect(traces[0][0].meta).to.have.property('db.type', 'redis')
42-
expect(traces[0][0].meta).to.have.property('span.kind', 'client')
43-
expect(traces[0][0].meta).to.have.property('out.host', 'localhost')
44-
expect(traces[0][0].meta).to.have.property('redis.raw_command', 'GET foo')
45-
expect(traces[0][0].metrics).to.have.property('network.destination.port', 6379)
32+
it('should do automatic instrumentation when using callbacks', async () => {
33+
await redis.get('foo')
34+
35+
await agent.assertFirstTraceSpan(span => {
36+
expect(span).to.have.property('name', expectedSchema.outbound.opName)
37+
expect(span).to.have.property('service', expectedSchema.outbound.serviceName)
38+
expect(span).to.have.property('resource', 'get')
39+
expect(span).to.have.property('type', 'redis')
40+
expect(span.meta).to.have.property('component', 'ioredis')
41+
expect(span.meta).to.have.property('db.name', '0')
42+
expect(span.meta).to.have.property('db.type', 'redis')
43+
expect(span.meta).to.have.property('span.kind', 'client')
44+
expect(span.meta).to.have.property('out.host', 'localhost')
45+
expect(span.meta).to.have.property('redis.raw_command', 'GET foo')
46+
expect(span.metrics).to.have.property('network.destination.port', 6379)
4647
})
47-
.then(done)
48-
.catch(done)
49-
50-
redis.get('foo').catch(done)
5148
})
5249

5350
it('should run the callback in the parent context', () => {
@@ -59,49 +56,42 @@ describe('Plugin', () => {
5956
})
6057
})
6158

62-
it('should handle errors', done => {
59+
it('should handle errors', async () => {
6360
let error
6461

65-
agent.assertSomeTraces(() => {}) // wait for initial info command
66-
agent
67-
.assertSomeTraces(traces => {
68-
expect(traces[0][0]).to.have.property('error', 1)
69-
expect(traces[0][0].meta).to.have.property(ERROR_TYPE, error.name)
70-
expect(traces[0][0].meta).to.have.property(ERROR_MESSAGE, error.message)
71-
expect(traces[0][0].meta).to.have.property(ERROR_STACK, error.stack)
72-
expect(traces[0][0].meta).to.have.property('component', 'ioredis')
73-
})
74-
.then(done)
75-
.catch(done)
76-
77-
redis.set('foo', 123, 'bar')
78-
.catch(err => {
79-
error = err
80-
})
81-
})
62+
try {
63+
await redis.set('foo', 123, 'bar')
64+
} catch (err) {
65+
error = err
66+
}
8267

83-
it('should work with userland promises', done => {
84-
agent.assertSomeTraces(() => {}) // wait for initial info command
85-
agent
86-
.assertSomeTraces(traces => {
87-
expect(traces[0][0]).to.have.property('name', expectedSchema.outbound.opName)
88-
expect(traces[0][0]).to.have.property('service', expectedSchema.outbound.serviceName)
89-
expect(traces[0][0]).to.have.property('resource', 'get')
90-
expect(traces[0][0]).to.have.property('type', 'redis')
91-
expect(traces[0][0].meta).to.have.property('db.name', '0')
92-
expect(traces[0][0].meta).to.have.property('db.type', 'redis')
93-
expect(traces[0][0].meta).to.have.property('span.kind', 'client')
94-
expect(traces[0][0].meta).to.have.property('out.host', 'localhost')
95-
expect(traces[0][0].meta).to.have.property('redis.raw_command', 'GET foo')
96-
expect(traces[0][0].meta).to.have.property('component', 'ioredis')
97-
expect(traces[0][0].metrics).to.have.property('network.destination.port', 6379)
98-
})
99-
.then(done)
100-
.catch(done)
68+
await agent.assertFirstTraceSpan(span => {
69+
expect(span).to.have.property('error', 1)
70+
expect(span.meta).to.have.property(ERROR_TYPE, error.name)
71+
expect(span.meta).to.have.property(ERROR_MESSAGE, error.message)
72+
expect(span.meta).to.have.property(ERROR_STACK, error.stack)
73+
expect(span.meta).to.have.property('component', 'ioredis')
74+
})
75+
})
10176

77+
it('should work with userland promises', async () => {
10278
breakThen(Promise.prototype)
10379

104-
redis.get('foo').catch(done)
80+
await redis.get('foo')
81+
82+
await agent.assertFirstTraceSpan(span => {
83+
expect(span).to.have.property('name', expectedSchema.outbound.opName)
84+
expect(span).to.have.property('service', expectedSchema.outbound.serviceName)
85+
expect(span).to.have.property('resource', 'get')
86+
expect(span).to.have.property('type', 'redis')
87+
expect(span.meta).to.have.property('db.name', '0')
88+
expect(span.meta).to.have.property('db.type', 'redis')
89+
expect(span.meta).to.have.property('span.kind', 'client')
90+
expect(span.meta).to.have.property('out.host', 'localhost')
91+
expect(span.meta).to.have.property('redis.raw_command', 'GET foo')
92+
expect(span.meta).to.have.property('component', 'ioredis')
93+
expect(span.metrics).to.have.property('network.destination.port', 6379)
94+
})
10595
})
10696

10797
withNamingSchema(
@@ -119,27 +109,20 @@ describe('Plugin', () => {
119109

120110
after(() => agent.close({ ritmReset: false }))
121111

122-
it('should be configured with the correct values', done => {
123-
agent
124-
.assertSomeTraces(traces => {
125-
expect(traces[0][0]).to.have.property('service', 'custom-test')
126-
})
127-
.then(done)
128-
.catch(done)
112+
it('should be configured with the correct values', async () => {
113+
await redis.get('foo')
129114

130-
redis.get('foo').catch(done)
115+
agent.assertFirstTraceSpan(span => {
116+
expect(span).to.have.property('service', 'custom-test')
117+
})
131118
})
132119

133-
it('should be able to filter commands', done => {
134-
agent.assertSomeTraces(() => {}) // wait for initial command
135-
agent
136-
.assertSomeTraces(traces => {
137-
expect(traces[0][0]).to.have.property('resource', 'get')
138-
})
139-
.then(done)
140-
.catch(done)
120+
it('should be able to filter commands', async () => {
121+
await redis.get('foo')
141122

142-
redis.get('foo').catch(done)
123+
await agent.assertFirstTraceSpan(span => {
124+
expect(span).to.have.property('resource', 'get')
125+
})
143126
})
144127

145128
withNamingSchema(
@@ -164,16 +147,12 @@ describe('Plugin', () => {
164147

165148
after(() => agent.close({ ritmReset: false }))
166149

167-
it('should be able to filter commands', done => {
168-
agent.assertSomeTraces(() => {}) // wait for initial command
169-
agent
170-
.assertSomeTraces(traces => {
171-
expect(traces[0][0]).to.have.property('resource', 'get')
172-
})
173-
.then(done)
174-
.catch(done)
150+
it('should be able to filter commands', async () => {
151+
await redis.get('foo')
175152

176-
redis.get('foo').catch(done)
153+
await agent.assertFirstTraceSpan(span => {
154+
expect(span).to.have.property('resource', 'get')
155+
})
177156
})
178157
})
179158
})

‎packages/dd-trace/test/plugins/agent.js

Lines changed: 90 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -234,36 +234,37 @@ let availableEndpoints = DEFAULT_AVAILABLE_ENDPOINTS
234234
* @param {Set} [handlers] - Set of handlers to add the callback to.
235235
* @returns {Promise<void>} A promise resolving if expectations are met
236236
*/
237-
function runCallback (callback, options, handlers) {
238-
const deferred = {}
239-
const promise = new Promise((resolve, reject) => {
240-
deferred.resolve = resolve
241-
deferred.reject = reject
242-
})
237+
function runCallbackAgainstTraces (callback, options, handlers) {
238+
let error
243239

244-
const timeoutMs = options !== null && typeof options === 'object' && options.timeoutMs ? options.timeoutMs : 1000
240+
let resolve
241+
let reject
242+
const promise = new Promise((_resolve, _reject) => {
243+
resolve = _resolve
244+
reject = _reject
245+
})
245246

246-
const timeout = setTimeout(() => {
247-
if (error) {
248-
deferred.reject(error)
249-
}
250-
}, timeoutMs)
247+
const rejectionTimeout = setTimeout(() => {
248+
if (error) reject(error)
249+
}, options?.timeoutMs || 1000)
251250

252-
let error
253-
const handlerPayload = { handler, spanResourceMatch: options && options.spanResourceMatch }
251+
const handlerPayload = {
252+
handler,
253+
spanResourceMatch: options?.spanResourceMatch
254+
}
254255

255256
function handler () {
256257
try {
257258
const result = callback.apply(null, arguments)
258259
handlers.delete(handlerPayload)
259-
clearTimeout(timeout)
260-
deferred.resolve(result)
260+
clearTimeout(rejectionTimeout)
261+
resolve(result)
261262
} catch (e) {
262-
if (options && options.rejectFirst) {
263-
clearTimeout(timeout)
264-
deferred.reject(e)
263+
if (options?.rejectFirst) {
264+
clearTimeout(rejectionTimeout)
265+
reject(e)
265266
} else {
266-
error = error || e
267+
error = error || e // if no spans match we report exactly the first mismatch error (which is unintuitive)
267268
}
268269
}
269270
}
@@ -275,7 +276,14 @@ function runCallback (callback, options, handlers) {
275276
}
276277

277278
module.exports = {
278-
// Load the plugin on the tracer with an optional config and start a mock agent.
279+
/**
280+
* Load the plugin on the tracer with an optional config and start a mock agent.
281+
*
282+
* @param {String|Array<String>} pluginName - Name or list of names of plugins to load
283+
* @param {Record<string, unknown>} config
284+
* @param {Record<string, unknown>} tracerConfig
285+
* @returns Promise<void>
286+
*/
279287
async load (pluginName, config, tracerConfig = {}) {
280288
tracer = require('../..')
281289
agent = express()
@@ -336,7 +344,7 @@ module.exports = {
336344

337345
server.on('connection', socket => sockets.push(socket))
338346

339-
const promise = new Promise((resolve, reject) => {
347+
const promise = new Promise((resolve, _reject) => {
340348
listener = server.listen(0, () => {
341349
const port = listener.address().port
342350

@@ -381,37 +389,89 @@ module.exports = {
381389
}
382390
},
383391

384-
// Register handler to be executed each agent call, multiple times
392+
/**
393+
* Register handler to be executed on each agent call, multiple times
394+
* @param {Function} handler
395+
*/
385396
subscribe (handler) {
386-
traceHandlers.add({ handler })
397+
traceHandlers.add({ handler }) // TODO: SHOULD BE .add(handler) SO WE CAN DELETE
387398
},
388399

389-
// Remove a handler
400+
/**
401+
* Remove a handler (TODO: THIS DOES NOTHING)
402+
* @param {Function} handler
403+
*/
390404
unsubscribe (handler) {
391405
traceHandlers.delete(handler)
392406
},
393407

394408
/**
395-
* Register a callback with expectations to be run on every tracing payload sent to the agent.
409+
* Callback for running test assertions against traces.
410+
*
411+
* @callback testAssertionTracesCallback
412+
* @param {Array.<Array.<span>>} traces - For a given payload, an array of traces, each trace is an array of spans.
413+
*/
414+
415+
/**
416+
* Callback for running test assertions against a span.
417+
*
418+
* @callback testAssertionSpanCallback
419+
* @param {span} span - For a given payload, the first span of the first trace.
420+
*/
421+
422+
/**
423+
* This callback gets executed once for every payload received by the agent.
424+
* It calls the callback with a `traces` argument which is an array of traces.
425+
* Each of these traces is an array of spans.
426+
*
427+
* @param {testAssertionTracesCallback} callback - runs once per agent payload
428+
* @param {Object} [options] - An options object
429+
* @param {number} [options.timeoutMs=1000] - The timeout in ms.
430+
* @param {boolean} [options.rejectFirst=false] - If true, reject the first time the callback throws.
431+
* @returns Promise
396432
*/
397433
assertSomeTraces (callback, options) {
398-
return runCallback(callback, options, traceHandlers)
434+
return runCallbackAgainstTraces(callback, options, traceHandlers)
435+
},
436+
437+
/**
438+
* Same as assertSomeTraces() but only provides the first span (traces[0][0])
439+
* This callback gets executed once for every payload received by the agent.
440+
441+
* @param {testAssertionSpanCallback} callback - runs once per agent payload
442+
* @param {Object} [options] - An options object
443+
* @param {number} [options.timeoutMs=1000] - The timeout in ms.
444+
* @param {boolean} [options.rejectFirst=false] - If true, reject the first time the callback throws.
445+
* @returns Promise
446+
*/
447+
assertFirstTraceSpan (callback, options) {
448+
return runCallbackAgainstTraces(function (traces) {
449+
return callback(traces[0][0])
450+
}, options, traceHandlers)
399451
},
400452

401453
/**
402454
* Register a callback with expectations to be run on every stats payload sent to the agent.
403455
*/
404456
expectPipelineStats (callback, options) {
405-
return runCallback(callback, options, statsHandlers)
457+
return runCallbackAgainstTraces(callback, options, statsHandlers)
406458
},
407459

408-
// Unregister any outstanding expectation callbacks.
460+
/**
461+
* Unregister any outstanding expectation callbacks.
462+
*/
409463
reset () {
410464
traceHandlers.clear()
411465
statsHandlers.clear()
412466
},
413467

414-
// Stop the mock agent, reset all expectations and wipe the require cache.
468+
/**
469+
* Stop the mock agent, reset all expectations and wipe the require cache.
470+
* @param {Object} opts
471+
* @param {boolean} opts.ritmReset - Resets the Require In The Middle cache. You probably don't need this.
472+
* @param {boolean} opts.wipe - Wipes tracer and non-native modules from require cache. You probably don't need this.
473+
* @returns
474+
*/
415475
close (opts = {}) {
416476
// Allow close to be called idempotent
417477
if (listener === null) {

0 commit comments

Comments
 (0)