From dbbe6bf61977f527f07bda5ff52a24f79578c3c9 Mon Sep 17 00:00:00 2001 From: Rauno Viskus Date: Thu, 13 Jan 2022 14:09:15 +0200 Subject: [PATCH 1/5] test: fix tests for ioredis --- .../package.json | 1 + .../test/ioredis.test.ts | 132 +++++++++--------- 2 files changed, 70 insertions(+), 63 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-ioredis/package.json b/plugins/node/opentelemetry-instrumentation-ioredis/package.json index d6be47b722..581245f1c8 100644 --- a/plugins/node/opentelemetry-instrumentation-ioredis/package.json +++ b/plugins/node/opentelemetry-instrumentation-ioredis/package.json @@ -65,6 +65,7 @@ "mocha": "7.2.0", "nyc": "15.1.0", "rimraf": "3.0.2", + "sinon": "^12.0.1", "test-all-versions": "5.0.1", "ts-mocha": "8.0.0", "typescript": "4.3.5" diff --git a/plugins/node/opentelemetry-instrumentation-ioredis/test/ioredis.test.ts b/plugins/node/opentelemetry-instrumentation-ioredis/test/ioredis.test.ts index d3dceb029e..f10e21ae5f 100644 --- a/plugins/node/opentelemetry-instrumentation-ioredis/test/ioredis.test.ts +++ b/plugins/node/opentelemetry-instrumentation-ioredis/test/ioredis.test.ts @@ -31,6 +31,7 @@ import { SimpleSpanProcessor, } from '@opentelemetry/sdk-trace-base'; import * as assert from 'assert'; +import * as sinon from 'sinon'; import * as ioredisTypes from 'ioredis'; import { IORedisInstrumentation } from '../src'; import { @@ -330,11 +331,14 @@ describe('ioredis', () => { it('should create a child span for streamify scanning', done => { const attributes = { ...DEFAULT_ATTRIBUTES, - [SemanticAttributes.DB_STATEMENT]: 'scan 0', + [SemanticAttributes.DB_STATEMENT]: 'scan 0 MATCH test-* COUNT 1000', }; const span = provider.getTracer('ioredis-test').startSpan('test span'); context.with(trace.setSpan(context.active(), span), () => { - const stream = client.scanStream(); + const stream = client.scanStream({ + count: 1000, + match: 'test-*', + }); stream .on('end', () => { assert.strictEqual(memoryExporter.getFinishedSpans().length, 1); @@ -771,26 +775,16 @@ describe('ioredis', () => { }); it('should call requestHook when set in config', async () => { - const config: IORedisInstrumentationConfig = { - requestHook: ( - span: Span, - requestInfo: IORedisRequestHookInformation - ) => { - assert.ok( - /\d{1,4}\.\d{1,4}\.\d{1,5}.*/.test( - requestInfo.moduleVersion as string - ) - ); - assert.strictEqual(requestInfo.cmdName, 'incr'); - assert.deepStrictEqual(requestInfo.cmdArgs, ['request-hook-test']); - - span.setAttribute( - 'attribute key from request hook', - 'custom value from request hook' - ); - }, - }; - instrumentation.setConfig(config); + const requestHook = sinon.spy(( + span: Span, + requestInfo: IORedisRequestHookInformation + ) => { + span.setAttribute( + 'attribute key from request hook', + 'custom value from request hook' + ); + }); + instrumentation.setConfig({ requestHook }); const span = provider.getTracer('ioredis-test').startSpan('test span'); await context.with(trace.setSpan(context.active(), span), async () => { @@ -802,22 +796,30 @@ describe('ioredis', () => { 'custom value from request hook' ); }); + + sinon.assert.calledOnce(requestHook); + const [ , requestInfo] = requestHook.firstCall.args; + assert.ok( + /\d{1,4}\.\d{1,4}\.\d{1,5}.*/.test( + requestInfo.moduleVersion as string + ) + ); + assert.strictEqual(requestInfo.cmdName, 'incr'); + assert.deepStrictEqual(requestInfo.cmdArgs, ['request-hook-test']); }); it('should ignore requestHook which throws exception', async () => { - const config: IORedisInstrumentationConfig = { - requestHook: ( - span: Span, - _requestInfo: IORedisRequestHookInformation - ) => { - span.setAttribute( - 'attribute key BEFORE exception', - 'this attribute is added to span BEFORE exception is thrown thus we can expect it' - ); - throw Error('error thrown in requestHook'); - }, - }; - instrumentation.setConfig(config); + const requestHook = sinon.spy(( + span: Span, + _requestInfo: IORedisRequestHookInformation + ) => { + span.setAttribute( + 'attribute key BEFORE exception', + 'this attribute is added to span BEFORE exception is thrown thus we can expect it' + ); + throw Error('error thrown in requestHook'); + }); + instrumentation.setConfig({ requestHook }); const span = provider.getTracer('ioredis-test').startSpan('test span'); await context.with(trace.setSpan(context.active(), span), async () => { @@ -829,26 +831,23 @@ describe('ioredis', () => { 'this attribute is added to span BEFORE exception is thrown thus we can expect it' ); }); + + sinon.assert.threw(requestHook); }); it('should call responseHook when set in config', async () => { - const config: IORedisInstrumentationConfig = { - responseHook: ( - span: Span, - cmdName: string, - _cmdArgs: Array, - response: unknown - ) => { - assert.strictEqual(cmdName, 'incr'); - // the command is 'incr' on a key which does not exist, thus it increase 0 by 1 and respond 1 - assert.strictEqual(response, 1); - span.setAttribute( - 'attribute key from hook', - 'custom value from hook' - ); - }, - }; - instrumentation.setConfig(config); + const responseHook = sinon.spy(( + span: Span, + cmdName: string, + _cmdArgs: Array, + response: unknown + ) => { + span.setAttribute( + 'attribute key from hook', + 'custom value from hook' + ); + }); + instrumentation.setConfig({ responseHook }); const span = provider.getTracer('ioredis-test').startSpan('test span'); await context.with(trace.setSpan(context.active(), span), async () => { @@ -860,20 +859,25 @@ describe('ioredis', () => { 'custom value from hook' ); }); + + sinon.assert.calledOnce(responseHook); + const [ , cmdName, , response] = responseHook.firstCall.args; + assert.strictEqual(cmdName, 'incr'); + // the command is 'incr' on a key which does not exist, thus it increase 0 by 1 and respond 1 + // TODO: enable again + // assert.strictEqual(response, 1); }); it('should ignore responseHook which throws exception', async () => { - const config: IORedisInstrumentationConfig = { - responseHook: ( - _span: Span, - _cmdName: string, - _cmdArgs: Array, - _response: unknown - ) => { - throw Error('error thrown in responseHook'); - }, - }; - instrumentation.setConfig(config); + const responseHook = sinon.spy(( + _span: Span, + _cmdName: string, + _cmdArgs: Array, + _response: unknown + ) => { + throw Error('error thrown in responseHook'); + }); + instrumentation.setConfig({ responseHook }); const span = provider.getTracer('ioredis-test').startSpan('test span'); await context.with(trace.setSpan(context.active(), span), async () => { @@ -883,6 +887,8 @@ describe('ioredis', () => { // hook throw exception, but span should not be affected assert.strictEqual(endedSpans.length, 1); }); + + sinon.assert.threw(responseHook); }); }); From 3c224cd5054c8e371378bf87f1a00f26e4f3b579 Mon Sep 17 00:00:00 2001 From: Rauno Viskus Date: Thu, 13 Jan 2022 14:29:24 +0200 Subject: [PATCH 2/5] test: narrowed down versions we test --- plugins/node/opentelemetry-instrumentation-ioredis/.tav.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-ioredis/.tav.yml b/plugins/node/opentelemetry-instrumentation-ioredis/.tav.yml index 249e1f531b..f81815c2df 100644 --- a/plugins/node/opentelemetry-instrumentation-ioredis/.tav.yml +++ b/plugins/node/opentelemetry-instrumentation-ioredis/.tav.yml @@ -1,7 +1,7 @@ ioredis: # Ignoring v4.19.0. Tests never ends. Caused by https://github.com/luin/ioredis/pull/1219 - versions: ">1 < 4.19.0 || > 4.19.0 < 5" + versions: "^2.5.0 || ^3.2.2 || 4.14.0 || 4.14.1 || 4.16.3 || 4.17.3 || 4.18.0 || 4.19.2 || 4.19.4 || 4.22.0 || 4.24.5 || 4.26.0 || 4.27.2 || 4.27.3 || ^4.27.6" commands: npm run test # Fix missing `contrib-test-utils` package - pretest: npm run --prefix ../../../ lerna:link + pretest: npm run --prefix ../../../ lerna:link From c0aba930c6dcdaf383c08d2b1a9d5f3a5e8c62ad Mon Sep 17 00:00:00 2001 From: Rauno Viskus Date: Thu, 13 Jan 2022 14:56:03 +0200 Subject: [PATCH 3/5] test: enable checking response param in the hook test --- .../test/ioredis.test.ts | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-ioredis/test/ioredis.test.ts b/plugins/node/opentelemetry-instrumentation-ioredis/test/ioredis.test.ts index f10e21ae5f..cf3a14cd8e 100644 --- a/plugins/node/opentelemetry-instrumentation-ioredis/test/ioredis.test.ts +++ b/plugins/node/opentelemetry-instrumentation-ioredis/test/ioredis.test.ts @@ -222,6 +222,7 @@ describe('ioredis', () => { afterEach(async () => { await client.del(hashKeyName); await client.del(testKeyName); + await client.del('response-hook-test'); memoryExporter.reset(); }); @@ -851,7 +852,7 @@ describe('ioredis', () => { const span = provider.getTracer('ioredis-test').startSpan('test span'); await context.with(trace.setSpan(context.active(), span), async () => { - await client.incr('response-hook-test'); + await client.set('response-hook-test', 'test-value'); const endedSpans = memoryExporter.getFinishedSpans(); assert.strictEqual(endedSpans.length, 1); assert.strictEqual( @@ -862,10 +863,8 @@ describe('ioredis', () => { sinon.assert.calledOnce(responseHook); const [ , cmdName, , response] = responseHook.firstCall.args; - assert.strictEqual(cmdName, 'incr'); - // the command is 'incr' on a key which does not exist, thus it increase 0 by 1 and respond 1 - // TODO: enable again - // assert.strictEqual(response, 1); + assert.strictEqual(cmdName, 'set'); + assert.strictEqual(response.toString(), 'OK'); }); it('should ignore responseHook which throws exception', async () => { From 8b1431f4e7b421dd8a0c71504913e555f60f34d7 Mon Sep 17 00:00:00 2001 From: Rauno Viskus Date: Thu, 13 Jan 2022 15:25:42 +0200 Subject: [PATCH 4/5] style: fix types and lint --- .../package.json | 1 + .../test/ioredis.test.ts | 90 ++++++++++--------- 2 files changed, 47 insertions(+), 44 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-ioredis/package.json b/plugins/node/opentelemetry-instrumentation-ioredis/package.json index 581245f1c8..2a6c1eafd9 100644 --- a/plugins/node/opentelemetry-instrumentation-ioredis/package.json +++ b/plugins/node/opentelemetry-instrumentation-ioredis/package.json @@ -58,6 +58,7 @@ "@opentelemetry/sdk-trace-node": "1.0.1", "@types/mocha": "7.0.2", "@types/node": "14.17.9", + "@types/sinon": "^10.0.6", "codecov": "3.8.3", "cross-env": "7.0.3", "gts": "3.1.0", diff --git a/plugins/node/opentelemetry-instrumentation-ioredis/test/ioredis.test.ts b/plugins/node/opentelemetry-instrumentation-ioredis/test/ioredis.test.ts index cf3a14cd8e..957e97b20f 100644 --- a/plugins/node/opentelemetry-instrumentation-ioredis/test/ioredis.test.ts +++ b/plugins/node/opentelemetry-instrumentation-ioredis/test/ioredis.test.ts @@ -776,16 +776,15 @@ describe('ioredis', () => { }); it('should call requestHook when set in config', async () => { - const requestHook = sinon.spy(( - span: Span, - requestInfo: IORedisRequestHookInformation - ) => { - span.setAttribute( - 'attribute key from request hook', - 'custom value from request hook' - ); - }); - instrumentation.setConfig({ requestHook }); + const requestHook = sinon.spy( + (span: Span, requestInfo: IORedisRequestHookInformation) => { + span.setAttribute( + 'attribute key from request hook', + 'custom value from request hook' + ); + } + ); + instrumentation.setConfig( { requestHook }); const span = provider.getTracer('ioredis-test').startSpan('test span'); await context.with(trace.setSpan(context.active(), span), async () => { @@ -799,7 +798,7 @@ describe('ioredis', () => { }); sinon.assert.calledOnce(requestHook); - const [ , requestInfo] = requestHook.firstCall.args; + const [, requestInfo] = requestHook.firstCall.args; assert.ok( /\d{1,4}\.\d{1,4}\.\d{1,5}.*/.test( requestInfo.moduleVersion as string @@ -810,17 +809,16 @@ describe('ioredis', () => { }); it('should ignore requestHook which throws exception', async () => { - const requestHook = sinon.spy(( - span: Span, - _requestInfo: IORedisRequestHookInformation - ) => { - span.setAttribute( - 'attribute key BEFORE exception', - 'this attribute is added to span BEFORE exception is thrown thus we can expect it' - ); - throw Error('error thrown in requestHook'); - }); - instrumentation.setConfig({ requestHook }); + const requestHook = sinon.spy( + (span: Span, _requestInfo: IORedisRequestHookInformation) => { + span.setAttribute( + 'attribute key BEFORE exception', + 'this attribute is added to span BEFORE exception is thrown thus we can expect it' + ); + throw Error('error thrown in requestHook'); + } + ); + instrumentation.setConfig( { requestHook }); const span = provider.getTracer('ioredis-test').startSpan('test span'); await context.with(trace.setSpan(context.active(), span), async () => { @@ -837,18 +835,20 @@ describe('ioredis', () => { }); it('should call responseHook when set in config', async () => { - const responseHook = sinon.spy(( - span: Span, - cmdName: string, - _cmdArgs: Array, - response: unknown - ) => { - span.setAttribute( - 'attribute key from hook', - 'custom value from hook' - ); - }); - instrumentation.setConfig({ responseHook }); + const responseHook = sinon.spy( + ( + span: Span, + cmdName: string, + _cmdArgs: Array, + response: unknown + ) => { + span.setAttribute( + 'attribute key from hook', + 'custom value from hook' + ); + } + ); + instrumentation.setConfig( { responseHook }); const span = provider.getTracer('ioredis-test').startSpan('test span'); await context.with(trace.setSpan(context.active(), span), async () => { @@ -862,21 +862,23 @@ describe('ioredis', () => { }); sinon.assert.calledOnce(responseHook); - const [ , cmdName, , response] = responseHook.firstCall.args; + const [, cmdName, , response] = responseHook.firstCall.args as [Span, string, unknown, Buffer]; assert.strictEqual(cmdName, 'set'); assert.strictEqual(response.toString(), 'OK'); }); it('should ignore responseHook which throws exception', async () => { - const responseHook = sinon.spy(( - _span: Span, - _cmdName: string, - _cmdArgs: Array, - _response: unknown - ) => { - throw Error('error thrown in responseHook'); - }); - instrumentation.setConfig({ responseHook }); + const responseHook = sinon.spy( + ( + _span: Span, + _cmdName: string, + _cmdArgs: Array, + _response: unknown + ) => { + throw Error('error thrown in responseHook'); + } + ); + instrumentation.setConfig( { responseHook }); const span = provider.getTracer('ioredis-test').startSpan('test span'); await context.with(trace.setSpan(context.active(), span), async () => { From 1a3411967b93a8e5a35857e2020fdf19baee6e2b Mon Sep 17 00:00:00 2001 From: Rauno Viskus Date: Thu, 13 Jan 2022 15:39:05 +0200 Subject: [PATCH 5/5] style: lint again --- .../test/ioredis.test.ts | 23 +++++++++++++++---- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-ioredis/test/ioredis.test.ts b/plugins/node/opentelemetry-instrumentation-ioredis/test/ioredis.test.ts index 957e97b20f..2a346a81b1 100644 --- a/plugins/node/opentelemetry-instrumentation-ioredis/test/ioredis.test.ts +++ b/plugins/node/opentelemetry-instrumentation-ioredis/test/ioredis.test.ts @@ -784,7 +784,9 @@ describe('ioredis', () => { ); } ); - instrumentation.setConfig( { requestHook }); + instrumentation.setConfig({ + requestHook, + }); const span = provider.getTracer('ioredis-test').startSpan('test span'); await context.with(trace.setSpan(context.active(), span), async () => { @@ -818,7 +820,9 @@ describe('ioredis', () => { throw Error('error thrown in requestHook'); } ); - instrumentation.setConfig( { requestHook }); + instrumentation.setConfig({ + requestHook, + }); const span = provider.getTracer('ioredis-test').startSpan('test span'); await context.with(trace.setSpan(context.active(), span), async () => { @@ -848,7 +852,9 @@ describe('ioredis', () => { ); } ); - instrumentation.setConfig( { responseHook }); + instrumentation.setConfig({ + responseHook, + }); const span = provider.getTracer('ioredis-test').startSpan('test span'); await context.with(trace.setSpan(context.active(), span), async () => { @@ -862,7 +868,12 @@ describe('ioredis', () => { }); sinon.assert.calledOnce(responseHook); - const [, cmdName, , response] = responseHook.firstCall.args as [Span, string, unknown, Buffer]; + const [, cmdName, , response] = responseHook.firstCall.args as [ + Span, + string, + unknown, + Buffer + ]; assert.strictEqual(cmdName, 'set'); assert.strictEqual(response.toString(), 'OK'); }); @@ -878,7 +889,9 @@ describe('ioredis', () => { throw Error('error thrown in responseHook'); } ); - instrumentation.setConfig( { responseHook }); + instrumentation.setConfig({ + responseHook, + }); const span = provider.getTracer('ioredis-test').startSpan('test span'); await context.with(trace.setSpan(context.active(), span), async () => {