test: add 'test-services' system for running tests locally with requisite services in Docker #2214

trentm · 2024-05-16T23:06:46Z

tl;dr:

npm run test-services:start     # starts services in Docker
npm run test:with-test-services # runs 'npm test' with envvars from test/test-services.env
npm run test-services:stop      # stops Docker containers

Closes: #2038

current status

This is a proposal for #2038. It is incomplete and still in draft so we can discuss it.
I've only handled redis and mongo services so far, and usage by instr-redis, instr-redis-4, instr-ioredis, instr-mongodb, instr-mongoose.
If we agree that this looks reasonable, I can do the rest of the instrumentations and services listed at #2038.

/cc @pichlermarc @blumamir

overview

This adds a set of test-services-related npm scripts to run test services in Docker for the instrumentation tests that need it.
It defines all the services in test/docker-compose.yaml. When complete this file will look quite similar to the services: ... section of .github/workflows/unit-test.yml.
It defines the environment vars that the tests need in ./test/test-services.env.
npm run test-services:start starts all the services in Docker.
npm run test:with-test-services runs npm test with the needed env vars from ./test/test-services.env.
npm run test-services:stop shuts down the Docker containers
Those commands work in the top-level dir, or also in the specific package dirs.
This removes the "local" testing stuff that existed in instr-redis-4 and instr-redis (e.g. npm run test:local). That was starting/stopping a redis container for each test run. That works okay for something speedy like redis, but I don't think it will work well for things like Postgres, Cassandra, etc.

example run from the top-level

Start the services:

% npm run test-services:start

> [email protected] test-services:start
> docker compose -f ./test/docker-compose.yaml up -d --wait

[+] Running 3/3
 ✔ Network oteljs-test-services_default    Created                      0.0s
 ✔ Container oteljs-test-services-mongo-1  Healthy                      0.0s
 ✔ Container oteljs-test-services-redis-1  Healthy                      0.0s


% docker ps
CONTAINER ID   IMAGE     COMMAND                  CREATED         STATUS                   PORTS                      NAMES
70f472b72e6a   redis:7   "docker-entrypoint.s…"   4 seconds ago   Up 4 seconds (healthy)   0.0.0.0:63790->6379/tcp    oteljs-test-services-redis-1
648d1c075f13   mongo:7   "docker-entrypoint.s…"   4 seconds ago   Up 4 seconds (healthy)   0.0.0.0:27017->27017/tcp   oteljs-test-services-mongo-1

Then run the tests:

test output

% npm run test:with-test-services

> [email protected] test:with-test-services
> NODE_OPTIONS='-r dotenv/config' DOTENV_CONFIG_PATH=./test/test-services.env npm test


> [email protected] test
> lerna run test

lerna notice cli v6.6.2
lerna info versioning independent

    ✔  @opentelemetry/redis-common:test (2s)
    ✔  @opentelemetry/sql-common:test (2s)
    ✔  @opentelemetry/winston-transport:test (3s)
    ✔  @opentelemetry/propagation-utils:test (3s)
    ✔  @opentelemetry/resource-detector-azure:test (3s)
    ✔  @opentelemetry/resource-detector-container:test (3s)
    ✔  @opentelemetry/resource-detector-gcp:test (3s)
    ✔  @opentelemetry/instrumentation-amqplib:test (4s)
    ✔  @opentelemetry/instrumentation-cucumber:test (4s)
    ✔  @opentelemetry/instrumentation-fs:test (3s)
    ✔  @opentelemetry/instrumentation-lru-memoizer:test (2s)
    ✔  @opentelemetry/instrumentation-socket.io:test (2s)
    ✔  @opentelemetry/resource-detector-alibaba-cloud:test (5s)
    ✔  @opentelemetry/instrumentation-dataloader:test (4s)
    ✔  @opentelemetry/instrumentation-tedious:test (3s)
    ✔  @opentelemetry/instrumentation-mongoose:test (4s)
    ✔  @opentelemetry/instrumentation-aws-lambda:test (3s)
    ✔  @opentelemetry/instrumentation-cassandra-driver:test (3s)
    ✔  @opentelemetry/instrumentation-bunyan:test (3s)
    ✔  @opentelemetry/instrumentation-connect:test (3s)
    ✔  @opentelemetry/resource-detector-aws:test (8s)
    ✔  @opentelemetry/instrumentation-dns:test (3s)
    ✔  @opentelemetry/instrumentation-generic-pool:test (3s)
    ✔  @opentelemetry/instrumentation-graphql:test (3s)
    ✔  @opentelemetry/instrumentation-fastify:test (5s)
    ✔  @opentelemetry/instrumentation-memcached:test (3s)
    ✔  @opentelemetry/instrumentation-knex:test (3s)
    ✔  @opentelemetry/instrumentation-mongodb:test (4s)
    ✔  @opentelemetry/instrumentation-koa:test (4s)
    ✔  @opentelemetry/instrumentation-undici:test (8s)
    ✔  @opentelemetry/instrumentation-mysql:test (3s)
    ✔  @opentelemetry/instrumentation-hapi:test (6s)
    ✔  @opentelemetry/instrumentation-nestjs-core:test (4s)
    ✔  @opentelemetry/instrumentation-net:test (3s)
    ✔  @opentelemetry/instrumentation-router:test (3s)
    ✔  @opentelemetry/instrumentation-redis:test (3s)
    ✔  @opentelemetry/instrumentation-restify:test (4s)
    ✔  @opentelemetry/instrumentation-redis-4:test (3s)
    ✔  @opentelemetry/instrumentation-ioredis:test (4s)
    ✔  @opentelemetry/instrumentation-pino:test (5s)
    ✔  @opentelemetry/instrumentation-mysql2:test (4s)
    ✔  @opentelemetry/instrumentation-winston:test (4s)
    ✔  @opentelemetry/instrumentation-pg:test (4s)
    ✔  @opentelemetry/baggage-span-processor:test (3s)
    ✔  @opentelemetry/resource-detector-github:test (3s)
    ✔  @opentelemetry/id-generator-aws-xray:test (3s)
    ✔  @opentelemetry/host-metrics:test (3s)
    ✔  @opentelemetry/instrumentation-runtime-node:test (3s)
    ✔  @opentelemetry/propagator-instana:test (3s)
    ✔  @opentelemetry/propagator-ot-trace:test (2s)
    ✔  @opentelemetry/instrumentation-aws-sdk:test (5s)
    ✔  @opentelemetry/instrumentation-express:test (14s)
    ✔  @opentelemetry/resource-detector-instana:test (5s)
    ✔  @opentelemetry/auto-instrumentations-node:test (2s)

 —————————————————————————————————————————————————————————————————————————————

 >  Lerna (powered by Nx)   Successfully ran target test for 54 projects (21s)

Then stop the services:

% npm run test-services:stop

> [email protected] test-services:stop
> docker compose -f ./test/docker-compose.yaml down

[+] Running 3/3
 ✔ Container oteljs-test-services-mongo-1  Removed                      0.1s
 ✔ Container oteljs-test-services-redis-1  Removed                      0.2s
 ✔ Network oteljs-test-services_default    Removed                      0.0s

open questions

Seem reasonable?
The npm script command to set the envvars is a bit burdensome: NODE_OPTIONS='-r dotenv/config' DOTENV_CONFIG_PATH=./test/test-services.env .... However, I believe it is cross-platform and will ensure those envvars are set in any Node.js process. I'd be happy to take a cleaner way. We could assume bash (sorry Windows) and do something a little simpler.

…site services in Docker tl;dr: npm run test-services:start # starts services in Docker npm run test:with-test-services # runs 'npm test' with envvars from test/test-services.env npm run test-services:stop # stops Docker containers Closes: open-telemetry#2038

…OCAL mechanism

codecov · 2024-05-16T23:47:39Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.40%. Comparing base (dfb2dff) to head (3b50f21).
Report is 198 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2214      +/-   ##
==========================================
- Coverage   90.97%   90.40%   -0.58%     
==========================================
  Files         146      149       +3     
  Lines        7492     7359     -133     
  Branches     1502     1527      +25     
==========================================
- Hits         6816     6653     -163     
- Misses        676      706      +30

see 66 files with indirect coverage changes

JamieDanielson

First pass reviewing this I like how this is laid out, and the fact that the services are still optional to run as a separate test script.
I think both mongo and redis tests are in here because they had the original test setup, but other packages should probably be done in separate PRs to limit scope.

JamieDanielson · 2024-05-23T19:56:19Z

CONTRIBUTING.md

+npm test
+```
+
+However, some instrumentations require some test-services to be running (e.g. a MongoDB server to test the `instrumentation-mongodb` package). Use the `test-services`-related npm scripts to start test services in Docker and to run the tests with the appropriate configuration to use them:


We should add Docker into the Tools used section above, noting that it's optional.

JamieDanielson · 2024-05-23T20:02:17Z

plugins/node/opentelemetry-instrumentation-redis-4/package.json

+    "test-services:start": "cd ../../.. && npm run test-services:start redis",
+    "test-services:stop": "cd ../../.. && npm run test-services:stop redis",
+    "test:with-test-services": "NODE_OPTIONS='-r dotenv/config' DOTENV_CONFIG_PATH=../../../test/test-services.env npm test",
+    "test:debug": "NODE_OPTIONS='-r dotenv/config' DOTENV_CONFIG_PATH=../../../test/test-services.env ts-mocha --inspect-brk --no-timeouts -p tsconfig.json 'test/redis.test.ts'",


We could use a .mocharc.cjs file for config options instead of specifying it in the CLI command.

Wouldn't that then get used for normal test runs as well? My understanding of the point of the test:debug -- which existed before this change -- was to run with options other than a default npm run test.

I believe mocha config files are like eslint config files where you can have multiple with custom names and pass them in wherever you need them. https://mochajs.org/#custom-locations

Given that the diff for this part of this PR is not changing how ts-mocha is called:

- "test:debug": "cross-env RUN_REDIS_TESTS_LOCAL=true ts-mocha --inspect-brk --no-timeouts -p tsconfig.json 'test/redis.test.ts'", + "test:debug": "NODE_OPTIONS='-r dotenv/config' DOTENV_CONFIG_PATH=../../../test/test-services.env ts-mocha --inspect-brk --no-timeouts -p tsconfig.json 'test/redis.test.ts'",

I think changing to .mocharc.cjs is out of scope for this change and would distract from the test-services-related changes being made. Thoughts?

Yep that's fair. I was mostly pointing out because of the long npm scripts in general that you mentioned, but otherwise agree it doesn't need to happen here.

JamieDanielson · 2024-05-23T20:11:17Z

The npm script command to set the envvars is a bit burdensome: NODE_OPTIONS='-r dotenv/config' DOTENV_CONFIG_PATH=./test/test-services.env .... However, I believe it is cross-platform and will ensure those envvars are set in any Node.js process. I'd be happy to take a cleaner way. We could assume bash (sorry Windows) and do something a little simpler

🤔 Do we have a strict requirement that tests must be able to run on Windows natively? Could we have a suggestion to use WSL, and then write these up in a separate file to keep this cleaner? Personally I'm a fan of Makefiles but I suspect that will be too big of a change here 😅

trentm · 2024-05-24T17:34:55Z

The npm script command to set the envvars is a bit burdensome: NODE_OPTIONS='-r dotenv/config' DOTENV_CONFIG_PATH=./test/test-services.env ...

[...] Personally I'm a fan of Makefiles [...]

Same, but ...

I don't think it is the place of this PR to move away from typically using npm run {something} to initiate development steps.
If we could assume Bash, then we could simplify those npm scripts without having to jump all the way to Makefiles. Something like:

    "test:with-test-services": "source ../../../test/test-services-env.sh; npm test",

vs the current and long:

    "test:with-test-services": "NODE_OPTIONS='-r dotenv/config' DOTENV_CONFIG_PATH=../../../test/test-services.env npm test",

but I don't know modern Windows dev well enough to know if that is just a middle finger to Windows devs.

(Aside: If only npm supported --env-file ... the way Node.js v20 does, then it'd be npm --env-file ../../../test/test-services.env test.)

trentm · 2024-05-24T17:41:16Z

Note that I'd considered dotenvx (https://github.com/dotenvx/dotenvx) which provides a somewhat concise:

dotenvx run -f ../../../test/test-services.env -- npm test

However, it installs 24MB worth of stuff -- grant some of those deps we already have in this repo -- so I balked. (dotenv, without the x, has no deps.) Perhaps we could reconsider dotenvx.

trentm · 2024-05-24T17:42:58Z

but other packages should probably be done in separate PRs to limit scope.

I don't care strongly either way. My bias would have been to do them all in a oner... unless the changes started getting big.

trentm · 2024-06-12T03:53:22Z

(Aside: If only npm supported --env-file ... the way Node.js v20 does, then it'd be npm --env-file ../../../test/test-services.env test.)

TIL that Node.js v22 adds a --run switch to run a package.json script. That's interesting. I think that means all the long dotenv usage will eventually (when v22 is the base version) be reduced to node --env-file ../../../test/test-services.env --run test.

maryliag

This is very helpful! Thank you working on this!!

CONTRIBUTING.md

maryliag · 2024-07-31T01:03:18Z

CONTRIBUTING.md

+
+```sh
+npm run test-services:start     # starts services in Docker
+npm run test:with-test-services # runs 'npm test' with envvars from test/test-services.env


