Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

trentm
Copy link
Contributor

@trentm trentm commented May 16, 2024

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

> opentelemetry-contrib@0.1.0 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

> opentelemetry-contrib@0.1.0 test:with-test-services
> NODE_OPTIONS='-r dotenv/config' DOTENV_CONFIG_PATH=./test/test-services.env npm test


> opentelemetry-contrib@0.1.0 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

> opentelemetry-contrib@0.1.0 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
Copy link

codecov bot commented May 16, 2024

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

Copy link
Member

@JamieDanielson JamieDanielson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

CONTRIBUTING.md Outdated
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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"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'",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

  • 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
Copy link
Contributor Author

trentm commented May 24, 2024

  • 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
Copy link
Contributor Author

trentm commented May 24, 2024

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
Copy link
Contributor Author

trentm commented May 24, 2024

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
Copy link
Contributor Author

trentm commented Jun 12, 2024

(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.

Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

CONTRIBUTING.md Outdated Show resolved Hide resolved

```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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@maryliag maryliag Jul 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

CONTRIBUTING.md Outdated Show resolved Hide resolved
```

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`).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
trentm and others added 5 commits July 30, 2024 18:34
Co-authored-by: Marylia Gutierrez <maryliag@gmail.com>
Co-authored-by: Marylia Gutierrez <maryliag@gmail.com>
The ':' happens to work with dotenv, but not with other similar env
parsers (like Node's recent --env-file).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment