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

Adding mysql integration tests #4067

Merged
merged 2 commits into from
Dec 14, 2023

Conversation

samiura
Copy link

@samiura samiura commented Dec 14, 2023

screen

@samiura samiura requested review from a team as code owners December 14, 2023 19:51
Copy link
Contributor

@atoulme atoulme left a comment

Choose a reason for hiding this comment

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

LGTM, good to merge if tests pass

Copy link
Contributor

@crobert-1 crobert-1 left a comment

Choose a reason for hiding this comment

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

Just some minor nits, thanks for adding tests!


var mysql = []testutils.Container{testutils.NewContainer().WithContext(
path.Join(".", "testdata", "server"),
).WithName("mysql").WithExposedPorts("3306:3306").WillWaitForPorts("3306").WillWaitForLogs("X Plugin ready for connections. Bind-address:")}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: 3306 is used a few times in this file, maybe move it to a variable?

ENV MYSQL_ROOT_PASSWORD='testuser'

COPY initMySqlDB.sql /docker-entrypoint-initdb.d/

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

digest_text_limit: 120
time_limit: 24h
limit: 250
exporters:
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
exporters:
exporters:

endpoint: "${OTLP_ENDPOINT}"
tls:
insecure: true

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

- instrumentation_scope:
attributes: {}
name: otelcol/mysqlreceiver
version: <ANY>
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
version: <ANY>
version: <VERSION_FROM_BUILD>

"github.com/signalfx/splunk-otel-collector/tests/testutils"
)

var mysql = []testutils.Container{testutils.NewContainer().WithContext(
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, until this is shared no need to make global

Comment on lines 31 to 32
// This test ensures the collector can connect to a MySQL DB, and properly get metrics. It's not intended to
// test the receiver itself.
Copy link
Contributor

Choose a reason for hiding this comment

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

wading into semantics, these tests are partially intended to test the underlying components. Though some of the expectations we have about the reported telemetry may need to be updated (resource metric file changes), they can detect breaking changes or bugs that may have been missed upstream.

@samiura samiura force-pushed the adding-mysql-integration-tests branch from 38819b6 to 8f77451 Compare December 14, 2023 21:33
@samiura samiura merged commit 9584c00 into signalfx:main Dec 14, 2023
44 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 14, 2023
@samiura samiura deleted the adding-mysql-integration-tests branch March 12, 2024 17:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants