-
Notifications
You must be signed in to change notification settings - Fork 550
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
base: main
Are you sure you want to change the base?
Changes from 4 commits
ae4886e
b8c0d0b
249dfb6
0d155a9
a21a880
d9e9ebc
10d1d6d
503f867
3b50f21
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -127,6 +127,25 @@ The conventional commit type (in PR title) is very important to automatically bu | |||||
|
||||||
There is no need to update the CHANGELOG in a PR because it will be updated as part of the release process (see [RELEASING.md](RELEASING.md) for more details). | ||||||
|
||||||
### Testing | ||||||
|
||||||
Most unit tests case be run via: | ||||||
|
||||||
```sh | ||||||
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: | ||||||
trentm marked this conversation as resolved.
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 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The value in having Or, if you are proposing that this new
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Though I agree that without reading the developer guide docs that There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||
npm run test-services:stop # stops Docker containers | ||||||
``` | ||||||
|
||||||
This set of commands works in the top-level directory to test all packages, or | ||||||
trentm marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
in a specific package directory that requires a test service (e.g. `plugins/node/opentelemetry-instrumentation-mongodb`). | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Actually that wasn't my intent. Packages that have no dep on a test-service won't typically have the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
And also add some clarification as:
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah I see, so it needs some rewording, I was confused 😅 |
||||||
|
||||||
### Benchmarks | ||||||
|
||||||
When two or more approaches must be compared, please write a benchmark in the benchmark/index.js module so that we can keep track of the most efficient algorithm. | ||||||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. curious, why this is no longer needed. Is not used anymore? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct, it is no longer used. It was used by When all packages' testing is switched over to using this new test-services stuff, then |
||
}; | ||
|
||
export function startDocker(db: keyof typeof dockerRunCmds) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,11 +34,13 @@ const instrumentation = registerInstrumentationTesting( | |
import * as mongoose from 'mongoose'; | ||
import User, { IUser, loadUsers } from './user'; | ||
import { assertSpan, getStatement } from './asserts'; | ||
import { DB_NAME, MONGO_URI } from './config'; | ||
import { shouldTest, DB_NAME, MONGO_URI } from './config'; | ||
|
||
// Please run mongodb in the background: docker run -d -p 27017:27017 -v ~/data:/data/db mongo | ||
describe('mongoose instrumentation', () => { | ||
before(async () => { | ||
if (!shouldTest) { | ||
return; | ||
} | ||
try { | ||
await mongoose.connect(MONGO_URI, { | ||
useNewUrlParser: true, | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
maybe adding those extra lines help in your case too? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
|
||
// properly when skipping tests in before() on nested describe() calls. | ||
// https://github.com/mochajs/mocha/issues/2819 | ||
if (!shouldTest) { | ||
this.skip(); | ||
} | ||
instrumentation.disable(); | ||
instrumentation.setConfig({ | ||
dbStatementSerializer: (_operation: string, payload) => | ||
|
@@ -75,6 +83,9 @@ describe('mongoose instrumentation', () => { | |
}); | ||
|
||
afterEach(async () => { | ||
if (!shouldTest) { | ||
return; | ||
} | ||
instrumentation.disable(); | ||
await User.collection.drop().catch(); | ||
}); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,12 +7,12 @@ | |
"repository": "open-telemetry/opentelemetry-js-contrib", | ||
"scripts": { | ||
"test": "nyc ts-mocha -p tsconfig.json --require '@opentelemetry/contrib-test-utils' 'test/redis.test.ts'", | ||
"test:debug": "cross-env RUN_REDIS_TESTS_LOCAL=true ts-mocha --inspect-brk --no-timeouts -p tsconfig.json 'test/redis.test.ts'", | ||
"test:local": "cross-env RUN_REDIS_TESTS_LOCAL=true npm run test", | ||
"test:docker:run": "docker run --rm -d --name otel-redis -p 63790:6379 redis:alpine", | ||
"test:docker:stop": "docker stop otel-redis", | ||
"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'", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could use a There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
"test-all-versions": "tav", | ||
"test-all-versions:local": "cross-env RUN_REDIS_TESTS_LOCAL=true npm run test-all-versions", | ||
"test-all-versions:with-test-services": "NODE_OPTIONS='-r dotenv/config' DOTENV_CONFIG_PATH=../../../test/test-services.env npm run test-all-versions", | ||
"tdd": "npm run test -- --watch-extensions ts --watch", | ||
"clean": "rimraf build/*", | ||
"lint": "eslint . --ext .ts", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -71,8 +71,7 @@ describe('[email protected]', () => { | |
const provider = new NodeTracerProvider(); | ||
const tracer = provider.getTracer('external'); | ||
let redis: typeof redisTypes; | ||
const shouldTestLocal = process.env.RUN_REDIS_TESTS_LOCAL; | ||
const shouldTest = process.env.RUN_REDIS_TESTS || shouldTestLocal; | ||
const shouldTest = process.env.RUN_REDIS_TESTS; | ||
|
||
let contextManager: AsyncHooksContextManager; | ||
beforeEach(() => { | ||
|
@@ -93,22 +92,12 @@ describe('[email protected]', () => { | |
this.skip(); | ||
} | ||
|
||
if (shouldTestLocal) { | ||
testUtils.startDocker('redis'); | ||
} | ||
|
||
redis = require('redis'); | ||
provider.addSpanProcessor(new SimpleSpanProcessor(memoryExporter)); | ||
instrumentation.setTracerProvider(provider); | ||
instrumentation.enable(); | ||
}); | ||
|
||
after(() => { | ||
if (shouldTestLocal) { | ||
testUtils.cleanUpDocker('redis'); | ||
} | ||
}); | ||
|
||
describe('#createClient()', () => { | ||
it('should propagate the current span to event handlers', done => { | ||
const span = tracer.startSpan('test span'); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
# A `docker compose` config file to run services required for testing some | ||
# packages. | ||
# | ||
# Note: This isn't used in CI. CI uses GitHub Actions' `services: ...` for | ||
# defining test services. | ||
# | ||
# Usage: | ||
# npm run test-services:start [SERVICE ...] | ||
# npm run test:with-test-services # runs test with 'test-services.env' envvars | ||
# npm run test-services:stop [SERVICE ...] | ||
|
||
name: oteljs-test-services | ||
|
||
services: | ||
redis: | ||
image: redis:7 | ||
ports: | ||
- "63790:6379" | ||
healthcheck: | ||
test: ["CMD", "redis-cli", "ping"] | ||
interval: 1s | ||
timeout: 10s | ||
retries: 30 | ||
|
||
mongo: | ||
image: mongo:7 | ||
ports: | ||
- "27017:27017" | ||
healthcheck: | ||
test: ["CMD", "mongosh", "--eval", "db.runCommand('ping').ok", "--quiet"] | ||
interval: 1s | ||
timeout: 10s | ||
retries: 30 |
There was a problem hiding this comment.
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.