-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[receiver/sqlquery] sqlserver integration tests, improved coverage #36922
[receiver/sqlquery] sqlserver integration tests, improved coverage #36922
Conversation
2c462cc
to
1bdd23c
Compare
dc3dc6e
to
1bdd23c
Compare
3044d01
to
bba41e4
Compare
BodyColumn: "body", | ||
AttributeColumns: []string{"attribute"}, | ||
}, | ||
type DbEngine struct { |
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.
I would inexpert that type just to be clear about it
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.
@atoulme Thank you for your comment. I'm not entirely clear on what you mean. Are you suggesting that I should simply rename this type to DbEngineUnderTest
?
bba41e4
to
da67353
Compare
Failing extension test is frequency of #37079, I've added a reference there. Unrelated to this change. |
receiver/sqlqueryreceiver/testdata/integration/sqlserver/Dockerfile
Outdated
Show resolved
Hide resolved
receiver/sqlqueryreceiver/testdata/integration/sqlserver/entrypoint.sh
Outdated
Show resolved
Hide resolved
da67353
to
49d5e36
Compare
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
49d5e36
to
272c41a
Compare
…d to sqlserver 2022
272c41a
to
b0efb0d
Compare
@crobert-1 @atoulme |
…pen-telemetry#36922) <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description Mainly added test for sqlserver, plus added coverage for logs (as requested in [comment](open-telemetry#35195 (review))) <!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. --> #### Link to tracking issue Fixes open-telemetry#29695 <!--Describe what testing was performed and which tests were added.--> #### Testing Main changes: * Added metrics tests for SqlServer * Covered MySql, SqlServer and Oracle for logs Improvements: - To avoid repetitions, changed approach to "parameterized" tests - run the same test flow for different database engines (for logs only). - Do not hardcode "localhost" and host port, get these values from testcontainers. - Replaced inserting test values into the database from running CLI commands in the container to executing SQL commands. - Updated Oracle init script to use `otel` schema instead of `sys`. Important: For Oracle and SQL Server, tests are **skipped** (tested locally) - I expect them to fail due to open-telemetry#27577 (which was not fixed but closed due to inactivity). <!--Describe the documentation added.--> <!--Please delete paragraphs that you did not use before submitting.-->
Description
Mainly added test for sqlserver, plus added coverage for logs (as requested in comment)
Link to tracking issue
Fixes #29695
Testing
Main changes:
Improvements:
otel
schema instead ofsys
.Important: For Oracle and SQL Server, tests are skipped (tested locally) - I expect them to fail due to #27577 (which was not fixed but closed due to inactivity).