is this command more about telling to use the env variables from the file you mentioned or does it also check if the required services are running?
Just thinking if make sense to combine this, and only call run test:with-test-services which will start the service and use the correspondent vars

npm run test:with-test-services just sets the envvars and then runs the tests.

Just thinking if make sense to combine this, and only call run test:with-test-services which will start the service and use the correspondent vars

The value in having test-services:start and test:with-test-services be separate commands is to support a dev/test cycle where one is repeatedly running the tests. It would be a time suck to start and stop services for each test run.

Or, if you are proposing that this new npm run test:and-start-services-if-necessary thing (it wouldn't have to be that awful long name :) would only start services if necessary and then run tests then:

Yes, that would avoid the time burden of starting and stopping test services for each run.

However, it creates an slight DX imbalance in that one still needs another script to stop any started test-services. Perhaps it is just a personal preference thing. I like the matching of test-services:start and test-services:end.

Also, FWIW, npm run test-services:{start,end} support arguments do the underlying docker compose ... call. So, if one is just working on the redis tests, then one can npm run test-services:start -- redis. This is relatively minor though.

Though I agree that without reading the developer guide docs that npm run test:with-test-services does potentially give the impression that it'll handle starting the test services for the developer. npm run test:and-assume-test-services-are-running-cuz-imma-use-them is a little long.

I was indeed thinking of keeping the end separate. It was just that the start seems to be a requirement for the with-test-services, so it would make sense to have them as one, since you would always run them together. I didn't think about the case where you would start but not necessarily use the with-test-services after, so make sense to have the start separate.
It would be nice if the with-test-services could start the service in case you have forgotten to start, but that could be some future improvement. I wouldn't hold any approval based on this 😄

npm run test:just-run-all-the-things

CONTRIBUTING.md

maryliag · 2024-07-31T01:08:18Z

CONTRIBUTING.md

+```
+
+This set of commands works in the top-level directory to test all packages, or
+in a specific package directory that requires a test service (e.g. `plugins/node/opentelemetry-instrumentation-mongodb`).


Suggested change

in a specific package directory that requires a test service (e.g. `plugins/node/opentelemetry-instrumentation-mongodb`).

in a specific package directory to test only that specific package (e.g. `plugins/node/opentelemetry-instrumentation-mongodb`), regardless if the tests need a test service.

regardless if the tests need a test service.

Actually that wasn't my intent. Packages that have no dep on a test-service won't typically have the npm run test:with-test-services script.

ah I see, the wording made me think that was the case, meaning I thought the set of commands was referring to the start -> with-test-services -> stop, but I guess by set you meant the group of 3 AND the npm start. Can you make this more clear? So something along the lines of:

The first option can be used in the top-level directory to test all packages, while the second option can be executed from specific package directory that requires a test service.

And also add some clarification as:

if you run the first option from the top-level, the tests with required services will (or will not? I'm not clear about that, so the clarification will definitely help here hehe) start their required services and be executed.

Er, now I'm confused. :) I was attempting to say that running all three of

npm run test-services:start # starts services in Docker npm run test:with-test-services # runs 'npm test' with envvars from test/test-services.env npm run test-services:stop # stops Docker containers

works if one is in the top-directory of the git clone. And they also work if cd'd into a specific package of the monorepo if, say, you are doing development just on that package.

Ah I see, so it needs some rewording, I was confused 😅

maryliag · 2024-07-31T01:09:13Z

packages/opentelemetry-test-utils/src/test-utils.ts

@@ -41,7 +41,6 @@ const dockerRunCmds = {
    'docker run --rm -d --name otel-mysql -p 33306:3306 -e MYSQL_ROOT_PASSWORD=rootpw -e MYSQL_DATABASE=test_db -e MYSQL_USER=otel -e MYSQL_PASSWORD=secret mysql:5.7 --log_output=TABLE --general_log=ON',
  postgres:
    'docker run --rm -d --name otel-postgres -p 54320:5432 -e POSTGRES_PASSWORD=postgres postgres:16-alpine',
-  redis: 'docker run --rm -d --name otel-redis -p 63790:6379 redis:alpine',


curious, why this is no longer needed. Is not used anymore?

Correct, it is no longer used. It was used by testUtils.startDocker('redis') calls, but those have been removed in the commits so far to this PR.

When all packages' testing is switched over to using this new test-services stuff, then startDocker, cleanUpDocker and all those dockerRunCmds entries can be removed from this file.

plugins/node/instrumentation-mongoose/test/config.ts

maryliag · 2024-07-31T01:16:04Z

plugins/node/instrumentation-mongoose/test/mongoose.test.ts

@@ -63,7 +65,13 @@ describe('mongoose instrumentation', () => {
    await mongoose.connection.close();
  });

-  beforeEach(async () => {
+  beforeEach(async function mongooseBeforeEach() {
+    // Skipping all tests in beforeEach() is a workaround. Mocha does not work


On pg tests there are some nested describes, and there there is only skip on the before and not on the before each, but is a little different:

const skip = () => { // this.skip() workaround // https://github.com/mochajs/mocha/issues/2683#issuecomment-375629901 this.test!.parent!.pending = true; this.skip(); }; if (!shouldTest) { skip(); }

maybe adding those extra lines help in your case too?

Thanks, I'll take a look.

This hunk that I added here was copying the same pattern from some of the instr-mongodb tests:

plugins/node/opentelemetry-instrumentation-mongodb/test/mongodb-v5-v6.test.ts 86: // Skipping all tests in beforeEach() is a workaround. Mocha does not work plugins/node/opentelemetry-instrumentation-mongodb/test/mongodb-v4.test.ts 77: // Skipping all tests in beforeEach() is a workaround. Mocha does not work plugins/node/opentelemetry-instrumentation-mongodb/test/mongodb-v3.test.ts 76: // Skipping all tests in beforeEach() is a workaround. Mocha does not work plugins/node/opentelemetry-instrumentation-mongodb/test/mongodb-v4-v5-v6.metrics.test.ts 106: // Skipping all tests in beforeEach() is a workaround. Mocha does not work

test/test-services.env

Co-authored-by: Marylia Gutierrez <[email protected]>

The ':' happens to work with dotenv, but not with other similar env parsers (like Node's recent --env-file).

…bool

trentm self-assigned this May 16, 2024

github-actions bot assigned blumamir, dyladan, legendecas, naseemkullah, osherv and pichlermarc May 16, 2024

github-actions bot requested a review from blumamir May 16, 2024 23:07

github-actions bot added pkg:instrumentation-mongoose pkg:instrumentation-tedious labels May 16, 2024

github-actions bot requested a review from dyladan May 16, 2024 23:07

github-actions bot added pkg:instrumentation-aws-sdk pkg:instrumentation-cassandra-driver labels May 16, 2024

github-actions bot requested a review from legendecas May 16, 2024 23:07

github-actions bot added the pkg:instrumentation-fastify label May 16, 2024

github-actions bot requested a review from naseemkullah May 16, 2024 23:07

github-actions bot added pkg:instrumentation-koa pkg:instrumentation-memcached labels May 16, 2024

github-actions bot requested a review from osherv May 16, 2024 23:07

github-actions bot added the pkg:instrumentation-mongodb label May 16, 2024

github-actions bot requested a review from pichlermarc May 16, 2024 23:07

github-actions bot added pkg:instrumentation-pg pkg:instrumentation-pino pkg:instrumentation-redis-4 pkg:instrumentation-redis pkg:instrumentation-socket.io pkg:sampler-aws-xray pkg:resource-detector-container pkg:resource-detector-azure pkg:test-utils labels May 16, 2024

trentm mentioned this pull request May 16, 2024

[internal] improve local testing experience #2038

Open

12 tasks

trentm added 3 commits May 16, 2024 16:09

lint fix

b8c0d0b

markdown lint fix

249dfb6

drop testUtils.startDocker usage in ioredis tests, no longer using _L…

0d155a9

…OCAL mechanism

JamieDanielson reviewed May 23, 2024

View reviewed changes

maryliag reviewed Jul 31, 2024

View reviewed changes

trentm and others added 5 commits July 30, 2024 18:34

better wording

a21a880

Co-authored-by: Marylia Gutierrez <[email protected]>

fix wording

d9e9ebc

Co-authored-by: Marylia Gutierrez <[email protected]>

consistently use '=' separator

10d1d6d

The ':' happens to work with dotenv, but not with other similar env parsers (like Node's recent --env-file).

use RUN_MONGODB_TESTS=true rather than 1 to avoid confusion, it is a …

503f867

…bool

Merge branch 'main' into tm-test-services

3b50f21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: add 'test-services' system for running tests locally with requisite services in Docker #2214

test: add 'test-services' system for running tests locally with requisite services in Docker #2214

trentm commented May 16, 2024

codecov bot commented May 16, 2024 •

edited

Loading

JamieDanielson left a comment

JamieDanielson May 23, 2024

JamieDanielson May 23, 2024

trentm May 24, 2024

JamieDanielson May 24, 2024

trentm May 24, 2024

JamieDanielson May 25, 2024

JamieDanielson commented May 23, 2024

trentm commented May 24, 2024

trentm commented May 24, 2024 •

edited

Loading

trentm commented May 24, 2024

trentm commented Jun 12, 2024

maryliag left a comment

maryliag Jul 31, 2024

trentm Jul 31, 2024

trentm Jul 31, 2024

maryliag Jul 31, 2024 •

edited

Loading

maryliag Jul 31, 2024

maryliag Jul 31, 2024

trentm Jul 31, 2024

maryliag Jul 31, 2024

trentm Jul 31, 2024

maryliag Jul 31, 2024

maryliag Jul 31, 2024

trentm Jul 31, 2024

maryliag Jul 31, 2024

trentm Jul 31, 2024

	in a specific package directory that requires a test service (e.g. `plugins/node/opentelemetry-instrumentation-mongodb`).
	in a specific package directory to test only that specific package (e.g. `plugins/node/opentelemetry-instrumentation-mongodb`), regardless if the tests need a test service.

test: add 'test-services' system for running tests locally with requisite services in Docker #2214

Are you sure you want to change the base?

test: add 'test-services' system for running tests locally with requisite services in Docker #2214

Conversation

trentm commented May 16, 2024

current status

overview

example run from the top-level

open questions

codecov bot commented May 16, 2024 • edited Loading

Codecov Report

JamieDanielson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamieDanielson commented May 23, 2024

trentm commented May 24, 2024

trentm commented May 24, 2024 • edited Loading

trentm commented May 24, 2024

trentm commented Jun 12, 2024

maryliag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maryliag Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 16, 2024 •

edited

Loading

trentm commented May 24, 2024 •

edited

Loading

maryliag Jul 31, 2024 •

edited

Loading